Bug 898775 - Fix the browser.sessionstore.resume_from_crash preference; r=yoric

browser/components/sessionstore/src/SessionStore.jsm

@@ -547,13 +547,6 @@ let SessionStoreInternal = {
       return this._prefBranch.getIntPref("sessionstore.interval");
     });
 
-    // when crash recovery is disabled, session data is not written to disk
-    XPCOMUtils.defineLazyGetter(this, "_resume_from_crash", function () {
-      // get crash recovery state from prefs and allow for proper reaction to state changes
-      this._prefBranch.addObserver("sessionstore.resume_from_crash", this, true);
-      return this._prefBranch.getBoolPref("sessionstore.resume_from_crash");
-    });
-
     XPCOMUtils.defineLazyGetter(this, "_max_tabs_undo", function () {
       this._prefBranch.addObserver("sessionstore.max_tabs_undo", this, true);
       return this._prefBranch.getIntPref("sessionstore.max_tabs_undo");
@@ -1191,14 +1184,6 @@ let SessionStoreInternal = {
         }
         this.saveStateDelayed(null, -1);
         break;
-      case "sessionstore.resume_from_crash":
-        this._resume_from_crash = this._prefBranch.getBoolPref("sessionstore.resume_from_crash");
-        // either create the file with crash recovery information or remove it
-        // (when _loadState is not STATE_RUNNING, that file is used for session resuming instead)
-        if (!this._resume_from_crash)
-          _SessionFile.wipe();
-        this.saveState(true);
-        break;
     }
   },
 
