Bug 893009 - Move _backupSessionFileOnce() logic into the SessionWorker; r=ttaubert
authorSteven MacLeod <smacleod@mozilla.com>
Fri, 26 Jul 2013 19:54:59 +0200
changeset 140249 6fba91b781eeb3e9b4a72f7d0ac4a161580fbbf1
parent 140248 6a62ce6a092bd57a28f0139b4b7cf5bd7863fb27
child 140250 dbaacbf32f6c7eac6a83f079640990fe04df5578
push id25021
push userryanvm@gmail.com
push dateSun, 28 Jul 2013 01:51:56 +0000
treeherdermozilla-central@015135250e06 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersttaubert
bugs893009
milestone25.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 893009 - Move _backupSessionFileOnce() logic into the SessionWorker; r=ttaubert X-Git-Commit-ID: cacdd9a34d6daf8302d8565a6e11d357c997bee7
browser/components/sessionstore/src/SessionStore.jsm
browser/components/sessionstore/src/SessionWorker.js
browser/components/sessionstore/src/_SessionFile.jsm
browser/components/sessionstore/test/browser_833286_atomic_backup.js
browser/components/sessionstore/test/unit/test_backup_once.js
browser/components/sessionstore/test/unit/test_no_backup_first_write.js
browser/components/sessionstore/test/unit/xpcshell.ini
--- a/browser/components/sessionstore/src/SessionStore.jsm
+++ b/browser/components/sessionstore/src/SessionStore.jsm
@@ -476,46 +476,26 @@ let SessionStoreInternal = {
           this._initialState.windows.forEach(function(aWindow) {
             delete aWindow.__lastSessionWindowID;
           });
         }
       }
       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 &&
         this._prefBranch.getBoolPref("sessionstore.resume_session_once"))
       this._prefBranch.setBoolPref("sessionstore.resume_session_once", false);
 
     this._initEncoding();
 
     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();
   },
 
   /**
    * If this is the first time we launc this build of Firefox,
    * backup sessionstore.js.
    */
