Bug 910563 - gracefully handle the thumbnail process crashing. r=adw
authorMark Hammond <mhammond@skippinet.com.au>
Wed, 11 Sep 2013 13:22:57 +1000
changeset 159423 181c7727f08b9799c80cbc985d460aa817673817
parent 159422 51fbc6fcec35a1a9de86d478ab2f7029e1028d12
child 159424 dd627cf60b9e24ad08d2ef026e571b1559b013bf
push id2961
push userlsblakk@mozilla.com
push dateMon, 28 Oct 2013 21:59:28 +0000
treeherdermozilla-beta@73ef4f13486f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersadw
bugs910563
milestone26.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 910563 - gracefully handle the thumbnail process crashing. r=adw
toolkit/components/thumbnails/BackgroundPageThumbs.jsm
toolkit/components/thumbnails/test/Makefile.in
toolkit/components/thumbnails/test/browser_thumbnails_background_crash.js
toolkit/components/thumbnails/test/thumbnails_crash_content_helper.js
--- a/toolkit/components/thumbnails/BackgroundPageThumbs.jsm
+++ b/toolkit/components/thumbnails/BackgroundPageThumbs.jsm
@@ -190,16 +190,36 @@ const BackgroundPageThumbs = {
     let bwidth = Math.min(1024, swidth.value);
     // Setting the width and height attributes doesn't work -- the resulting
     // thumbnails are blank and transparent -- but setting the style does.
     browser.style.width = bwidth + "px";
     browser.style.height = (bwidth * sheight.value / swidth.value) + "px";
 
     this._parentWin.document.documentElement.appendChild(browser);
 
+    // an event that is sent if the remote process crashes - no need to remove
+    // it as we want it to be there as long as the browser itself lives.
+    browser.addEventListener("oop-browser-crashed", () => {
+      Cu.reportError("BackgroundThumbnails remote process crashed - recovering");
+      this._destroyBrowser();
+      let curCapture = this._captureQueue.length ? this._captureQueue[0] : null;
+      // we could retry the pending capture, but it's possible the crash
+      // was due directly to it, so trying again might just crash again.
+      // We could keep a flag to indicate if it previously crashed, but
+      // "resetting" the capture requires more work - so for now, we just
+      // discard it.
+      if (curCapture && curCapture.pending) {
+        curCapture._done(null);
+        // _done automatically continues queue processing.
+      }
+      // else: we must have been idle and not currently doing a capture (eg,
+      // maybe a GC or similar crashed) - so there's no need to attempt a
+      // queue restart - the next capture request will set everything up.
+    });
+
     browser.messageManager.loadFrameScript(FRAME_SCRIPT_URL, false);
     this._thumbBrowser = browser;
   },
 
   _destroyBrowser: function () {
     if (!this._thumbBrowser)
       return;
     this._thumbBrowser.remove();
@@ -221,17 +241,18 @@ const BackgroundPageThumbs = {
     this._captureQueue[0].start(this._thumbBrowser.messageManager);
     if (this._destroyBrowserTimer) {
       this._destroyBrowserTimer.cancel();
       delete this._destroyBrowserTimer;
     }
   },
 
   /**
-   * Called when the current capture completes or times out.
+   * Called when the current capture completes or fails (eg, times out, remote
+   * process crashes.)
    */
   _onCaptureOrTimeout: function (capture) {
     // Since timeouts start as an item is being processed, only the first
     // item in the queue can be passed to this method.
     if (capture !== this._captureQueue[0])
       throw new Error("The capture should be at the head of the queue.");
     this._captureQueue.shift();
     this._capturesByURL.delete(capture.url);
--- a/toolkit/components/thumbnails/test/Makefile.in
+++ b/toolkit/components/thumbnails/test/Makefile.in
@@ -17,13 +17,15 @@ MOCHITEST_BROWSER_FILES := \
 	background_red_scroll.html \
 	background_red_redirect.sjs \
 	privacy_cache_control.sjs \
 	$(NULL)
 
 ifndef RELEASE_BUILD
 MOCHITEST_BROWSER_FILES += \
 	browser_thumbnails_background.js \
+	browser_thumbnails_background_crash.js \
 	browser_thumbnails_update.js \
 	thumbnails_background.sjs \
+	thumbnails_crash_content_helper.js \
 	thumbnails_update.sjs \
 	$(NULL)
 endif
new file mode 100644
--- /dev/null
+++ b/toolkit/components/thumbnails/test/browser_thumbnails_background_crash.js
@@ -0,0 +1,162 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/ */
+
+const TEST_PAGE_URL = "http://mochi.test:8888/browser/toolkit/components/thumbnails/test/thumbnails_background.sjs";
+const TEST_CONTENT_HELPER = "chrome://mochitests/content/browser/toolkit/components/thumbnails/test/thumbnails_crash_content_helper.js";
+
+const imports = {};
+Cu.import("resource://gre/modules/BackgroundPageThumbs.jsm", imports);
+Cu.import("resource://gre/modules/PageThumbs.jsm", imports);
+Cu.import("resource://gre/modules/Task.jsm", imports);
+Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js", imports);
+
+function test() {
+  waitForExplicitFinish();
+  Services.obs.addObserver(crashObserver, 'ipc:content-shutdown', false);
+
+  spawnNextTest();
+}
+
+function spawnNextTest() {
+  if (!tests.length) {
+    Services.obs.removeObserver(crashObserver, 'ipc:content-shutdown');
+    is(numCrashes, 2, "saw 2 crashes from this test");
+    finish();
+    return;
+  }
+  let test = tests.shift();
+  info("Running sub-test " + test.name);
+  imports.Task.spawn(test).then(spawnNextTest, function onError(err) {
+    ok(false, err);
+    spawnNextTest();
+  });
+}
+
+let tests = [
+
+  function crashDuringCapture() {
+    // make a good capture first - this ensures we have the <browser>
+    let goodUrl = testPageURL();
+    yield capture(goodUrl);
+    let goodFile = fileForURL(goodUrl);
+    ok(goodFile.exists(), "Thumbnail should be cached after capture: " + goodFile.path);
+    goodFile.remove(false);
+    // inject our content script.
+    let mm = injectContentScript();
+    // queue up 2 captures - the first has a wait, so this is the one that
+    // will die.  The second one should immediately capture after the crash.
+    let waitUrl = testPageURL({ wait: 30000 });
+    let deferred1 = capture(waitUrl);
+    let deferred2 = capture(goodUrl);
+    info("Crashing the thumbnail content process.");
+    mm.sendAsyncMessage("thumbnails-test:crash");
+    yield deferred1;
+    let waitFile = fileForURL(waitUrl);
+    ok(!waitFile.exists(), "Thumbnail should not have been saved due to the crash: " + waitFile.path);
+    yield deferred2;
+    ok(goodFile.exists(), "We should have recovered and completed the 2nd capture after the crash: " + goodFile.path);
+    goodFile.remove(false);
+  },
+
+  function crashWhileIdle() {
+    // make a good capture first - this ensures we have the <browser>
+    let goodUrl = testPageURL();
+    yield capture(goodUrl);
+    let goodFile = fileForURL(goodUrl);
+    ok(goodFile.exists(), "Thumbnail should be cached after capture: " + goodFile.path);
+    goodFile.remove(false);
+    // inject our content script.
+    let mm = injectContentScript();
+    // the observer for the crashing process is basically async, so it's hard
+    // to know when the <browser> has actually seen it.  Easist is to just add
+    // our own observer.
+    let deferred = imports.Promise.defer();
+    Services.obs.addObserver(function crashObserver() {
+      Services.obs.removeObserver(crashObserver, "oop-frameloader-crashed");
+      // spin the event loop to ensure the BPT observer was called.
+      executeSoon(function() {
+        // Now queue another capture and ensure it recovers.
+        capture(goodUrl).then(() => {
+          ok(goodFile.exists(), "We should have recovered and handled new capture requests: " + goodFile.path);
+          goodFile.remove(false);
+          deferred.resolve();
+        });
+      });
+    } , "oop-frameloader-crashed", false);
+
+    // Nothing is pending - crash the process.
+    info("Crashing the thumbnail content process.");
+    mm.sendAsyncMessage("thumbnails-test:crash");
+    yield deferred.promise;
+  },
+];
+
+function injectContentScript() {
+  let thumbnailBrowser = imports.BackgroundPageThumbs._thumbBrowser;
+  let mm = thumbnailBrowser.messageManager;
+  mm.loadFrameScript(TEST_CONTENT_HELPER, false);
+  return mm;
+}
+
+function capture(url, options) {
+  let deferred = imports.Promise.defer();
+  options = options || {};
+  options.onDone = function onDone(capturedURL) {
+    deferred.resolve(capturedURL);
+  };
+  imports.BackgroundPageThumbs.capture(url, options);
+  return deferred.promise;
+}
+
+function fileForURL(url) {
+  let path = imports.PageThumbsStorage.getFilePathForURL(url);
+  let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsIFile);
+  file.initWithPath(path);
+  return file;
+}
+
+function testPageURL(opts) {
+  return TEST_PAGE_URL + "?" + encodeURIComponent(JSON.stringify(opts || {}));
+}
+
+// This observer is needed so we can clean up all evidence of the crash so
+// the testrunner thinks things are peachy.
+let numCrashes = 0;
+function crashObserver(subject, topic, data) {
+  is(topic, 'ipc:content-shutdown', 'Received correct observer topic.');
+  ok(subject instanceof Components.interfaces.nsIPropertyBag2,
+     'Subject implements nsIPropertyBag2.');
+  // we might see this called as the process terminates due to previous tests.
+  // We are only looking for "abnormal" exits...
+  if (!subject.hasKey("abnormal")) {
+    info("This is a normal termination and isn't the one we are looking for...");
+    return;
+  }
+  numCrashes++;
+
+  var dumpID;
+  if ('nsICrashReporter' in Components.interfaces) {
+    dumpID = subject.getPropertyAsAString('dumpID');
+    ok(dumpID, "dumpID is present and not an empty string");
+  }
+
+  if (dumpID) {
+    var minidumpDirectory = getMinidumpDirectory();
+    removeFile(minidumpDirectory, dumpID + '.dmp');
+    removeFile(minidumpDirectory, dumpID + '.extra');
+  }
+
+}
+
+function getMinidumpDirectory() {
+  var dir = Services.dirsvc.get('ProfD', Components.interfaces.nsIFile);
+  dir.append("minidumps");
+  return dir;
+}
+function removeFile(directory, filename) {
+  var file = directory.clone();
+  file.append(filename);
+  if (file.exists()) {
+    file.remove(false);
+  }
+}
new file mode 100644
--- /dev/null
+++ b/toolkit/components/thumbnails/test/thumbnails_crash_content_helper.js
@@ -0,0 +1,29 @@
+/* Any copyright is dedicated to the Public Domain.
+* http://creativecommons.org/publicdomain/zero/1.0/ */
+
+// Ideally we would use CrashTestUtils.jsm, but that's only available for
+// xpcshell tests - so we just copy a ctypes crasher from it.
+Cu.import("resource://gre/modules/ctypes.jsm");
+let crash = function() { // this will crash when called.
+  let zero = new ctypes.intptr_t(8);
+  let badptr = ctypes.cast(zero, ctypes.PointerType(ctypes.int32_t));
+  badptr.contents
+};
+
+
+TestHelper = {
+  init: function() {
+    addMessageListener("thumbnails-test:crash", this);
+  },
+
+  receiveMessage: function(msg) {
+    switch (msg.name) {
+      case "thumbnails-test:crash":
+        privateNoteIntentionalCrash();
+        crash();
+      break;
+    }
+  },
+}
+
+TestHelper.init();