Bug 1427007 - Part 1: When a SessionWorker is restarted, we also need to re-initialize it with the correct SessionFile data. r?Yoric draft
authorMike de Boer <mdeboer@mozilla.com>
Fri, 02 Feb 2018 16:21:03 +0100
changeset 750523 ce6cdda382a8a6a2528ff424013b06f7f9674e93
parent 750453 0690bf69410ad52ba220d1a26e39c3a49c25cb58
child 750524 1f11ed64a94c64755e15749be2e10b09f65be5a1
child 750537 2a6b320400288033d6e4b085022b08911e1edafe
push id97697
push usermdeboer@mozilla.com
push dateFri, 02 Feb 2018 15:21:48 +0000
reviewersYoric
bugs1427007
milestone60.0a1
Bug 1427007 - Part 1: When a SessionWorker is restarted, we also need to re-initialize it with the correct SessionFile data. r?Yoric We now know that worker restarts are rather frequent and we've had reports that are certain to point at us forgetting to properly re-initialize the worker. This takes care of this by factoring the init flow into its own method and setting the flag when a failing worker is terminated. MozReview-Commit-ID: G5jccjxkBsF
browser/components/sessionstore/SessionFile.jsm
--- a/browser/components/sessionstore/SessionFile.jsm
+++ b/browser/components/sessionstore/SessionFile.jsm
@@ -32,18 +32,16 @@ const Cr = Components.results;
 
 ChromeUtils.import("resource://gre/modules/Services.jsm");
 ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");
 ChromeUtils.import("resource://gre/modules/osfile.jsm");
 ChromeUtils.import("resource://gre/modules/AsyncShutdown.jsm");
 
 ChromeUtils.defineModuleGetter(this, "console",
   "resource://gre/modules/Console.jsm");
-ChromeUtils.defineModuleGetter(this, "PromiseUtils",
-  "resource://gre/modules/PromiseUtils.jsm");
 ChromeUtils.defineModuleGetter(this, "RunState",
   "resource:///modules/sessionstore/RunState.jsm");
 ChromeUtils.defineModuleGetter(this, "TelemetryStopwatch",
   "resource://gre/modules/TelemetryStopwatch.jsm");
 XPCOMUtils.defineLazyServiceGetter(this, "Telemetry",
   "@mozilla.org/base/telemetry;1", "nsITelemetry");
 XPCOMUtils.defineLazyServiceGetter(this, "sessionStartup",
   "@mozilla.org/browser/sessionstartup;1", "nsISessionStartup");
@@ -195,37 +193,41 @@ var SessionFileInternal = {
   _failures: 0,
 
   // Object that keeps statistics that should help us make informed decisions
   // about the current status of the worker.
   _workerHealth: {
     failures: 0
   },
 
-  // Resolved once initialization is complete.
-  // The promise never rejects.
-  _deferredInitialized: PromiseUtils.defer(),
+  // `true` once we have started initialization of the worker.
+  _initializationStarted: false,
 
-  // `true` once we have started initialization, i.e. once something
-  // has been scheduled that will eventually resolve `_deferredInitialized`.
-  _initializationStarted: false,
+  // A string that will be set to the session file name part that was read from
+  // disk. It will be available _after_ a session file read() is done.
+  _readOrigin: null,
+
+  // `true` if the old, uncompressed, file format was used to read from disk, as
+  // a fallback mechanism.
+  _usingOldExtension: 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;
     }
   },
 
   async _readInternal(useOldExtension) {
     let result;
     let noFilesFound = true;
+    this._usingOldExtension = useOldExtension;
 
     // 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;
         let startMs = Date.now();
@@ -280,18 +282,16 @@ var SessionFileInternal = {
         }
       }
     }
     return {result, noFilesFound};
   },
 
   // Find the correct session file, read it and setup the worker.
   async read() {
-    this._initializationStarted = true;
-
     // Load session files with lz4 compression.
     let {result, noFilesFound} = await this._readInternal(false);
     if (!result) {
       // No result? Probably because of migration, let's
       // load uncompressed session files.
       let r = await this._readInternal(true);
       result = r.result;
     }
@@ -305,61 +305,81 @@ var SessionFileInternal = {
       // If everything fails, start with an empty session.
       result = {
         origin: "empty",
         source: "",
         parsed: null,
         useOldExtension: false
       };
     }
+    this._readOrigin = result.origin;
 
     result.noFilesFound = noFilesFound;
 
     // Initialize the worker (in the background) to let it handle backups and also
     // as a workaround for bug 964531.
-    let promiseInitialized = SessionWorker.post("init", [result.origin, result.useOldExtension, this.Paths, {
-      maxUpgradeBackups: Services.prefs.getIntPref(PREF_MAX_UPGRADE_BACKUPS, 3),
-      maxSerializeBack: Services.prefs.getIntPref(PREF_MAX_SERIALIZE_BACK, 10),
-      maxSerializeForward: Services.prefs.getIntPref(PREF_MAX_SERIALIZE_FWD, -1)
-    }]);
-
-    promiseInitialized.catch(err => {
-      // Ensure that we report errors but that they do not stop us.
-      Promise.reject(err);
-    }).then(() => this._deferredInitialized.resolve());
+    this._initWorker();
 
     return result;
   },
 
