Bug 1427007 - Part 1: When a SessionWorker is restarted, we also need to re-initialize it with the correct SessionFile data. r=Yoric a=ritu
authorMike de Boer <mdeboer@mozilla.com>
Fri, 02 Feb 2018 16:21:03 +0100
changeset 454884 aeb7a1482e4d9ecd0e4c40604c167e2dd287e19b
parent 454883 bcf52fbf84456bf8d0eb4bf85179275e4075bde5
child 454885 b89f4d21c530d5ec7d2903350745843e1215b5a6
push id1648
push usermtabara@mozilla.com
push dateThu, 01 Mar 2018 12:45:47 +0000
treeherdermozilla-release@cbb9688c2eeb [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersYoric, ritu
bugs1427007
milestone59.0
Bug 1427007 - Part 1: When a SessionWorker is restarted, we also need to re-initialize it with the correct SessionFile data. r=Yoric a=ritu 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;
 
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/osfile.jsm");
 Cu.import("resource://gre/modules/AsyncShutdown.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "console",
   "resource://gre/modules/Console.jsm");
-XPCOMUtils.defineLazyModuleGetter(this, "PromiseUtils",
-  "resource://gre/modules/PromiseUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "RunState",
   "resource:///modules/sessionstore/RunState.jsm");
 XPCOMUtils.defineLazyModuleGetter(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"));