@@ -3899,35 +3879,19 @@ let SessionStoreInternal = {
     Services.obs.notifyObservers(stateString, "sessionstore-state-write", "");
     data = stateString.data;
 
     // Don't touch the file if an observer has deleted all state data.
     if (!data) {
       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.
     promise = promise.then(() => {
       this._lastSaveTime = Date.now();
       Services.obs.notifyObservers(null, "sessionstore-state-write-complete",
         "");
     });
--- a/browser/components/sessionstore/src/SessionWorker.js
+++ b/browser/components/sessionstore/src/SessionWorker.js
@@ -52,16 +52,22 @@ self.onmessage = function (msg) {
 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,
 
+  // 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"),
 
   // 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
@@ -102,17 +108,29 @@ let Agent = {
 
     // No sessionstore data files found. Return an empty string.
     return "";
   },
 
   /**
    * 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"});
   },
 
   /**
    * 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.
@@ -135,29 +153,18 @@ let Agent = {
     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;
-    }
+    let bytes = Encoder.encode(JSON.stringify(state));
+    return File.writeAtomic(this.path, bytes, {tmpPath: this.path + ".tmp"});
   },
 
   /**
    * Creates a copy of sessionstore.js.
    */
   createBackupCopy: function (ext) {
     try {
       return File.copy(this.path, this.backupPath + ext);
--- a/browser/components/sessionstore/src/_SessionFile.jsm
+++ b/browser/components/sessionstore/src/_SessionFile.jsm
@@ -62,34 +62,28 @@ this._SessionFile = {
     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 (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
    * 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.
-   */
-  moveToBackupPath: function () {
-    return SessionFileInternal.moveToBackupPath();
-  },
-  /**
-   * Create a backup copy, asynchronously.
    * This is designed to perform backup on upgrade.
    */
   createBackupCopy: function (ext) {
     return SessionFileInternal.createBackupCopy(ext);
   },
   /**
    * Remove a backup copy, asynchronously.
    * This is designed to clean up a backup on upgrade.
@@ -207,24 +201,24 @@ let SessionFileInternal = {
     SessionWorker.post("setInitialState", [text]);
     return text;
   },
 
   read: function () {
     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);
 
         // Now wait for the result and measure how long we had to wait for the result
         yield promise;
         TelemetryStopwatch.finish("FX_SESSION_RESTORE_WRITE_FILE_MS", refObj);
       } catch (ex) {
         TelemetryStopwatch.cancel("FX_SESSION_RESTORE_WRITE_FILE_LONGEST_OP_MS", refObj);
@@ -234,20 +228,16 @@ let SessionFileInternal = {
       }
     }.bind(this));
   },
 
   writeLoadStateOnceAfterStartup: function (aLoadState) {
     return SessionWorker.post("writeLoadStateOnceAfterStartup", [aLoadState]);
   },
 
-  moveToBackupPath: function () {
-    return SessionWorker.post("moveToBackupPath");
-  },
-
   createBackupCopy: function (ext) {
     return SessionWorker.post("createBackupCopy", [ext]);
   },
 
   removeBackupCopy: function (ext) {
     return SessionWorker.post("removeBackupCopy", [ext]);
   },
 
--- a/browser/components/sessionstore/test/browser_833286_atomic_backup.js
+++ b/browser/components/sessionstore/test/browser_833286_atomic_backup.js
@@ -1,12 +1,14 @@
 /* Any copyright is dedicated to the Public Domain.
    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);
 Cu.import("resource://gre/modules/Task.jsm", tmp);
 Cu.import("resource:///modules/sessionstore/_SessionFile.jsm", tmp);
 
 const {OS, Task, _SessionFile} = tmp;
 
@@ -18,17 +20,17 @@ const backupPath = OS.Path.join(OS.Const
 
 // A text decoder.
 let gDecoder = new TextDecoder();
 // Global variables that contain sessionstore.js and sessionstore.bak data for
 // comparison between tests.
 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";
 
   function observer() {
     Services.prefs.clearUserPref(PREF_SS_INTERVAL);
     Services.obs.removeObserver(observer, topic);
     executeSoon(function taskCallback() {
       Task.spawn(aSaveStateCallback);
@@ -36,61 +38,56 @@ function waitForSaveStateComplete(aSaveS
   }
 
   Services.obs.addObserver(observer, topic, false);
 }
 
 // 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);
 }
 
 registerCleanupFunction(function() {
   // Cleaning up after the test: removing the sessionstore.bak file.
   Task.spawn(function cleanupTask() {
     yield OS.File.remove(backupPath);
   });
 });
 
 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.");
   ok(!ssBackupExists, "sessionstore.bak should not have been created, yet");
 
   // Save sessionstore.js data to compare to the sessionstore.bak data in the
   // next test.
   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);
   ok(ssExists, "sessionstore.js exists.");
   ok(ssBackupExists, "sessionstore.bak should now be created.");
 
   // Read sessionstore.bak data.
   let array = yield OS.File.read(backupPath);
@@ -122,20 +119,21 @@ function testWriteBackup() {
   ssDataRead = yield _SessionFile.read();
   is(ssDataRead, gSSBakData,
     "_SessionFile.read read sessionstore.bak correctly.");
 
   // Read sessionstore.bak with _SessionFile.syncRead.
   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.
   let array = yield OS.File.read(backupPath);
   let ssBakData = gDecoder.decode(array);
   // Ensure the sessionstore.bak did not change.
   is(ssBakData, gSSBakData, "sessionstore.bak is unchanged.");
 
new file mode 100644
--- /dev/null
+++ b/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);
+});
new file mode 100644
--- /dev/null
+++ b/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)));
+});
--- a/browser/components/sessionstore/test/unit/xpcshell.ini
+++ b/browser/components/sessionstore/test/unit/xpcshell.ini
@@ -1,10 +1,12 @@
 [DEFAULT]
 head = head.js
 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]
 [test_startup_session_async.js]