From f9096d13e0308da49d477ef4a8508bbe8e9f52f8 Mon Sep 17 00:00:00 2001 From: Steven MacLeod Date: Fri, 26 Jul 2013 19:54:59 +0200 Subject: [PATCH] Bug 893009 - Move _backupSessionFileOnce() logic into the SessionWorker; r=ttaubert X-Git-Commit-ID: cacdd9a34d6daf8302d8565a6e11d357c997bee7 --- .../sessionstore/src/SessionStore.jsm | 42 ++-------------- .../sessionstore/src/SessionWorker.js | 35 ++++++++------ .../sessionstore/src/_SessionFile.jsm | 18 ++----- .../test/browser_833286_atomic_backup.js | 40 ++++++++-------- .../test/unit/test_backup_once.js | 48 +++++++++++++++++++ .../test/unit/test_no_backup_first_write.js | 32 +++++++++++++ .../sessionstore/test/unit/xpcshell.ini | 2 + 7 files changed, 129 insertions(+), 88 deletions(-) create mode 100644 browser/components/sessionstore/test/unit/test_backup_once.js create mode 100644 browser/components/sessionstore/test/unit/test_no_backup_first_write.js diff --git a/browser/components/sessionstore/src/SessionStore.jsm b/browser/components/sessionstore/src/SessionStore.jsm index 202f5dc97f05..e128aac5297f 100644 --- a/browser/components/sessionstore/src/SessionStore.jsm +++ b/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. diff --git a/browser/components/sessionstore/src/SessionWorker.js b/browser/components/sessionstore/src/SessionWorker.js index a4d17813ce9d..6fd765e778f8 100644 --- a/browser/components/sessionstore/src/SessionWorker.js +++ b/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"}); }, /** diff --git a/browser/components/sessionstore/src/_SessionFile.jsm b/browser/components/sessionstore/src/_SessionFile.jsm index e96784b10316..c6dbd5b80a56 100644 --- a/browser/components/sessionstore/src/_SessionFile.jsm +++ b/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]); }, diff --git a/browser/components/sessionstore/test/browser_833286_atomic_backup.js b/browser/components/sessionstore/test/browser_833286_atomic_backup.js index a04a6ed8ce5c..925368c124c5 100644 --- a/browser/components/sessionstore/test/browser_833286_atomic_backup.js +++ b/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. diff --git a/browser/components/sessionstore/test/unit/test_backup_once.js b/browser/components/sessionstore/test/unit/test_backup_once.js new file mode 100644 index 000000000000..26ba93b2a4f1 --- /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); +}); diff --git a/browser/components/sessionstore/test/unit/test_no_backup_first_write.js b/browser/components/sessionstore/test/unit/test_no_backup_first_write.js new file mode 100644 index 000000000000..d3e635b1884b --- /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))); +}); diff --git a/browser/components/sessionstore/test/unit/xpcshell.ini b/browser/components/sessionstore/test/unit/xpcshell.ini index 009c50bc58a7..a11a4e6324e6 100644 --- a/browser/components/sessionstore/test/unit/xpcshell.ini +++ b/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]