Bug 1393800 - Have mochitests expecting crashes wait for the crashes to be recorded before clean up; r=mconley

This patch includes a bunch of somewhat related fixes, these are:

- Ensuring that when a mochitest calls SimpleTest.expectChildProcessCrash()
  the harness will wait for the crashes to be recorded before deleting the
  dump files. This involves a message round-trip between the content and
  parent process so to minimize its performance impact on all the non-crashing
  tests it is done only when required.
- As an additional optimization, the SimpleTest harness will not send a
  message to the content process anymore whenever it receives an
  ipc:content-shutdown event, instead it does it only for abnormal shutdowns.
- Manually fixing remaining mochitests causing crashes to wait for crashes to
  be recorded before finishing and deleting the dump files.
- Modifying BrowserTestUtils.crashBrowser() so that it optionally does not
  delete the dump files, this is useful for tests that submit their dumps and
  thus delete them on their own.


MozReview-Commit-ID: 4SLJ8BjJ18n

--HG--
extra : source : b5452a41bb962c6929292c5c538e19ac28d84fe7

browser/base/content/test/plugins/browser_pluginCrashReportNonDeterminism.js

@@ -1,6 +1,8 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/ */
 
+Cu.import("resource://gre/modules/PromiseUtils.jsm");
+
 /**
  * With e10s, plugins must run in their own process. This means we have
  * three processes at a minimum when we're running a plugin:
@@ -79,51 +81,56 @@ function preparePlugin(browser, pluginFallbackState) {
   });
 }
 
-add_task(async function setup() {
-  // Bypass click-to-play
-  setTestPluginEnabledState(Ci.nsIPluginTag.STATE_ENABLED);
+// Bypass click-to-play
+setTestPluginEnabledState(Ci.nsIPluginTag.STATE_ENABLED);
 
-  // Clear out any minidumps we create from plugins - we really don't care
-  // about them.
-  let crashObserver = (subject, topic, data) => {
-    if (topic != "plugin-crashed") {
-      return;
-    }
+// Deferred promise object used by the test to wait for the crash handler
+let crashDeferred = null;
 
-    let propBag = subject.QueryInterface(Ci.nsIPropertyBag2);
-    let minidumpID = propBag.getPropertyAsAString("pluginDumpID");
+// Clear out any minidumps we create from plugins - we really don't care
+// about them.
+let crashObserver = (subject, topic, data) => {
+  if (topic != "plugin-crashed") {
+    return;
+  }
 
-    Services.crashmanager.ensureCrashIsPresent(minidumpID).then(() => {
-      let minidumpDir = Services.dirsvc.get("ProfD", Ci.nsIFile);
-      minidumpDir.append("minidumps");
+  let propBag = subject.QueryInterface(Ci.nsIPropertyBag2);
+  let minidumpID = propBag.getPropertyAsAString("pluginDumpID");
 
-      let pluginDumpFile = minidumpDir.clone();
-      pluginDumpFile.append(minidumpID + ".dmp");
+  Services.crashmanager.ensureCrashIsPresent(minidumpID).then(() => {
+    let minidumpDir = Services.dirsvc.get("ProfD", Ci.nsIFile);
+    minidumpDir.append("minidumps");
 
-      let extraFile = minidumpDir.clone();
-      extraFile.append(minidumpID + ".extra");
+    let pluginDumpFile = minidumpDir.clone();
+    pluginDumpFile.append(minidumpID + ".dmp");
 
-      ok(pluginDumpFile.exists(), "Found minidump");
-      ok(extraFile.exists(), "Found extra file");
+    let extraFile = minidumpDir.clone();
+    extraFile.append(minidumpID + ".extra");
 
-      pluginDumpFile.remove(false);
-      extraFile.remove(false);
-    });
-  };
+    ok(pluginDumpFile.exists(), "Found minidump");
+    ok(extraFile.exists(), "Found extra file");
 
-  Services.obs.addObserver(crashObserver, "plugin-crashed");
-  // plugins.testmode will make BrowserPlugins:Test:ClearCrashData work.
-  Services.prefs.setBoolPref("plugins.testmode", true);
-  registerCleanupFunction(() => {
-    Services.prefs.clearUserPref("plugins.testmode");
-    Services.obs.removeObserver(crashObserver, "plugin-crashed");
+    pluginDumpFile.remove(false);
+    extraFile.remove(false);
+    crashDeferred.resolve();
   });
+};
+
+Services.obs.addObserver(crashObserver, "plugin-crashed");
+// plugins.testmode will make BrowserPlugins:Test:ClearCrashData work.
+Services.prefs.setBoolPref("plugins.testmode", true);
+registerCleanupFunction(() => {
+  Services.prefs.clearUserPref("plugins.testmode");
+  Services.obs.removeObserver(crashObserver, "plugin-crashed");
 });
 
 /**
  * In this case, the chrome process hears about the crash first.
  */
 add_task(async function testChromeHearsPluginCrashFirst() {
+  // Setup the crash observer promise
+  crashDeferred = PromiseUtils.defer();
+
   // Open a remote window so that we can run this test even if e10s is not
   // enabled by default.
   let win = await BrowserTestUtils.openNewBrowserWindow({remote: true});
@@ -183,12 +190,16 @@ add_task(async function testChromeHearsPluginCrashFirst() {
       "Should have been showing crash report UI");
   });
   await BrowserTestUtils.closeWindow(win);
+  await crashDeferred.promise;
 });
 
 /**
  * In this case, the content process hears about the crash first.
  */
 add_task(async function testContentHearsCrashFirst() {
+  // Setup the crash observer promise
+  crashDeferred = PromiseUtils.defer();
+
   // Open a remote window so that we can run this test even if e10s is not
   // enabled by default.
   let win = await BrowserTestUtils.openNewBrowserWindow({remote: true});
@@ -253,4 +264,5 @@ add_task(async function testContentHearsCrashFirst() {
   });
 
   await BrowserTestUtils.closeWindow(win);
+  await crashDeferred.promise;
 });