-  // Post a message to the worker, making sure that it has been initialized
-  // first.
+  // Initialize the worker in the background.
+  // Since this called _before_ any other messages are posted to the worker (see
+  // `_postToWorker()`), we know that this initialization process will be completed
+  // on time.
+  // Thus, effectively, this blocks callees on its completion.
+  // In case of a worker crash/ shutdown during its initialization phase,
+  // `_checkWorkerHealth()` will detect it and flip the `_initializationStarted`
+  // property back to `false`. This means that we'll respawn the worker upon the
+  // next request, followed by the initialization sequence here. In other words;
+  // exactly the same procedure as when the worker crashed/ shut down 'regularly'.
+  //
+  // This will never throw an error.
+  _initWorker() {
+    return new Promise(resolve => {
+      if (this._initializationStarted) {
+        resolve();
+        return;
+      }
+
+      if (!this._readOrigin) {
+        throw new Error("_initWorker called too early! Please read the session file from disk first.");
+      }
+
+      this._initializationStarted = true;
+      SessionWorker.post("init", [this._readOrigin, this._usingOldExtension, this.Paths, {
+        maxUpgradeBackups: Services.prefs.getIntPref(PREF_MAX_UPGRADE_BACKUPS, 3),
+        maxSerializeBack: Services.prefs.getIntPref(PREF_MAX_SERIALIZE_BACK, 10),
+        maxSerializeForward: Services.prefs.getIntPref(PREF_MAX_SERIALIZE_FWD, -1)
+      }]).catch(err => {
+        // Ensure that we report errors but that they do not stop us.
+        Promise.reject(err);
+      }).then(resolve);
+    });
+  },
+
+  // Post a message to the worker, making sure that it has been initialized first.
   async _postToWorker(...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();
-    }
-    await this._deferredInitialized.promise;
+    await this._initWorker();
     return SessionWorker.post(...args);
   },
 
   /**
    * For good measure, terminate the worker when we've had over `kMaxWriteFailures`
    * amount of failures to deal with. This will spawn a fresh worker upon the next
    * write.
    * This also resets the `_workerHealth` stats.
    */
   _checkWorkerHealth() {
     if (this._workerHealth.failures >= kMaxWriteFailures) {
       SessionWorker.terminate();
+      // Flag as not-initialized, to ensure that the worker state init is performed
+      // upon the next request.
+      this._initializationStarted = false;
+      // Reset the counter and report to telemetry.
       this._workerHealth.failures = 0;
       Telemetry.scalarAdd("browser.session.restore.worker_restart_count", 1);
     }
   },
 
   write(aData) {
     if (RunState.isClosed) {
       return Promise.reject(new Error("SessionFile is closed"));