Bug 1392130 - Ensure that when one of the Sessionstore shutdown blockers finishes and wins the race, to properly cleanup the running timer and observers. r?Yoric draft
authorMike de Boer <mdeboer@mozilla.com>
Tue, 23 Jan 2018 15:24:01 +0100
changeset 723548 0d68724c60025b44df7d67f8bb5eb16e46dd8ef4
parent 723546 c749a7beccd781d293bf431f44967e74af0b6980
child 746887 cef5edcb25218fbd2b15dd541c116646de8fb272
push id96459
push usermdeboer@mozilla.com
push dateTue, 23 Jan 2018 14:24:44 +0000
reviewersYoric
bugs1392130
milestone60.0a1
Bug 1392130 - Ensure that when one of the Sessionstore shutdown blockers finishes and wins the race, to properly cleanup the running timer and observers. r?Yoric MozReview-Commit-ID: 1K4QDCSM2oj
browser/components/sessionstore/SessionStore.jsm
toolkit/components/asyncshutdown/AsyncShutdown.jsm
--- a/browser/components/sessionstore/SessionStore.jsm
+++ b/browser/components/sessionstore/SessionStore.jsm
@@ -168,16 +168,17 @@ XPCOMUtils.defineLazyServiceGetters(this
 });
 
 XPCOMUtils.defineLazyModuleGetters(this, {
   AppConstants: "resource://gre/modules/AppConstants.jsm",
   AsyncShutdown: "resource://gre/modules/AsyncShutdown.jsm",
   DevToolsShim: "chrome://devtools-shim/content/DevToolsShim.jsm",
   GlobalState: "resource:///modules/sessionstore/GlobalState.jsm",
   PrivacyFilter: "resource:///modules/sessionstore/PrivacyFilter.jsm",
+  PromiseUtils: "resource://gre/modules/PromiseUtils.jsm",
   RecentWindow: "resource:///modules/RecentWindow.jsm",
   RunState: "resource:///modules/sessionstore/RunState.jsm",
   SessionCookies: "resource:///modules/sessionstore/SessionCookies.jsm",
   SessionFile: "resource:///modules/sessionstore/SessionFile.jsm",
   SessionSaver: "resource:///modules/sessionstore/SessionSaver.jsm",
   TabAttributes: "resource:///modules/sessionstore/TabAttributes.jsm",
   TabCrashHandler: "resource:///modules/ContentCrashHandlers.jsm",
   TabState: "resource:///modules/sessionstore/TabState.jsm",
@@ -1597,47 +1598,55 @@ var SessionStoreInternal = {
     // blocker.
     let progress = { total: -1, current: -1 };
 
     // We're going down! Switch state so that we treat closing windows and
     // tabs correctly.
     RunState.setQuitting();
 
     if (!syncShutdown) {
-      // We've got some time to shut down, so let's do this properly.
-      // To prevent blocker from breaking the 60 sec limit(which will cause a
-      // crash) of async shutdown during flushing all windows, we resolve the
-      // promise passed to blocker once:
-      // 1. the flushing exceed 50 sec, or
-      // 2. 'oop-frameloader-crashed' or 'ipc:content-shutdown' is observed.
-      // Thus, Firefox still can open the last session on next startup.
+      // We've got some time to shut down, so let's do this properly that there
+      // will be a complete session available upon next startup.
+      // To prevent a blocker from taking longer than the DELAY_CRASH_MS limit
+      // (which will cause a crash) of AsyncShutdown whilst flushing all windows,
+      // we resolve the Promise blocker once:
+      // 1. the flush duration exceeds 10 seconds before DELAY_CRASH_MS, or
+      // 2. 'oop-frameloader-crashed', or
+      // 3. 'ipc:content-shutdown' is observed.
       AsyncShutdown.quitApplicationGranted.addBlocker(
         "SessionStore: flushing all windows",
         () => {
-          var promises = [];
-          promises.push(this.flushAllWindowsAsync(progress));
-          promises.push(this.looseTimer(50000));
-
-          var promiseOFC = new Promise(resolve => {
-            Services.obs.addObserver(function obs(subject, topic) {
-              Services.obs.removeObserver(obs, topic);
-              resolve();
-            }, "oop-frameloader-crashed");
-          });
-          promises.push(promiseOFC);
-
-          var promiseICS = new Promise(resolve => {
-            Services.obs.addObserver(function obs(subject, topic) {
-              Services.obs.removeObserver(obs, topic);
-              resolve();
-            }, "ipc:content-shutdown");
-          });
-          promises.push(promiseICS);
-
-          return Promise.race(promises);
+          // Set up the list of promises that will signal a complete sessionstore
+          // shutdown: either all data is saved, or we crashed or the message IPC
+          // channel went away in the meantime.
+          let promises = [this.flushAllWindowsAsync(progress)];
+
+          const observeTopic = topic => {
+            let deferred = PromiseUtils.defer();
+            const cleanup = () => Services.obs.removeObserver(deferred.resolve, topic);
+            Services.obs.addObserver(deferred.resolve, topic);
+            deferred.promise.then(cleanup, cleanup);
+            return deferred;
+          };
+
+          // Build a list of deferred executions that require cleanup once the
+          // Promise race is won.
+          // Ensure that the timer fires earlier than the AsyncShutdown crash timer.
+          let waitTimeMaxMs = Math.max(0, AsyncShutdown.DELAY_CRASH_MS - 10000);
+          let defers = [this.looseTimer(waitTimeMaxMs),
+            observeTopic("oop-frameloader-crashed"), observeTopic("ipc:content-shutdown")];
+          // Add these monitors to the list of Promises to start the race.
+          promises.push(...defers.map(deferred => deferred.promise));
+
+          return Promise.race(promises)
+            .then(() => {
+              // When a Promise won the race, make sure we clean up the running
+              // monitors.
+              defers.forEach(deferred => deferred.reject());
+            });
         },
         () => progress);
     } else {
       // We have to shut down NOW, which means we only get to save whatever
       // we already had cached.
     }
   },
 
@@ -4793,28 +4802,27 @@ var SessionStoreInternal = {
    * up to the closest second).
    *
    * @return Promise
    */
   looseTimer(delay) {
     let DELAY_BEAT = 1000;
     let timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
     let beats = Math.ceil(delay / DELAY_BEAT);
-    let promise =  new Promise(resolve => {
-      timer.initWithCallback(function() {
-        if (beats <= 0) {
-          resolve();
-        }
-        --beats;
-      }, DELAY_BEAT, Ci.nsITimer.TYPE_REPEATING_PRECISE_CAN_SKIP);
-    });
+    let deferred = PromiseUtils.defer();
+    timer.initWithCallback(function() {
+      if (beats <= 0) {
+        deferred.resolve();
+      }
+      --beats;
+    }, DELAY_BEAT, Ci.nsITimer.TYPE_REPEATING_PRECISE_CAN_SKIP);
     // Ensure that the timer is both canceled once we are done with it
     // and not garbage-collected until then.
-    promise.then(() => timer.cancel(), () => timer.cancel());
-    return promise;
+    deferred.promise.then(() => timer.cancel(), () => timer.cancel());
+    return deferred;
   },
 
   /**
    * Send the "SessionStore:restoreHistory" message to content, triggering a
    * content restore. This method is intended to be used internally by
    * SessionStore, as it also ensures that permissions are avaliable in the
    * content process before triggering the history restore in the content
    * process.
--- a/toolkit/components/asyncshutdown/AsyncShutdown.jsm
+++ b/toolkit/components/asyncshutdown/AsyncShutdown.jsm
@@ -355,16 +355,25 @@ this.AsyncShutdown = {
    * Access function getPhase. For testing purposes only.
    */
   get _getPhase() {
     let accepted = Services.prefs.getBoolPref("toolkit.asyncshutdown.testing", false);
     if (accepted) {
       return getPhase;
     }
     return undefined;
+  },
+
+  /**
+   * This constant is used as the amount of milliseconds to allow shutdown to be
+   * blocked until we crash the process forcibly and is read from the
+   * 'toolkit.asyncshutdown.crash_timeout' pref.
+   */
+  get DELAY_CRASH_MS() {
+    return DELAY_CRASH_MS;
   }
 };
 
 /**
  * Register a new phase.
  *
  * @param {string} topic The notification topic for this Phase.
  * @see {https://developer.mozilla.org/en-US/docs/Observer_Notifications}