browser/base/content/test/tabcrashed/browser_clearEmail.js

@@ -34,7 +34,9 @@ add_task(async function test_clear_email() {
     prefs.setBoolPref("emailMe", true);
 
     let tab = gBrowser.getTabForBrowser(browser);
-    await BrowserTestUtils.crashBrowser(browser);
+    await BrowserTestUtils.crashBrowser(browser,
+                                        /* shouldShowTabCrashPage */ true,
+                                        /* shouldClearMinidumps */ false);
     let doc = browser.contentDocument;
 
     // Since about:tabcrashed will run in the parent process, we can safely

dom/ipc/tests/chrome.ini

@@ -2,7 +2,6 @@
 skip-if = os == 'android'
 support-files =
   process_error.xul
-  process_error_contentscript.js
 
 [test_process_error.xul]
 skip-if = !crashreporter

dom/ipc/tests/process_error.xul

@@ -7,6 +7,7 @@
   <browser id="thebrowser" type="content" remote="true" />
   <script type="application/javascript"><![CDATA[
     Components.utils.import("resource://gre/modules/Services.jsm");
+    Components.utils.import("resource://testing-common/BrowserTestUtils.jsm");
 
     const ok = window.opener.wrappedJSObject.ok;
     const is = window.opener.wrappedJSObject.is;
@@ -18,24 +19,19 @@
       ok(subject instanceof Components.interfaces.nsIPropertyBag2,
          'Subject implements nsIPropertyBag2.');
 
-      var waitCrash = Promise.resolve();
       var dumpID;
       if ('nsICrashReporter' in Components.interfaces) {
         dumpID = subject.getPropertyAsAString('dumpID');
         ok(dumpID, "dumpID is present and not an empty string");
-        waitCrash = Services.crashmanager.ensureCrashIsPresent(dumpID);
       }
 
       Services.obs.removeObserver(crashObserver, 'ipc:content-shutdown');
-      waitCrash.then(done);
+      done();
     }
 
     Services.obs.addObserver(crashObserver, 'ipc:content-shutdown');
 
-    document.getElementById('thebrowser')
-            .QueryInterface(Components.interfaces.nsIFrameLoaderOwner)
-            .frameLoader.messageManager
-            .loadFrameScript('chrome://mochitests/content/chrome/dom/ipc/tests/process_error_contentscript.js', true);
+    BrowserTestUtils.crashBrowser(document.getElementById('thebrowser'));
   ]]></script>
 
 </window>

dom/ipc/tests/process_error_contentscript.js

@@ -1,7 +0,0 @@
-Components.utils.import("resource://gre/modules/ctypes.jsm");
-
-privateNoteIntentionalCrash();
-
-var zero = new ctypes.intptr_t(8);
-var badptr = ctypes.cast(zero, ctypes.PointerType(ctypes.int32_t));
-var crash = badptr.contents;

dom/plugins/test/mochitest/hang_test.js

@@ -42,14 +42,10 @@ var testObserver = {
     let additionalDumps = extraData.additional_minidumps.split(',');
     ok(additionalDumps.indexOf('browser') >= 0, "browser in additional_minidumps");
 
-    let additionalDumpFiles = [];
     for (let name of additionalDumps) {
       let file = profD.clone();
       file.append(pluginId + "-" + name + ".dmp");
       ok(file.exists(), "additional dump '"+name+"' exists");
-      if (file.exists()) {
-        additionalDumpFiles.push(file);
-      }
     }
 
     // check cpu usage field
@@ -103,7 +99,5 @@ function onPluginCrashed(aEvent) {
            getService(Ci.nsIObserverService);
   os.removeObserver(testObserver, "plugin-crashed");
 
-  Services.crashmanager.ensureCrashIsPresent(aEvent.pluginDumpID).then(() => {
-    SimpleTest.finish();
-  });
+  SimpleTest.finish();
 }

dom/plugins/test/mochitest/test_busy_hang.xul

@@ -19,8 +19,6 @@
   </body>
   <script class="testbody" type="application/javascript">
     <![CDATA[
-Components.utils.import("resource://gre/modules/Services.jsm");
-
 SimpleTest.waitForExplicitFinish();
 SimpleTest.expectChildProcessCrash();
 

dom/plugins/test/mochitest/test_crash_notify.xul

@@ -15,7 +15,7 @@
 </body>
 <script class="testbody" type="application/javascript">
 <![CDATA[
-Components.utils.import("resource://gre/modules/Services.jsm");
+Components.utils.import("resource://gre/modules/PromiseUtils.jsm");
 
 SimpleTest.waitForExplicitFinish();
 SimpleTest.expectChildProcessCrash();
@@ -23,6 +23,8 @@ SimpleTest.expectChildProcessCrash();
 var success = false;
 
 var observerFired = false;
+var observerDeferred = PromiseUtils.defer();
+var eventListenerDeferred = PromiseUtils.defer();
 
 var testObserver = {
   observe: function(subject, topic, data) {
@@ -46,6 +48,8 @@ var testObserver = {
     let extraFile = profD.clone();
     extraFile.append(id + ".extra");
     ok(extraFile.exists(), "extra file exists");
+
+    observerDeferred.resolve();
   },
 
   QueryInterface: function(iid) {
@@ -84,9 +88,7 @@ function onPluginCrashed(aEvent) {
            getService(Components.interfaces.nsIObserverService);
   os.removeObserver(testObserver, "plugin-crashed");
 
-  Services.crashmanager.ensureCrashIsPresent(aEvent.pluginDumpID).then(() => {
-    SimpleTest.finish();
-  });
+  eventListenerDeferred.resolve();
 }
 
 function runTests() {
@@ -101,6 +103,13 @@ function runTests() {
     pluginElement.crash();
   } catch (e) {
   }
+
+  Promise.all([
+    observerDeferred.promise,
+    eventListenerDeferred.promise
+  ]).then(() => {
+    SimpleTest.finish();
+  });
 }
 ]]>
 </script>

dom/plugins/test/mochitest/test_idle_hang.xul

@@ -19,8 +19,6 @@
 </body>
 <script class="testbody" type="application/javascript">
 <![CDATA[
-Components.utils.import("resource://gre/modules/Services.jsm");
-
 SimpleTest.waitForExplicitFinish();
 SimpleTest.expectChildProcessCrash();
 

testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm

@@ -1001,12 +1001,15 @@ this.BrowserTestUtils = {
    *        True if it is expected that the tab crashed page will be shown
    *        for this browser. If so, the Promise will only resolve once the
    *        tab crash page has loaded.
+   * @param (bool) shouldClearMinidumps
+   *        True if the minidumps left behind by the crash should be removed.
    *
    * @returns (Promise)
    * @resolves An Object with key-value pairs representing the data from the
    *           crash report's extra file (if applicable).
    */
-  async crashBrowser(browser, shouldShowTabCrashPage=true) {
+  async crashBrowser(browser, shouldShowTabCrashPage=true,
+                     shouldClearMinidumps=true) {
     let extra = {};
     let KeyValueParser = {};
     if (AppConstants.MOZ_CRASHREPORTER) {
@@ -1107,8 +1110,10 @@ this.BrowserTestUtils = {
               }
             }
 
-            removeFile(minidumpDirectory, dumpID + '.dmp');
-            removeFile(minidumpDirectory, dumpID + '.extra');
+            if (shouldClearMinidumps) {
+              removeFile(minidumpDirectory, dumpID + '.dmp');
+              removeFile(minidumpDirectory, dumpID + '.extra');
+            }
           });
         }
 

testing/mochitest/tests/SimpleTest/TestRunner.js

@@ -638,8 +638,13 @@ TestRunner.testFinished = function(tests) {
     }
 
     SpecialPowers.executeAfterFlushingMessageQueue(function() {
-        cleanUpCrashDumpFiles();
-        SpecialPowers.flushPermissions(function () { SpecialPowers.flushPrefEnv(runNextTest); });
+        SpecialPowers.waitForCrashes(TestRunner._expectingProcessCrash)
+                     .then(() => {
+            cleanUpCrashDumpFiles();
+            SpecialPowers.flushPermissions(function () {
+                SpecialPowers.flushPrefEnv(runNextTest);
+            });
+        });
     });
 };
 

testing/specialpowers/content/SpecialPowersObserver.jsm

@@ -70,6 +70,7 @@ SpecialPowersObserver.prototype._loadFrameScript = function() {
   if (!this._isFrameScriptLoaded) {
     // Register for any messages our API needs us to handle
     this._messageManager.addMessageListener("SPPrefService", this);
+    this._messageManager.addMessageListener("SPProcessCrashManagerWait", this);
     this._messageManager.addMessageListener("SPProcessCrashService", this);
     this._messageManager.addMessageListener("SPPingService", this);
     this._messageManager.addMessageListener("SpecialPowers.Quit", this);
@@ -140,6 +141,7 @@ SpecialPowersObserver.prototype.uninit = function() {
 
   if (this._isFrameScriptLoaded) {
     this._messageManager.removeMessageListener("SPPrefService", this);
+    this._messageManager.removeMessageListener("SPProcessCrashManagerWait", this);
     this._messageManager.removeMessageListener("SPProcessCrashService", this);
     this._messageManager.removeMessageListener("SPPingService", this);
     this._messageManager.removeMessageListener("SpecialPowers.Quit", this);

testing/specialpowers/content/SpecialPowersObserverAPI.js

@@ -117,6 +117,10 @@ SpecialPowersObserverAPI.prototype = {
             }
           }
         } else { // ipc:content-shutdown
+          if (!aSubject.hasKey("abnormal")) {
+            return; // This is a normal shutdown, ignore it
+          }
+
           addDumpIDToMessage("dumpID");
         }
         this._sendAsyncMessage("SPProcessCrashService", message);
@@ -393,6 +397,17 @@ SpecialPowersObserverAPI.prototype = {
         return undefined; // See comment at the beginning of this function.
       }
 
+      case "SPProcessCrashManagerWait": {
+        let promises = aMessage.json.crashIds.map((crashId) => {
+          return Services.crashmanager.ensureCrashIsPresent(crashId);
+        });
+
+        Promise.all(promises).then(() => {
+          this._sendReply(aMessage, "SPProcessCrashManagerWait", {});
+        });
+        return undefined; // See comment at the beginning of this function.
+      }
+
       case "SPPermissionManager": {
         let msg = aMessage.json;
         let principal = msg.principal;

testing/specialpowers/content/specialpowers.js

@@ -49,6 +49,7 @@ function SpecialPowers(window) {
                             "SpecialPowers.RemoveFiles",
                             "SPPingService",
                             "SPLoadExtension",
+                            "SPProcessCrashManagerWait",
                             "SPStartupExtension",
                             "SPUnloadExtension",
                             "SPExtensionMessage"];

testing/specialpowers/content/specialpowersAPI.js

@@ -670,6 +670,31 @@ SpecialPowersAPI.prototype = {
     return bindDOMWindowUtils(aWindow);
   },
 
+  waitForCrashes(aExpectingProcessCrash) {
+    return new Promise((resolve, reject) => {
+      if (!aExpectingProcessCrash) {
+        resolve();
+      }
+
+      var crashIds = this._encounteredCrashDumpFiles.filter((filename) => {
+        return ((filename.length === 40) && filename.endsWith(".dmp"));
+      }).map((id) => {
+        return id.slice(0, -4); // Strip the .dmp extension to get the ID
+      });
+
+      let self = this;
+      function messageListener(msg) {
+        self._removeMessageListener("SPProcessCrashManagerWait", messageListener);
+        resolve();
+      }
+
+      this._addMessageListener("SPProcessCrashManagerWait", messageListener);
+      this._sendAsyncMessage("SPProcessCrashManagerWait", {
+        crashIds
+      });
+    });
+  },
+
   removeExpectedCrashDumpFiles(aExpectingProcessCrash) {
     var success = true;
     if (aExpectingProcessCrash) {