@@ -2547,11 +2532,9 @@ let SessionStoreInternal = {
    * gather session data as object
    * @param aUpdateAll
    *        Bool update all windows
-   * @param aPinnedOnly
-   *        Bool collect pinned tabs only
    * @returns object
    */
-  _getCurrentState: function ssi_getCurrentState(aUpdateAll, aPinnedOnly) {
+  _getCurrentState: function ssi_getCurrentState(aUpdateAll) {
     this._handleClosedWindows();
 
     var activeWindow = this._getMostRecentBrowserWindow();
@@ -2614,24 +2597,6 @@ let SessionStoreInternal = {
     }
 #endif
 
-    if (aPinnedOnly) {
-      // perform a deep copy so that existing session variables are not changed.
-      total = JSON.parse(this._toJSONString(total));
-      total = total.filter(function (win) {
-        win.tabs = win.tabs.filter(function (tab) tab.pinned);
-        // remove closed tabs
-        win._closedTabs = [];
-        // correct selected tab index if it was stripped out
-        if (win.selected > win.tabs.length)
-          win.selected = 1;
-        return win.tabs.length > 0;
-      });
-      if (total.length == 0)
-        return null;
-
-      lastClosedWindowsCopy = [];
-    }
-
     if (activeWindow) {
       this.activeWindowSSiCache = activeWindow.__SSi || "";
     }
@@ -3759,12 +3724,10 @@ let SessionStoreInternal = {
   saveState: function ssi_saveState(aUpdateAll) {
     // If crash recovery is disabled, we only want to resume with pinned tabs
     // if we crash.
-    let pinnedOnly = this._loadState == STATE_RUNNING && !this._resume_from_crash;
-
     TelemetryStopwatch.start("FX_SESSION_RESTORE_COLLECT_DATA_MS");
     TelemetryStopwatch.start("FX_SESSION_RESTORE_COLLECT_DATA_LONGEST_OP_MS");
 
-    var oState = this._getCurrentState(aUpdateAll, pinnedOnly);
+    var oState = this._getCurrentState(aUpdateAll);
     if (!oState) {
       TelemetryStopwatch.cancel("FX_SESSION_RESTORE_COLLECT_DATA_MS");
       TelemetryStopwatch.cancel("FX_SESSION_RESTORE_COLLECT_DATA_LONGEST_OP_MS");
@@ -3844,8 +3807,7 @@ let SessionStoreInternal = {
     }
 
     // Write (atomically) to a session file, using a tmp file.
-    let promise =
-      _SessionFile.write(data, {backupOnFirstWrite: this._resume_from_crash});
+    let promise = _SessionFile.write(data);
 
     // Once the session file is successfully updated, save the time stamp of the
     // last save and notify the observers.

browser/components/sessionstore/src/SessionWorker.js

@@ -124,23 +124,33 @@ let Agent = {
   /**
    * Write the session to disk.
    */
-  write: function (stateString, options) {
+  write: function (stateString) {
+    let exn;
     let telemetry = {};
+
     if (!this.hasWrittenState) {
-      if (options && options.backupOnFirstWrite) {
-        try {
-          let startMs = Date.now();
-          File.move(this.path, this.backupPath);
-          telemetry.FX_SESSION_RESTORE_BACKUP_FILE_MS = Date.now() - startMs;
-        } catch (ex if isNoSuchFileEx(ex)) {
-          // Ignore exceptions about non-existent files.
-        }
+      try {
+        let startMs = Date.now();
+        File.move(this.path, this.backupPath);
+        telemetry.FX_SESSION_RESTORE_BACKUP_FILE_MS = Date.now() - startMs;
+      } catch (ex if isNoSuchFileEx(ex)) {
+        // Ignore exceptions about non-existent files.
+      } catch (ex) {
+        // Throw the exception after we wrote the state to disk
+        // so that the backup can't interfere with the actual write.
+        exn = ex;
       }
 
       this.hasWrittenState = true;
     }
 
-    return this._write(stateString, telemetry);
+    let ret = this._write(stateString, telemetry);
+
+    if (exn) {
+      throw exn;
+    }
+
+    return ret;
   },
 
   /**

browser/components/sessionstore/src/_SessionFile.jsm

@@ -67,8 +67,8 @@ this._SessionFile = {
   /**
    * Write the contents of the session file, asynchronously.
    */
-  write: function (aData, aOptions = {}) {
-    return SessionFileInternal.write(aData, aOptions);
+  write: function (aData) {
+    return SessionFileInternal.write(aData);
   },
   /**
    * Writes the initial state to disk again only to change the session's load
@@ -209,13 +209,13 @@ let SessionFileInternal = {
     });
   },
 
-  write: function (aData, aOptions) {
+  write: function (aData) {
     let refObj = {};
     return TaskUtils.spawn(function task() {
       TelemetryStopwatch.start("FX_SESSION_RESTORE_WRITE_FILE_LONGEST_OP_MS", refObj);
 
       try {
-        let promise = SessionWorker.post("write", [aData, aOptions]);
+        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);
 

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

@@ -25,7 +25,7 @@ add_task(function test_first_write_backup() {
   let initial_content = decoder.decode(yield OS.File.read(pathStore));
 
   do_check_true(!(yield OS.File.exists(pathBackup)));
-  yield _SessionFile.write(content, {backupOnFirstWrite: true});
+  yield _SessionFile.write(content);
   do_check_true(yield OS.File.exists(pathBackup));
 
   let backup_content = decoder.decode(yield OS.File.read(pathBackup));
@@ -38,7 +38,7 @@ add_task(function test_second_write_no_backup() {
   let initial_content = decoder.decode(yield OS.File.read(pathStore));
   let initial_backup_content = decoder.decode(yield OS.File.read(pathBackup));
 
-  yield _SessionFile.write(content, {backupOnFirstWrite: true});
+  yield _SessionFile.write(content);
 
   let written_content = decoder.decode(yield OS.File.read(pathStore));
   do_check_eq(content, written_content);

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

@@ -1,32 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-let toplevel = this;
-Cu.import("resource://gre/modules/osfile.jsm");
-
-function run_test() {
-  let profd = do_get_profile();
-  Cu.import("resource:///modules/sessionstore/_SessionFile.jsm", toplevel);
-  pathStore = OS.Path.join(OS.Constants.Path.profileDir, "sessionstore.js");
-  pathBackup = OS.Path.join(OS.Constants.Path.profileDir, "sessionstore.bak");
-  let source = do_get_file("data/sessionstore_valid.js");
-  source.copyTo(profd, "sessionstore.js");
-  run_next_test();
-}
-
-let pathStore;
-let pathBackup;
-
-// Write to the store first with |backupOnFirstWrite: false|,
-// and make sure second write does not backup even with
-// |backupOnFirstWrite: true|
-add_task(function test_no_backup_on_second_write() {
-  let content = "test_1";
-
-  do_check_true(!(yield OS.File.exists(pathBackup)));
-  yield _SessionFile.write(content, {backupOnFirstWrite: false});
-  do_check_true(!(yield OS.File.exists(pathBackup)));
-
-  yield _SessionFile.write(content, {backupOnFirstWrite: true});
-  do_check_true(!(yield OS.File.exists(pathBackup)));
-});

browser/components/sessionstore/test/unit/xpcshell.ini

@@ -5,7 +5,6 @@ firefox-appdir = browser
 
 [test_backup.js]
 [test_backup_once.js]
-[test_no_backup_first_write.js]
 [test_startup_nosession_sync.js]
 # bug 845190 - thread pool wasn't shutdown assertions
 skip-if = (os == "win" || "linux") && debug