Bug 891360 - Move SessionStore I/O logic to a dedicated worker; r=yoric

browser/components/sessionstore/src/SessionStore.jsm

@@ -460,7 +460,11 @@ let SessionStoreInternal = {
 
     // A Lazy getter for the sessionstore.js backup promise.
     XPCOMUtils.defineLazyGetter(this, "_backupSessionFileOnce", function () {
-      return _SessionFile.createBackupCopy();
+      // We're creating a backup of sessionstore.js by moving it to .bak
+      // because that's a lot faster than creating a copy. sessionstore.js
+      // would be overwritten shortly afterwards anyway so we can save time
+      // and just move instead of copy.
+      return _SessionFile.moveToBackupPath();
     });
 
     // at this point, we've as good as resumed the session, so we can
@@ -504,12 +508,12 @@ let SessionStoreInternal = {
     return Task.spawn(function task() {
       try {
         // Perform background backup
-        yield _SessionFile.createUpgradeBackupCopy("-" + buildID);
+        yield _SessionFile.createBackupCopy("-" + buildID);
 
         this._prefBranch.setCharPref(PREF_UPGRADE, buildID);
 
         // In case of success, remove previous backup.
-        yield _SessionFile.removeUpgradeBackup("-" + latestBackup);
+        yield _SessionFile.removeBackupCopy("-" + latestBackup);
       } catch (ex) {
         debug("Could not perform upgrade backup " + ex);
         debug(ex.stack);
@@ -772,12 +776,12 @@ let SessionStoreInternal = {
           this._restoreCount = this._initialState.windows ? this._initialState.windows.length : 0;
           this.restoreWindow(aWindow, this._initialState,
                              this._isCmdLineEmpty(aWindow, this._initialState));
-
-          // _loadState changed from "stopped" to "running"
-          // force a save operation so that crashes happening during startup are correctly counted
-          this._initialState.session.state = STATE_RUNNING_STR;
-          this._saveStateObject(this._initialState);
           this._initialState = null;
+
+          // _loadState changed from "stopped" to "running". Save the session's
+          // load state immediately so that crashes happening during startup
+          // are correctly counted.
+          _SessionFile.writeLoadStateOnceAfterStartup(STATE_RUNNING_STR);
         }
       }
       else {

browser/components/sessionstore/src/SessionWorker.js

@@ -0,0 +1,221 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+/**
+ * A worker dedicated to handle I/O for Session Store.
+ */
+
+"use strict";
+
+importScripts("resource://gre/modules/osfile.jsm");
+
+let File = OS.File;
+let Encoder = new TextEncoder();
+let Decoder = new TextDecoder();
+
+/**
+ * Communications with the controller.
+ *
+ * Accepts messages:
+ * {fun:function_name, args:array_of_arguments_or_null, id: custom_id}
+ *
+ * Sends messages:
+ * {ok: result, id: custom_id} / {fail: serialized_form_of_OS.File.Error,
+ *                                id: custom_id}
+ */
+self.onmessage = function (msg) {
+  let data = msg.data;
+  if (!(data.fun in Agent)) {
+    throw new Error("Cannot find method " + data.fun);
+  }
+
+  let result;
+  let id = data.id;
+
+  try {
+    result = Agent[data.fun].apply(Agent, data.args);
+  } catch (ex if ex instanceof OS.File.Error) {
+    // Instances of OS.File.Error know how to serialize themselves
+    // (deserialization ensures that we end up with OS-specific
+    // instances of |OS.File.Error|)
+    self.postMessage({fail: OS.File.Error.toMsg(ex), id: id});
+    return;
+  }
+
+  // Other exceptions do not, and should be propagated through DOM's
+  // built-in mechanism for uncaught errors, although this mechanism
+  // may lose interesting information.
+  self.postMessage({ok: result, id: id});
+};
+
+let Agent = {
+  // The initial session string as read from disk.
+  initialState: null,
+
+  // Boolean that tells whether we already wrote
+  // the loadState to disk once after startup.
+  hasWrittenLoadStateOnce: false,
+
+  // The path to sessionstore.js
+  path: OS.Path.join(OS.Constants.Path.profileDir, "sessionstore.js"),
+
+  // The path to sessionstore.bak
+  backupPath: OS.Path.join(OS.Constants.Path.profileDir, "sessionstore.bak"),
+
+  /**
+   * This method is only intended to be called by _SessionFile.syncRead() and
+   * can be removed when we're not supporting synchronous SessionStore
+   * initialization anymore. When sessionstore.js is read from disk
+   * synchronously the state string must be supplied to the worker manually by
+   * calling this method.
+   */
+  setInitialState: function (aState) {
+    // _SessionFile.syncRead() should not be called after startup has finished.
+    // Thus we also don't support any setInitialState() calls after we already
+    // wrote the loadState to disk.
+    if (this.hasWrittenLoadStateOnce) {
+      throw new Error("writeLoadStateOnceAfterStartup() must only be called once.");
+    }
+
+    // Initial state might have been filled by read() already but yet we might
+    // be called by _SessionFile.syncRead() before SessionStore.jsm had a chance
+    // to call writeLoadStateOnceAfterStartup(). It's safe to ignore
+    // setInitialState() calls if this happens.
+    if (!this.initialState) {
+      this.initialState = aState;
+    }
+  },
+
+  /**
+   * Read the session from disk.
+   * In case sessionstore.js does not exist, attempt to read sessionstore.bak.
+   */
+  read: function () {
+    for (let path of [this.path, this.backupPath]) {
+      try {
+        return this.initialState = Decoder.decode(File.read(path));
+      } catch (ex if isNoSuchFileEx(ex)) {
+        // Ignore exceptions about non-existent files.
+      }
+    }
+
+    // No sessionstore data files found. Return an empty string.
+    return "";
+  },
+
+  /**
+   * Write the session to disk.
+   */
+  write: function (stateString) {
+    let bytes = Encoder.encode(stateString);
+    return File.writeAtomic(this.path, bytes, {tmpPath: this.path + ".tmp"});
+  },
+
+  /**
+   * Writes the session state to disk again but changes session.state to
+   * 'running' before doing so. This is intended to be called only once, shortly
+   * after startup so that we detect crashes on startup correctly.
+   */
+  writeLoadStateOnceAfterStartup: function (loadState) {
+    if (this.hasWrittenLoadStateOnce) {
+      throw new Error("writeLoadStateOnceAfterStartup() must only be called once.");
+    }
+
+    if (!this.initialState) {
+      throw new Error("writeLoadStateOnceAfterStartup() must not be called " +
+                      "without a valid session state or before it has been " +
+                      "read from disk.";
+    }
+
+    // Make sure we can't call this function twice.
+    this.hasWrittenLoadStateOnce = true;
+
+    let state;
+    try {
+      state = JSON.parse(this.initialState);
+    } finally {
+      this.initialState = null;
+    }
+
+    state.session = state.session || {};
+    state.session.state = loadState;
+    return this.write(JSON.stringify(state));
+  },
+
+  /**
+   * Moves sessionstore.js to sessionstore.bak.
+   */
+  moveToBackupPath: function () {
+    try {
+      return File.move(this.path, this.backupPath);
+    } catch (ex if isNoSuchFileEx(ex)) {
+      // Ignore exceptions about non-existent files.
+      return true;
+    }
+  },
+
+  /**
+   * Creates a copy of sessionstore.js.
+   */
+  createBackupCopy: function (ext) {
+    try {
+      return File.copy(this.path, this.backupPath + ext);
+    } catch (ex if isNoSuchFileEx(ex)) {
+      // Ignore exceptions about non-existent files.
+      return true;
+    }
+  },
+
+  /**
+   * Removes a backup copy.
+   */
+  removeBackupCopy: function (ext) {
+    try {
+      return File.remove(this.backupPath + ext);
+    } catch (ex if isNoSuchFileEx(ex)) {
+      // Ignore exceptions about non-existent files.
+      return true;
+    }
+  },
+
+  /**
+   * Wipes all files holding session data from disk.
+   */
+  wipe: function () {
+    let exn;
+
+    // Erase session state file
+    try {
+      File.remove(this.path);
+    } catch (ex if isNoSuchFileEx(ex)) {
+      // Ignore exceptions about non-existent files.
+    } catch (ex) {
+      // Don't stop immediately.
+      exn = ex;
+    }
+
+    // Erase any backup, any file named "sessionstore.bak[-buildID]".
+    let iter = new File.DirectoryIterator(OS.Constants.Path.profileDir);
+    for (let entry in iter) {
+      if (!entry.isDir && entry.path.startsWith(this.backupPath)) {
+        try {
+          File.remove(entry.path);
+        } catch (ex) {
+          // Don't stop immediately.
+          exn = exn || ex;
+        }
+      }
+    }
+
+    if (exn) {
+      throw exn;
+    }
+
+    return true;
+  }
+};
+
+function isNoSuchFileEx(aReason) {
+  return aReason instanceof OS.File.Error && aReason.becauseNoSuchFile;
+}

browser/components/sessionstore/src/_SessionFile.jsm

@@ -32,6 +32,7 @@ const Ci = Components.interfaces;
 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/osfile/_PromiseWorker.jsm", this);
 Cu.import("resource://gre/modules/commonjs/sdk/core/promise.js");
 
 XPCOMUtils.defineLazyModuleGetter(this, "TelemetryStopwatch",
@@ -44,66 +45,62 @@ XPCOMUtils.defineLazyModuleGetter(this, "Task",
   "resource://gre/modules/Task.jsm");
 XPCOMUtils.defineLazyServiceGetter(this, "Telemetry",
   "@mozilla.org/base/telemetry;1", "nsITelemetry");
-
-// An encoder to UTF-8.
-XPCOMUtils.defineLazyGetter(this, "gEncoder", function () {
-  return new TextEncoder();
-});
-// A decoder.
-XPCOMUtils.defineLazyGetter(this, "gDecoder", function () {
-  return new TextDecoder();
-});
+XPCOMUtils.defineLazyModuleGetter(this, "Deprecated",
+  "resource://gre/modules/Deprecated.jsm");
 
 this._SessionFile = {
-  /**
-   * A promise fulfilled once initialization (either synchronous or
-   * asynchronous) is complete.
-   */
-  promiseInitialized: function SessionFile_initialized() {
-    return SessionFileInternal.promiseInitialized;
-  },
   /**
    * Read the contents of the session file, asynchronously.
    */
-  read: function SessionFile_read() {
+  read: function () {
     return SessionFileInternal.read();
   },
   /**
    * Read the contents of the session file, synchronously.
    */
-  syncRead: function SessionFile_syncRead() {
+  syncRead: function () {
+    Deprecated.warning(
+      "syncRead is deprecated and will be removed in a future version",
+      "https://bugzilla.mozilla.org/show_bug.cgi?id=532150")
     return SessionFileInternal.syncRead();
   },
   /**
    * Write the contents of the session file, asynchronously.
    */
-  write: function SessionFile_write(aData) {
+  write: function (aData) {
     return SessionFileInternal.write(aData);
   },
+  /**
+   * Writes the initial state to disk again only to change the session's load
+   * state. This must only be called once, it will throw an error otherwise.
+   */
+  writeLoadStateOnceAfterStartup: function (aLoadState) {
+    return SessionFileInternal.writeLoadStateOnceAfterStartup(aLoadState);
+  },
   /**
    * Create a backup copy, asynchronously.
    */
-  createBackupCopy: function SessionFile_createBackupCopy() {
-    return SessionFileInternal.createBackupCopy();
+  moveToBackupPath: function () {
+    return SessionFileInternal.moveToBackupPath();
   },
   /**
    * Create a backup copy, asynchronously.
    * This is designed to perform backup on upgrade.
    */
-  createUpgradeBackupCopy: function(ext) {
-    return SessionFileInternal.createUpgradeBackupCopy(ext);
+  createBackupCopy: function (ext) {
+    return SessionFileInternal.createBackupCopy(ext);
   },
   /**
    * Remove a backup copy, asynchronously.
    * This is designed to clean up a backup on upgrade.
    */
-  removeUpgradeBackup: function(ext) {
-    return SessionFileInternal.removeUpgradeBackup(ext);
+  removeBackupCopy: function (ext) {
+    return SessionFileInternal.removeBackupCopy(ext);
   },
   /**
    * Wipe the contents of the session file, asynchronously.
    */
-  wipe: function SessionFile_wipe() {
+  wipe: function () {
     return SessionFileInternal.wipe();
   }
 };
@@ -147,11 +144,6 @@ const TaskUtils = {
 };
 
 let SessionFileInternal = {
-  /**
-   * A promise fulfilled once initialization is complete
-   */
-  promiseInitialized: Promise.defer(),
-
   /**
    * The path to sessionstore.js
    */
@@ -168,7 +160,7 @@ let SessionFileInternal = {
    *        A path to read the file from.
    * @returns string if successful, undefined otherwise.
    */
-  readAuxSync: function ssfi_readAuxSync(aPath) {
+  readAuxSync: function (aPath) {
     let text;
     try {
       let file = new FileUtils.File(aPath);
@@ -197,7 +189,7 @@ let SessionFileInternal = {
    * happened between backup and write), attempt to read the sessionstore.bak
    * instead.
    */
-  syncRead: function ssfi_syncRead() {
+  syncRead: function () {
     // Start measuring the duration of the synchronous read.
     TelemetryStopwatch.start("FX_SESSION_RESTORE_SYNC_READ_FILE_MS");
     // First read the sessionstore.js.
@@ -208,83 +200,26 @@ let SessionFileInternal = {
     }
     // Finish the telemetry probe and return an empty string.
     TelemetryStopwatch.finish("FX_SESSION_RESTORE_SYNC_READ_FILE_MS");
-    return text || "";
+    text = text || "";
+
+    // The worker needs to know the initial state read from
+    // disk so that writeLoadStateOnceAfterStartup() works.
+    SessionWorker.post("setInitialState", [text]);
+    return text;
   },
 
-  /**
-   * Utility function to safely read a file asynchronously.
-   * @param aPath
-   *        A path to read the file from.
-   * @param aReadOptions
-   *        Read operation options.
-   *        |outExecutionDuration| option will be reused and can be
-   *        incrementally updated by the worker process.
-   * @returns string if successful, undefined otherwise.
-   */
-  readAux: function ssfi_readAux(aPath, aReadOptions) {
-    let self = this;
-    return TaskUtils.spawn(function () {
-      let text;
-      try {
-        let bytes = yield OS.File.read(aPath, undefined, aReadOptions);
-        text = gDecoder.decode(bytes);
-        // If the file is read successfully, add a telemetry probe based on
-        // the updated duration value of the |outExecutionDuration| option.
-        let histogram = Telemetry.getHistogramById(
-          "FX_SESSION_RESTORE_READ_FILE_MS");
-        histogram.add(aReadOptions.outExecutionDuration);
-      } catch (ex if self._isNoSuchFile(ex)) {
-        // Ignore exceptions about non-existent files.
-      } catch (ex) {
-        Cu.reportError(ex);
-      }
-      throw new Task.Result(text);
-    });
+  read: function () {
+    return SessionWorker.post("read").then(msg => msg.ok);
   },
 
-  /**
-   * Read the sessionstore file asynchronously.
-   *
-   * In case sessionstore.js file does not exist or is corrupted (something
-   * happened between backup and write), attempt to read the sessionstore.bak
-   * instead.
-   */
-  read: function ssfi_read() {
-    let self = this;
-    return TaskUtils.spawn(function task() {
-      // Specify |outExecutionDuration| option to hold the combined duration of
-      // the asynchronous reads off the main thread (of both sessionstore.js and
-      // sessionstore.bak, if necessary). If sessionstore.js does not exist or
-      // is corrupted, |outExecutionDuration| will register the time it took to
-      // attempt to read the file. It will then be subsequently incremented by
-      // the read time of sessionsore.bak.
-      let readOptions = {
-        outExecutionDuration: null
-      };
-      // First read the sessionstore.js.
-      let text = yield self.readAux(self.path, readOptions);
-      if (typeof text === "undefined") {
-        // If sessionstore.js does not exist or is corrupted, read the
-        // sessionstore.bak.
-        text = yield self.readAux(self.backupPath, readOptions);
-      }
-      // Return either the content of the sessionstore.bak if it was read
-      // successfully or an empty string otherwise.
-      throw new Task.Result(text || "");
-    });
-  },
-
-  write: function ssfi_write(aData) {
+  write: function (aData) {
     let refObj = {};
-    let self = this;
     return TaskUtils.spawn(function task() {
       TelemetryStopwatch.start("FX_SESSION_RESTORE_WRITE_FILE_MS", refObj);
       TelemetryStopwatch.start("FX_SESSION_RESTORE_WRITE_FILE_LONGEST_OP_MS", refObj);
 
-      let bytes = gEncoder.encode(aData);
-
       try {
-        let promise = OS.File.writeAtomic(self.path, bytes, {tmpPath: self.path + ".tmp"});
+        let promise = SessionWorker.post("write", [aData]);
         // At this point, we measure how long we stop the main thread
         TelemetryStopwatch.finish("FX_SESSION_RESTORE_WRITE_FILE_LONGEST_OP_MS", refObj);
 
@@ -294,93 +229,51 @@ let SessionFileInternal = {
       } catch (ex) {
         TelemetryStopwatch.cancel("FX_SESSION_RESTORE_WRITE_FILE_LONGEST_OP_MS", refObj);
         TelemetryStopwatch.cancel("FX_SESSION_RESTORE_WRITE_FILE_MS", refObj);
-        Cu.reportError("Could not write session state file " + self.path
-                       + ": " + aReason);
-      }
-    });
-  },
-
-  createBackupCopy: function ssfi_createBackupCopy() {
-    let backupCopyOptions = {
-      outExecutionDuration: null
-    };
-    let self = this;
-    return TaskUtils.spawn(function task() {
-      try {
-        yield OS.File.move(self.path, self.backupPath, backupCopyOptions);
-        Telemetry.getHistogramById("FX_SESSION_RESTORE_BACKUP_FILE_MS").add(
-          backupCopyOptions.outExecutionDuration);
-      } catch (ex if self._isNoSuchFile(ex)) {
-        // Ignore exceptions about non-existent files.
-      } catch (ex) {
-        Cu.reportError("Could not backup session state file: " + ex);
-        throw ex;
-      }
-    });
-  },
-
-  createUpgradeBackupCopy: function(ext) {
-    return TaskUtils.spawn(function task() {
-      try {
-        yield OS.File.copy(this.path, this.backupPath + ext);
-      } catch (ex if this._isNoSuchFile(ex)) {
-        // Ignore exceptions about non-existent files.
-      } catch (ex) {
-        Cu.reportError("Could not backup session state file to " +
-                       dest + ": " + ex);
-        throw ex;
+        Cu.reportError("Could not write session state file " + this.path
+                       + ": " + ex);
       }
     }.bind(this));
   },
 
-  removeUpgradeBackup: function(ext) {
-    return TaskUtils.spawn(function task() {
-      try {
-        yield OS.File.remove(this.backupPath + ext);
-      } catch (ex if this._isNoSuchFile(ex)) {
-        // Ignore exceptions about non-existent files.
-      }
-    }.bind(this));
+  writeLoadStateOnceAfterStartup: function (aLoadState) {
+    return SessionWorker.post("writeLoadStateOnceAfterStartup", [aLoadState]);
   },
 
-  wipe: function ssfi_wipe() {
-    let self = this;
-    return TaskUtils.spawn(function task() {
-      let exn;
-      // Erase session state file
-      try {
-        yield OS.File.remove(self.path);
-      } catch (ex if self._isNoSuchFile(ex)) {
-        // Ignore exceptions about non-existent files.
-      } catch (ex) {
-        // Report error, don't stop immediately
-        Cu.reportError("Could not remove session state file: " + ex);
-        exn = ex;
-      }
-
-      // Erase any backup, any file named "sessionstore.bak[-buildID]".
-      let iterator = new OS.File.DirectoryIterator(OS.Constants.Path.profileDir);
-      for (let promise of iterator) {
-        let entry = yield promise;
-        if (!entry.isDir && entry.path.startsWith(self.backupPath)) {
-          try {
-            yield OS.File.remove(entry.path);
-          } catch (ex) {
-            // Report error, don't stop immediately
-            Cu.reportError("Could not remove backup file " + entry.path + " : " + ex);
-            exn = exn || ex;
-          }
-        }
-      }
-
-      if (exn) {
-        throw exn;
-      }
-    });
-
+  moveToBackupPath: function () {
+    return SessionWorker.post("moveToBackupPath");
   },
 
-  _isNoSuchFile: function ssfi_isNoSuchFile(aReason) {
-    return aReason instanceof OS.File.Error && aReason.becauseNoSuchFile;
+  createBackupCopy: function (ext) {
+    return SessionWorker.post("createBackupCopy", [ext]);
+  },
+
+  removeBackupCopy: function (ext) {
+    return SessionWorker.post("removeBackupCopy", [ext]);
+  },
+
+  wipe: function () {
+    return SessionWorker.post("wipe");
   }
 };
+
+// Interface to a dedicated thread handling I/O
+let SessionWorker = (function () {
+  let worker = new PromiseWorker("resource:///modules/sessionstore/SessionWorker.js",
+    OS.Shared.LOG.bind("SessionWorker"));
+  return {
+    post: function post(...args) {
+      let promise = worker.post.apply(worker, args);
+      return promise.then(
+        null,
+        function onError(error) {
+          // Decode any serialized error
+          if (error instanceof PromiseWorker.WorkerError) {
+            throw OS.File.Error.fromMsg(error.data);
+          } else {
+            throw error;
+          }
+        }
+      );
+    }
+  };
+})();

browser/components/sessionstore/src/moz.build

@@ -16,6 +16,7 @@ EXTRA_JS_MODULES = [
     'DocumentUtils.jsm',
     'SessionMigration.jsm',
     'SessionStorage.jsm',
+    'SessionWorker.js',
     'XPathGenerator.jsm',
     '_SessionFile.jsm',
 ]

browser/components/sessionstore/test/browser_833286_atomic_backup.js

@@ -78,9 +78,9 @@ function testWriteNoBackup() {
   let array = yield OS.File.read(path);
   gSSData = gDecoder.decode(array);
 
-  // Manually trigger _SessionFile.createBackupCopy since the backup once
+  // Manually trigger _SessionFile.moveToBackupPath since the backup once
   // promise is already resolved and backup would not be triggered again.
-  yield _SessionFile.createBackupCopy();
+  yield _SessionFile.moveToBackupPath();
 
   nextTest(testWriteBackup);
 }
@@ -140,4 +140,4 @@ function testNoWriteBackup() {
   is(ssBakData, gSSBakData, "sessionstore.bak is unchanged.");
 
   executeSoon(finish);
-}
+}

browser/components/sessionstore/test/unit/test_backup.js

@@ -19,7 +19,7 @@ function pathBackup(ext) {
 
 // Ensure that things proceed smoothly if there is no file to back up
 add_task(function test_nothing_to_backup() {
-  yield _SessionFile.createUpgradeBackupCopy("");
+  yield _SessionFile.createBackupCopy("");
 });
 
 // Create a file, back it up, remove it
@@ -29,14 +29,14 @@ add_task(function test_do_backup() {
   yield OS.File.writeAtomic(pathStore, content, {tmpPath: pathStore + ".tmp"});
 
   do_print("Ensuring that the backup is created");
-  yield _SessionFile.createUpgradeBackupCopy(ext);
+  yield _SessionFile.createBackupCopy(ext);
   do_check_true((yield OS.File.exists(pathBackup(ext))));
 
   let data = yield OS.File.read(pathBackup(ext));
   do_check_eq((new TextDecoder()).decode(data), content);
 
   do_print("Ensuring that we can remove the backup");
-  yield _SessionFile.removeUpgradeBackup(ext);
+  yield _SessionFile.removeBackupCopy(ext);
   do_check_false((yield OS.File.exists(pathBackup(ext))));
 });