Bug 893009 - Move _backupSessionFileOnce() logic into the SessionWorker; r=ttaubert

X-Git-Commit-ID: cacdd9a34d6daf8302d8565a6e11d357c997bee7

browser/components/sessionstore/src/SessionStore.jsm

@@ -481,15 +481,6 @@ let SessionStoreInternal = {
       catch (ex) { debug("The session file is invalid: " + ex); }
     }
 
-    // A Lazy getter for the sessionstore.js backup promise.
-    XPCOMUtils.defineLazyGetter(this, "_backupSessionFileOnce", function () {
-      // 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
     // clear the resume_session_once flag, if it's set
     if (this._loadState != STATE_QUITTING &&
@@ -500,17 +491,6 @@ let SessionStoreInternal = {
 
     this._performUpgradeBackup();
 
-    // The service is ready. Backup-on-upgrade might still be in progress,
-    // but we do not have a race condition:
-    //
-    // - if the file to backup is named sessionstore.js, secondary
-    // backup will be started in this tick, so any further I/O will be
-    // scheduled to start after the secondary backup is complete;
-    //
-    // - if the file is named sessionstore.bak, it will only be erased
-    // by the getter to |_backupSessionFileOnce|, which specifically
-    // waits until the secondary backup has been completed or deemed
-    // useless before causing any side-effects.
     this._sessionInitialized = true;
     this._promiseInitialization.resolve();
   },
@@ -3904,25 +3884,9 @@ let SessionStoreInternal = {
       return;
     }
 
-    let promise;
-    // If "sessionstore.resume_from_crash" is true, attempt to backup the
-    // session file first, before writing to it.
-    if (this._resume_from_crash) {
-      // 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);
-    });
+    // Write (atomically) to a session file, using a tmp file.
+    let promise =
+      _SessionFile.write(data, {backupOnFirstWrite: this._resume_from_crash});
 
     // 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

@@ -57,6 +57,12 @@ let Agent = {
   // the loadState to disk once after startup.
   hasWrittenLoadStateOnce: false,
 
+  // Boolean that tells whether we already made a
+  // call to write(). We will only attempt to move
+  // sessionstore.js to sessionstore.bak on the
+  // first write.
+  hasWrittenState: false,
+
   // The path to sessionstore.js
   path: OS.Path.join(OS.Constants.Path.profileDir, "sessionstore.js"),
 
@@ -107,7 +113,19 @@ let Agent = {
   /**
    * Write the session to disk.
    */
-  write: function (stateString) {
+  write: function (stateString, options) {
+    if (!this.hasWrittenState) {
+      if (options && options.backupOnFirstWrite) {
+        try {
+          File.move(this.path, this.backupPath);
+        } catch (ex if isNoSuchFileEx(ex)) {
+          // Ignore exceptions about non-existent files.
+        }
+      }
+
+      this.hasWrittenState = true;
+    }
+
     let bytes = Encoder.encode(stateString);
     return File.writeAtomic(this.path, bytes, {tmpPath: this.path + ".tmp"});
   },
@@ -140,19 +158,8 @@ let Agent = {
 
     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;
-    }
+    let bytes = Encoder.encode(JSON.stringify(state));
+    return File.writeAtomic(this.path, bytes, {tmpPath: this.path + ".tmp"});
   },
 
   /**

browser/components/sessionstore/src/_SessionFile.jsm

@@ -67,8 +67,8 @@ this._SessionFile = {
   /**
    * Write the contents of the session file, asynchronously.
    */
-  write: function (aData) {
-    return SessionFileInternal.write(aData);
+  write: function (aData, aOptions = {}) {
+    return SessionFileInternal.write(aData, aOptions);
   },
   /**
    * Writes the initial state to disk again only to change the session's load
@@ -77,12 +77,6 @@ this._SessionFile = {
   writeLoadStateOnceAfterStartup: function (aLoadState) {
     return SessionFileInternal.writeLoadStateOnceAfterStartup(aLoadState);
   },
-  /**
-   * Create a backup copy, asynchronously.
-   */
-  moveToBackupPath: function () {
-    return SessionFileInternal.moveToBackupPath();
-  },
   /**
    * Create a backup copy, asynchronously.
    * This is designed to perform backup on upgrade.
@@ -212,14 +206,14 @@ let SessionFileInternal = {
     return SessionWorker.post("read").then(msg => msg.ok);
   },
 
-  write: function (aData) {
+  write: function (aData, aOptions) {
     let refObj = {};
     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);
 
       try {
-        let promise = SessionWorker.post("write", [aData]);
+        let promise = SessionWorker.post("write", [aData, aOptions]);
         // At this point, we measure how long we stop the main thread
         TelemetryStopwatch.finish("FX_SESSION_RESTORE_WRITE_FILE_LONGEST_OP_MS", refObj);
 
@@ -239,10 +233,6 @@ let SessionFileInternal = {
     return SessionWorker.post("writeLoadStateOnceAfterStartup", [aLoadState]);
   },
 
-  moveToBackupPath: function () {
-    return SessionWorker.post("moveToBackupPath");
-  },
-
   createBackupCopy: function (ext) {
     return SessionWorker.post("createBackupCopy", [ext]);
   },

browser/components/sessionstore/test/browser_833286_atomic_backup.js

@@ -2,6 +2,8 @@
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 // This tests are for a sessionstore.js atomic backup.
+// Each test will wait for a write to the Session Store
+// before executing.
 
 let tmp = {};
 Cu.import("resource://gre/modules/osfile.jsm", tmp);
@@ -23,7 +25,7 @@ let gDecoder = new TextDecoder();
 let gSSData;
 let gSSBakData;
 
-// waitForSaveStateComplete waits for a state write completion.
+// Wait for a state write to complete and then execute a callback.
 function waitForSaveStateComplete(aSaveStateCallback) {
   let topic = "sessionstore-state-write-complete";
 
@@ -41,6 +43,9 @@ function waitForSaveStateComplete(aSaveStateCallback) {
 // Register next test callback and trigger state saving change.
 function nextTest(testFunc) {
   waitForSaveStateComplete(testFunc);
+
+  // We set the interval for session store state saves to be zero
+  // to cause a save ASAP.
   Services.prefs.setIntPref(PREF_SS_INTERVAL, 0);
 }
 
@@ -53,21 +58,13 @@ registerCleanupFunction(function() {
 
 function test() {
   waitForExplicitFinish();
-  nextTest(testInitialWriteNoBackup);
+  nextTest(testAfterFirstWrite);
 }
 
-function testInitialWriteNoBackup() {
-  // Ensure that sessionstore.js is created, but not sessionstore.bak.
-  let ssExists = yield OS.File.exists(path);
-  let ssBackupExists = yield OS.File.exists(backupPath);
-  ok(ssExists, "sessionstore.js should be created.");
-  ok(!ssBackupExists, "sessionstore.bak should not have been created, yet.");
-
-  nextTest(testWriteNoBackup);
-}
-
-function testWriteNoBackup() {
-  // Ensure sessionstore.bak is not created.
+function testAfterFirstWrite() {
+  // Ensure sessionstore.bak is not created. We start with a clean
+  // profile so there was nothing to move to sessionstore.bak before
+  // initially writing sessionstore.js
   let ssExists = yield OS.File.exists(path);
   let ssBackupExists = yield OS.File.exists(backupPath);
   ok(ssExists, "sessionstore.js should exist.");
@@ -78,14 +75,14 @@ function testWriteNoBackup() {
   let array = yield OS.File.read(path);
   gSSData = gDecoder.decode(array);
 
-  // Manually trigger _SessionFile.moveToBackupPath since the backup once
-  // promise is already resolved and backup would not be triggered again.
-  yield _SessionFile.moveToBackupPath();
+  // Manually move to the backup since the first write has already happened
+  // and a backup would not be triggered again.
+  yield OS.File.move(path, backupPath);
 
-  nextTest(testWriteBackup);
+  nextTest(testReadBackup);
 }
 
-function testWriteBackup() {
+function testReadBackup() {
   // Ensure sessionstore.bak is finally created.
   let ssExists = yield OS.File.exists(path);
   let ssBackupExists = yield OS.File.exists(backupPath);
@@ -127,10 +124,11 @@ function testWriteBackup() {
   ssDataRead = _SessionFile.syncRead();
   is(ssDataRead, gSSBakData,
     "_SessionFile.syncRead read sessionstore.bak correctly.");
-  nextTest(testNoWriteBackup);
+
+  nextTest(testBackupUnchanged);
 }
 
-function testNoWriteBackup() {
+function testBackupUnchanged() {
   // Ensure sessionstore.bak is backed up only once.
 
   // Read sessionstore.bak data.

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

@@ -0,0 +1,48 @@
+/* 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);
+  decoder = new TextDecoder();
+  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;
+let decoder;
+
+// Write to the store, and check that a backup is created first
+add_task(function test_first_write_backup() {
+  let content = "test_1";
+  let initial_content = decoder.decode(yield OS.File.read(pathStore));
+
+  do_check_true(!(yield OS.File.exists(pathBackup)));
+  yield _SessionFile.write(content, {backupOnFirstWrite: true});
+  do_check_true(yield OS.File.exists(pathBackup));
+
+  let backup_content = decoder.decode(yield OS.File.read(pathBackup));
+  do_check_eq(initial_content, backup_content);
+});
+
+// Write to the store again, and check that the backup is not updated
+add_task(function test_second_write_no_backup() {
+  let content = "test_2";
+  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});
+
+  let written_content = decoder.decode(yield OS.File.read(pathStore));
+  do_check_eq(content, written_content);
+
+  let backup_content = decoder.decode(yield OS.File.read(pathBackup));
+  do_check_eq(initial_backup_content, backup_content);
+});

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

@@ -0,0 +1,32 @@
+/* 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

@@ -4,6 +4,8 @@ tail =
 firefox-appdir = browser
 
 [test_backup.js]
+[test_backup_once.js]
+[test_no_backup_first_write.js]
 [test_startup_nosession_sync.js]
 [test_startup_nosession_async.js]
 [test_startup_session_sync.js]