[PATCH 1/2] Bug 833286 - added improvements to atomic backup of sessionstore.js. r=yoric, ttaubert

---
 .../components/sessionstore/src/SessionStore.jsm   |   45 +++++--
 .../components/sessionstore/src/_SessionFile.jsm   |  131 ++++++++++++++-----
 2 files changed, 127 insertions(+), 49 deletions(-)

browser/components/sessionstore/src/SessionStore.jsm

@@ -459,13 +459,10 @@ let SessionStoreInternal = {
       catch (ex) { debug("The session file is invalid: " + ex); }
     }
 
-    if (this._resume_from_crash) {
+    // A Lazy getter for the sessionstore.js backup promise.
-      // Launch background copy of the session file. Note that we do
+    XPCOMUtils.defineLazyGetter(this, "_backupSessionFileOnce", function () {
-      // not have race conditions here as _SessionFile guarantees
+      return _SessionFile.createBackupCopy();
-      // that any I/O operation is completed before proceeding to
+    });
-      // the next I/O operation.
-      _SessionFile.createBackupCopy();
-    }
 
     // at this point, we've as good as resumed the session, so we can
     // clear the resume_session_once flag, if it's set
@@ -3758,13 +3755,33 @@ let SessionStoreInternal = {
       return;
     }
 
-    let self = this;
+    let promise;
-    _SessionFile.write(data).then(
+    // If "sessionstore.resume_from_crash" is true, attempt to backup the
-      function onSuccess() {
+    // session file first, before writing to it.
-        self._lastSaveTime = Date.now();
+    if (this._resume_from_crash) {
-        Services.obs.notifyObservers(null, "sessionstore-state-write-complete", "");
+      // Note that we do not have race conditions here as _SessionFile
+      // guarantees that any I/O operation is completed before proceeding to
+      // the next I/O operation.
+      // Note backup happens only once, on initial save.
+      promise = this._backupSessionFileOnce;
+    } else {
+      promise = Promise.resolve();
     }
-    );
+
+    // Attempt to write to the session file (potentially, depending on
+    // "sessionstore.resume_from_crash" preference, after successful backup).
+    promise = promise.then(function onSuccess() {
+      // Write (atomically) to a session file, using a tmp file.
+      return _SessionFile.write(data);
+    });
+
+    // Once the session file is successfully updated, save the time stamp of the
+    // last save and notify the observers.
+    promise = promise.then(() => {
+      this._lastSaveTime = Date.now();
+      Services.obs.notifyObservers(null, "sessionstore-state-write-complete",
+        "");
+    });
   },
 
   /* ........ Auxiliary Functions .............. */

browser/components/sessionstore/src/_SessionFile.jsm

@@ -49,6 +49,10 @@ XPCOMUtils.defineLazyServiceGetter(this, "Telemetry",
 XPCOMUtils.defineLazyGetter(this, "gEncoder", function () {
   return new TextEncoder();
 });
+// A decoder.
+XPCOMUtils.defineLazyGetter(this, "gDecoder", function () {
+  return new TextDecoder();
+});
 
 this._SessionFile = {
   /**
@@ -144,61 +148,118 @@ let SessionFileInternal = {
    */
   backupPath: OS.Path.join(OS.Constants.Path.profileDir, "sessionstore.bak"),
 
+  /**
+   * Utility function to safely read a file synchronously.
+   * @param aPath
+   *        A path to read the file from.
+   * @returns string if successful, undefined otherwise.
+   */
+  readAuxSync: function ssfi_readAuxSync(aPath) {
+    let text;
+    try {
+      let file = new FileUtils.File(aPath);
+      let chan = NetUtil.newChannel(file);
+      let stream = chan.open();
+      text = NetUtil.readInputStreamToString(stream, stream.available(),
+        {charset: "utf-8"});
+    } catch (e if e.result == Components.results.NS_ERROR_FILE_NOT_FOUND) {
+      // Ignore exceptions about non-existent files.
+    } catch (ex) {
+      // Any other error.
+      Cu.reportError(ex);
+    } finally {
+      return text;
+    }
+  },
+
   /**
    * Read the sessionstore file synchronously.
    *
    * This function is meant to serve as a fallback in case of race
    * between a synchronous usage of the API and asynchronous
    * initialization.
+   *
+   * In case if sessionstore.js file does not exist or is corrupted (something
+   * happened between backup and write), attempt to read the sessionstore.bak
+   * instead.
    */
   syncRead: function ssfi_syncRead() {
-    let text;
+    // Start measuring the duration of the synchronous read.
-    let exn;
     TelemetryStopwatch.start("FX_SESSION_RESTORE_SYNC_READ_FILE_MS");
-    try {
+    // First read the sessionstore.js.
-      let file = new FileUtils.File(this.path);
+    let text = this.readAuxSync(this.path);
-      let chan = NetUtil.newChannel(file);
+    if (typeof text === "undefined") {
-      let stream = chan.open();
+      // If sessionstore.js does not exist or is corrupted, read sessionstore.bak.
-      text = NetUtil.readInputStreamToString(stream, stream.available(), {charset: "utf-8"});
+      text = this.readAuxSync(this.backupPath);
-    } catch (e if e.result == Components.results.NS_ERROR_FILE_NOT_FOUND) {
+    }
-      return "";
+    // Finish the telemetry probe and return an empty string.
-    } catch(ex) {
-      exn = ex;
-    } finally {
     TelemetryStopwatch.finish("FX_SESSION_RESTORE_SYNC_READ_FILE_MS");
-    }
+    return text || "";
-    if (exn) {
-      Cu.reportError(exn);
-      return "";
-    }
-    return text;
   },
 
-  read: function ssfi_read() {
+  /**
-    let refObj = {};
+   * 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 task() {
+    return TaskUtils.spawn(function () {
-      TelemetryStopwatch.start("FX_SESSION_RESTORE_READ_FILE_MS", refObj);
       let text;
       try {
-        let bytes = yield OS.File.read(self.path);
+        let bytes = yield OS.File.read(aPath, undefined, aReadOptions);
-        text = new TextDecoder().decode(bytes);
+        text = gDecoder.decode(bytes);
-        TelemetryStopwatch.finish("FX_SESSION_RESTORE_READ_FILE_MS", refObj);
+        // 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) {
-        if (self._isNoSuchFile(ex)) {
-          // The file does not exist, this is perfectly valid
-          TelemetryStopwatch.finish("FX_SESSION_RESTORE_READ_FILE_MS", refObj);
-        } else {
-          // Any other error: let's not measure with telemetry
-          TelemetryStopwatch.cancel("FX_SESSION_RESTORE_READ_FILE_MS", refObj);
         Cu.reportError(ex);
       }
-        text = "";
-      }
       throw new Task.Result(text);
     });
   },
 
+  /**
+   * 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) {
     let refObj = {};
     let self = this;
@@ -232,7 +293,7 @@ let SessionFileInternal = {
     let self = this;
     return TaskUtils.spawn(function task() {
       try {
-        yield OS.File.copy(self.path, self.backupPath, backupCopyOptions);
+        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)) {