Bug 1251347 - Making sure that SessionFile.write initializes its workers;r=mconley
authorDavid Rajchenbach-Teller <dteller@mozilla.com>
Fri, 26 Feb 2016 12:02:43 +0100
changeset 322260 bc6e8eb75949f36b116abceb33e3281d675396b5
parent 322259 cc6dd36ae9082f827e583dbd0543d88b610f954f
child 322261 9604b2f00497587cf9cc009cdd08dff7504de998
push id5913
push userjlund@mozilla.com
push dateMon, 25 Apr 2016 16:57:49 +0000
treeherdermozilla-beta@dcaf0a6fa115 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmconley
bugs1251347, 1243549
milestone47.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 1251347 - Making sure that SessionFile.write initializes its workers;r=mconley Bug 1243549 fixed a race condition during SessionFile startup which could cause calls to SessionFile.write to send messages to the worker before it was initialized. The fix consisted in waiting until initialization was complete before proceeding. As it turns out, there are cases in which we send messages to the worker without ever attempting to initialize it, so this wait ends up causing a hang/shutdown. This patch fixes the issue by making sure that any message sent to the worker first initializes the worker if it hasn't been initialized yet. Since initializing the worker requires us reading the session store files to find out which one is valid, well, we do exactly that. MozReview-Commit-ID: 1bOgCaF6ahM
browser/components/sessionstore/SessionFile.jsm
--- a/browser/components/sessionstore/SessionFile.jsm
+++ b/browser/components/sessionstore/SessionFile.jsm
@@ -190,36 +190,44 @@ var SessionFileInternal = {
   // Number of failed calls to `write`.
   // Used for error reporting.
   _failures: 0,
 
   // Resolved once initialization is complete.
   // The promise never rejects.
   _deferredInitialized: PromiseUtils.defer(),
 
+  // `true` once we have started initialization, i.e. once something
+  // has been scheduled that will eventually resolve `_deferredInitialized`.
+  _initializationStarted: false,
+
   // The ID of the latest version of Gecko for which we have an upgrade backup
   // or |undefined| if no upgrade backup was ever written.
   get latestUpgradeBackupID() {
     try {
       return Services.prefs.getCharPref(PREF_UPGRADE_BACKUP);
     } catch (ex) {
       return undefined;
     }
   },
 
+  // Find the correct session file, read it and setup the worker.
   read: Task.async(function* () {
+    this._initializationStarted = true;
+
     let result;
     let noFilesFound = true;
     // Attempt to load by order of priority from the various backups
     for (let key of this.Paths.loadOrder) {
       let corrupted = false;
       let exists = true;
       try {
         let path = this.Paths[key];
         let startMs = Date.now();
+
         let source = yield OS.File.read(path, { encoding: "utf-8" });
         let parsed = JSON.parse(source);
 
         if (!SessionStore.isFormatVersionCompatible(parsed.version || ["sessionrestore", 0] /*fallback for old versions*/)) {
           // Skip sessionstore files that we don't understand.
           Cu.reportError("Cannot extract data from Session Restore file " + path + ". Wrong format/version: " + JSON.stringify(parsed.version) + ".");
           continue;
         }
@@ -264,29 +272,49 @@ var SessionFileInternal = {
         origin: "empty",
         source: "",
         parsed: null
       };
     }
 
     result.noFilesFound = noFilesFound;
 
-    // Initialize the worker to let it handle backups and also
+    // Initialize the worker (in the background) to let it handle backups and also
     // as a workaround for bug 964531.
-    let initialized = SessionWorker.post("init", [result.origin, this.Paths, {
+    let promiseInitialized = SessionWorker.post("init", [result.origin, this.Paths, {
       maxUpgradeBackups: Preferences.get(PREF_MAX_UPGRADE_BACKUPS, 3),
       maxSerializeBack: Preferences.get(PREF_MAX_SERIALIZE_BACK, 10),
       maxSerializeForward: Preferences.get(PREF_MAX_SERIALIZE_FWD, -1)
     }]);
 
-    initialized.catch(Promise.reject).then(() => this._deferredInitialized.resolve());
+    promiseInitialized.catch(err => {
+      // Ensure that we report errors but that they do not stop us.
+      Promise.reject(err);
+    }).then(() => this._deferredInitialized.resolve());
 
     return result;
   }),
 
+  // Post a message to the worker, making sure that it has been initialized
+  // first.
+  _postToWorker: Task.async(function*(...args) {
+    if (!this._initializationStarted) {
+      // Initializing the worker is somewhat complex, as proper handling of
+      // backups requires us to first read and check the session. Consequently,
+      // the only way to initialize the worker is to first call `this.read()`.
+
+      // The call to `this.read()` causes background initialization of the worker.
+      // Initialization will be complete once `this._deferredInitialized.promise`
+      // resolves.
+      this.read();
+    }
+    yield this._deferredInitialized.promise;
+    return SessionWorker.post(...args)
+  }),
+
   write: function (aData) {
     if (RunState.isClosed) {
       return Promise.reject(new Error("SessionFile is closed"));
     }
 
     let isFinalWrite = false;
     if (RunState.isClosing) {
       // If shutdown has started, we will want to stop receiving
@@ -295,17 +323,17 @@ var SessionFileInternal = {
       RunState.setClosed();
     }
 
     let performShutdownCleanup = isFinalWrite &&
       !sessionStartup.isAutomaticRestoreEnabled();
 
     this._attempts++;
     let options = {isFinalWrite, performShutdownCleanup};
-    let promise = this._deferredInitialized.promise.then(() => SessionWorker.post("write", [aData, options]));
+    let promise = this._postToWorker("write", [aData, options]);
 
     // Wait until the write is done.
     promise = promise.then(msg => {
       // Record how long the write took.
       this._recordTelemetry(msg.telemetry);
       this._successes++;
       if (msg.result.upgradeBackup) {
         // We have just completed a backup-on-upgrade, store the information
@@ -345,17 +373,17 @@ var SessionFileInternal = {
 
       if (isFinalWrite) {
         Services.obs.notifyObservers(null, "sessionstore-final-state-write-complete", "");
       }
     });
   },
 
   wipe: function () {
-    return this._deferredInitialized.promise.then(() => SessionWorker.post("wipe"));
+    return this._postToWorker("wipe");
   },
 
   _recordTelemetry: function(telemetry) {
     for (let id of Object.keys(telemetry)){
       let value = telemetry[id];
       let samples = [];
       if (Array.isArray(value)) {
         samples.push(...value);