Bug 1427007 - Part 2: the SessionFile unit tests dependended on a very specific interaction between wipe() and read(), which changed in part 1. This fixes the tests up by making the dependency a bit more clear and explicit. r=Yoric a=ritu DEVEDITION_59_0b10_BUILD3 DEVEDITION_59_0b10_RELEASE FIREFOX_59_0b10_BUILD3 FIREFOX_59_0b10_RELEASE
authorMike de Boer <mdeboer@mozilla.com>
Fri, 02 Feb 2018 17:34:32 +0100
changeset 452428 b89f4d21c530d5ec7d2903350745843e1215b5a6
parent 452427 aeb7a1482e4d9ecd0e4c40604c167e2dd287e19b
child 452429 fd5298eb075bfbaddbed4300ed4a38cc2eb9198c
push id8711
push userarchaeopteryx@coole-files.de
push dateThu, 15 Feb 2018 11:14:55 +0000
treeherdermozilla-beta@b89f4d21c530 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersYoric, ritu
bugs1427007
milestone59.0
Bug 1427007 - Part 2: the SessionFile unit tests dependended on a very specific interaction between wipe() and read(), which changed in part 1. This fixes the tests up by making the dependency a bit more clear and explicit. r=Yoric a=ritu MozReview-Commit-ID: uoPx46cNL1
browser/components/sessionstore/SessionFile.jsm
browser/components/sessionstore/test/browser_backup_recovery.js
browser/components/sessionstore/test/browser_upgrade_backup.js
browser/components/sessionstore/test/unit/test_shutdown_cleanup.js
--- a/browser/components/sessionstore/SessionFile.jsm
+++ b/browser/components/sessionstore/SessionFile.jsm
@@ -446,17 +446,21 @@ var SessionFileInternal = {
         Services.obs.notifyObservers(null, "sessionstore-final-state-write-complete");
       } else {
         this._checkWorkerHealth();
       }
     });
   },
 
   wipe() {
-    return this._postToWorker("wipe");
+    return this._postToWorker("wipe").then(() => {
+      // After a wipe, we need to make sure to re-initialize upon the next read(),
+      // because the state variables as sent to the worker have changed.
+      this._initializationStarted = false;
+    });
   },
 
   _recordTelemetry(telemetry) {
     for (let id of Object.keys(telemetry)) {
       let value = telemetry[id];
       let samples = [];
       if (Array.isArray(value)) {
         samples.push(...value);
--- a/browser/components/sessionstore/test/browser_backup_recovery.js
+++ b/browser/components/sessionstore/test/browser_backup_recovery.js
@@ -17,16 +17,21 @@ var gDecoder = new TextDecoder();
 // comparison between tests.
 var gSSData;
 var gSSBakData;
 
 function promiseRead(path) {
   return File.read(path, {encoding: "utf-8", compression: "lz4"});
 }
 
+async function reInitSessionFile() {
+  await SessionFile.wipe();
+  await SessionFile.read();
+}
+
 add_task(async function init() {
   // Make sure that we are not racing with SessionSaver's time based
   // saves.
   Services.prefs.setIntPref(PREF_SS_INTERVAL, 10000000);
   registerCleanupFunction(() => Services.prefs.clearUserPref(PREF_SS_INTERVAL));
 });
 
 add_task(async function test_creation() {
@@ -35,18 +40,17 @@ add_task(async function test_creation() 
 
   // Create dummy sessionstore backups
   let OLD_BACKUP = Path.join(Constants.Path.profileDir, "sessionstore.baklz4");
   let OLD_UPGRADE_BACKUP = Path.join(Constants.Path.profileDir, "sessionstore.baklz4-0000000");
 
   await File.writeAtomic(OLD_BACKUP, "sessionstore.bak");
   await File.writeAtomic(OLD_UPGRADE_BACKUP, "sessionstore upgrade backup");
 
-  await SessionFile.wipe();
-  await SessionFile.read(); // Reinitializes SessionFile
+  await reInitSessionFile();
 
   // Ensure none of the sessionstore files and backups exists
   for (let k of Paths.loadOrder) {
     ok(!(await File.exists(Paths[k])), "After wipe " + k + " sessionstore file doesn't exist");
   }
   ok(!(await File.exists(OLD_BACKUP)), "After wipe, old backup doesn't exist");
   ok(!(await File.exists(OLD_UPGRADE_BACKUP)), "After wipe, old upgrade backup doesn't exist");
 
@@ -58,43 +62,47 @@ add_task(async function test_creation() 
   info("Testing situation after a single write");
   await promiseBrowserLoaded(tab.linkedBrowser);
   await TabStateFlusher.flush(tab.linkedBrowser);
   await SessionSaver.run();
 
   ok((await File.exists(Paths.recovery)), "After write, recovery sessionstore file exists again");
   ok(!(await File.exists(Paths.recoveryBackup)), "After write, recoveryBackup sessionstore doesn't exist");
   ok((await promiseRead(Paths.recovery)).indexOf(URL) != -1, "Recovery sessionstore file contains the required tab");
-  ok(!(await File.exists(Paths.clean)), "After first write, clean shutdown sessionstore doesn't exist, since we haven't shutdown yet");
+  ok(!(await File.exists(Paths.clean)), "After first write, clean shutdown " +
+    "sessionstore doesn't exist, since we haven't shutdown yet");
 
   // Open a second tab, save session, ensure that the correct files exist.
   info("Testing situation after a second write");
   let URL2 = URL_BASE + "?second_write";
   tab.linkedBrowser.loadURI(URL2);
   await promiseBrowserLoaded(tab.linkedBrowser);
   await TabStateFlusher.flush(tab.linkedBrowser);
   await SessionSaver.run();
 
   ok((await File.exists(Paths.recovery)), "After second write, recovery sessionstore file still exists");
   ok((await promiseRead(Paths.recovery)).indexOf(URL2) != -1, "Recovery sessionstore file contains the latest url");
   ok((await File.exists(Paths.recoveryBackup)), "After write, recoveryBackup sessionstore now exists");
   let backup = await promiseRead(Paths.recoveryBackup);
   ok(backup.indexOf(URL2) == -1, "Recovery backup doesn't contain the latest url");
   ok(backup.indexOf(URL) != -1, "Recovery backup contains the original url");
-  ok(!(await File.exists(Paths.clean)), "After first write, clean shutdown sessinstore doesn't exist, since we haven't shutdown yet");
+  ok(!(await File.exists(Paths.clean)), "After first write, clean shutdown " +
+    "sessionstore doesn't exist, since we haven't shutdown yet");
 
   info("Reinitialize, ensure that we haven't leaked sensitive files");
   await SessionFile.read(); // Reinitializes SessionFile
   await SessionSaver.run();
-  ok(!(await File.exists(Paths.clean)), "After second write, clean shutdown sessonstore doesn't exist, since we haven't shutdown yet");
-  ok(!(await File.exists(Paths.upgradeBackup)), "After second write, clean shutdwn sessionstore doesn't exist, since we haven't shutdown yet");
-  ok(!(await File.exists(Paths.nextUpgradeBackup)), "After second write, clean sutdown sessionstore doesn't exist, since we haven't shutdown yet");
+  ok(!(await File.exists(Paths.clean)), "After second write, clean shutdown " +
+    "sessionstore doesn't exist, since we haven't shutdown yet");
+  ok(!(await File.exists(Paths.upgradeBackup)), "After second write, clean " +
+    "shutdown sessionstore doesn't exist, since we haven't shutdown yet");
+  ok(!(await File.exists(Paths.nextUpgradeBackup)), "After second write, clean " +
+    "shutdown sessionstore doesn't exist, since we haven't shutdown yet");
 
   gBrowser.removeTab(tab);
-  await SessionFile.wipe();
 });
 
 var promiseSource = async function(name) {
   let URL = "http://example.com/?atomic_backup_test_recovery=" + Math.random() + "&name=" + name;
   let tab = BrowserTestUtils.addTab(gBrowser, URL);
 
   await promiseBrowserLoaded(tab.linkedBrowser);
   await TabStateFlusher.flush(tab.linkedBrowser);
@@ -102,63 +110,62 @@ var promiseSource = async function(name)
   gBrowser.removeTab(tab);
 
   let SOURCE = await promiseRead(Paths.recovery);
   await SessionFile.wipe();
   return SOURCE;
 };
 
 add_task(async function test_recovery() {
-  // Remove all files.
-  await SessionFile.wipe();
+  await reInitSessionFile();
   info("Attempting to recover from the recovery file");
 
   // Create Paths.recovery, ensure that we can recover from it.
   let SOURCE = await promiseSource("Paths.recovery");
   await File.makeDir(Paths.backups);
   await File.writeAtomic(Paths.recovery, SOURCE, {encoding: "utf-8", compression: "lz4"});
   is((await SessionFile.read()).source, SOURCE, "Recovered the correct source from the recovery file");
-  await SessionFile.wipe();
 
   info("Corrupting recovery file, attempting to recover from recovery backup");
   SOURCE = await promiseSource("Paths.recoveryBackup");
   await File.makeDir(Paths.backups);
   await File.writeAtomic(Paths.recoveryBackup, SOURCE, {encoding: "utf-8", compression: "lz4"});
   await File.writeAtomic(Paths.recovery, "<Invalid JSON>", {encoding: "utf-8", compression: "lz4"});
   is((await SessionFile.read()).source, SOURCE, "Recovered the correct source from the recovery file");
-  await SessionFile.wipe();
 });
 
 add_task(async function test_recovery_inaccessible() {
   // Can't do chmod() on non-UNIX platforms, we need that for this test.
   if (AppConstants.platform != "macosx" && AppConstants.platform != "linux") {
     return;
   }
 
+  await reInitSessionFile();
   info("Making recovery file inaccessible, attempting to recover from recovery backup");
   let SOURCE_RECOVERY = await promiseSource("Paths.recovery");
   let SOURCE = await promiseSource("Paths.recoveryBackup");
   await File.makeDir(Paths.backups);
   await File.writeAtomic(Paths.recoveryBackup, SOURCE, {encoding: "utf-8", compression: "lz4"});
 
   // Write a valid recovery file but make it inaccessible.
   await File.writeAtomic(Paths.recovery, SOURCE_RECOVERY, {encoding: "utf-8", compression: "lz4"});
   await File.setPermissions(Paths.recovery, { unixMode: 0 });
 
   is((await SessionFile.read()).source, SOURCE, "Recovered the correct source from the recovery file");
   await File.setPermissions(Paths.recovery, { unixMode: 0o644 });
 });
 
 add_task(async function test_clean() {
-  await SessionFile.wipe();
+  await reInitSessionFile();
   let SOURCE = await promiseSource("Paths.clean");
   await File.writeAtomic(Paths.clean, SOURCE, {encoding: "utf-8", compression: "lz4"});
   await SessionFile.read();
   await SessionSaver.run();
-  is((await promiseRead(Paths.cleanBackup)), SOURCE, "After first read/write, clean shutdown file has been moved to cleanBackup");
+  is((await promiseRead(Paths.cleanBackup)), SOURCE, "After first read/write, " +
+    "clean shutdown file has been moved to cleanBackup");
 });
 
 
 /**
  * Tests loading of sessionstore when format version is known.
  */
 add_task(async function test_version() {
   info("Preparing sessionstore");
@@ -175,16 +182,17 @@ add_task(async function test_version() {
   // Ensure that we can recover from Paths.recovery
   is((await SessionFile.read()).source, SOURCE, "Recovered the correct source from the clean file");
 });
 
 /**
  * Tests fallback to previous backups if format version is unknown.
  */
 add_task(async function test_version_fallback() {
+  await reInitSessionFile();
   info("Preparing data, making sure that it has a version number");
   let SOURCE = await promiseSource("Paths.clean");
   let BACKUP_SOURCE = await promiseSource("Paths.cleanBackup");
 
   is(JSON.parse(SOURCE).version[0], "sessionrestore", "Found sessionstore format version");
   is(JSON.parse(BACKUP_SOURCE).version[0], "sessionrestore", "Found backup sessionstore format version");
 
   await File.makeDir(Paths.backups);
@@ -200,10 +208,10 @@ add_task(async function test_version_fal
   parsedSource = JSON.parse(SOURCE);
   parsedSource.version[1] = Number.MAX_SAFE_INTEGER;
   await File.writeAtomic(Paths.clean, JSON.stringify(parsedSource), {encoding: "utf-8", compression: "lz4"});
   await File.writeAtomic(Paths.cleanBackup, BACKUP_SOURCE, {encoding: "utf-8", compression: "lz4"});
   is((await SessionFile.read()).source, BACKUP_SOURCE, "Recovered the correct source from the backup recovery file");
 });
 
 add_task(async function cleanup() {
-  await SessionFile.wipe();
+  await reInitSessionFile();
 });
--- a/browser/components/sessionstore/test/browser_upgrade_backup.js
+++ b/browser/components/sessionstore/test/browser_upgrade_backup.js
@@ -9,30 +9,30 @@ const Paths = SessionFile.Paths;
 const PREF_UPGRADE = "browser.sessionstore.upgradeBackup.latestBuildID";
 const PREF_MAX_UPGRADE_BACKUPS = "browser.sessionstore.upgradeBackup.maxUpgradeBackups";
 
 /**
  * Prepares tests by retrieving the current platform's build ID, clearing the
  * build where the last backup was created and creating arbitrary JSON data
  * for a new backup.
  */
-var prepareTest = async function() {
+function prepareTest() {
   let result = {};
 
   result.buildID = Services.appinfo.platformBuildID;
   Services.prefs.setCharPref(PREF_UPGRADE, "");
   result.contents = JSON.stringify({"browser_upgrade_backup.js": Math.random()});
 
   return result;
-};
+}
 
 /**
  * Retrieves all upgrade backups and returns them in an array.
  */
-var getUpgradeBackups = async function() {
+async function getUpgradeBackups() {
   let iterator;
   let backups = [];
 
   try {
     iterator = new OS.File.DirectoryIterator(Paths.backups);
 
     // iterate over all files in the backup directory
     await iterator.forEach(function(file) {
@@ -45,27 +45,27 @@ var getUpgradeBackups = async function()
   } finally {
     if (iterator) {
       iterator.close();
     }
   }
 
   // return results
   return backups;
-};
+}
 
 add_task(async function init() {
   // Wait until initialization is complete
   await SessionStore.promiseInitialized;
-  await SessionFile.wipe();
 });
 
 add_task(async function test_upgrade_backup() {
-  let test = await prepareTest();
+  let test = prepareTest();
   info("Let's check if we create an upgrade backup");
+  await SessionFile.wipe();
   await OS.File.writeAtomic(Paths.clean, test.contents, {encoding: "utf-8", compression: "lz4"});
   await SessionFile.read(); // First call to read() initializes the SessionWorker
   await SessionFile.write(""); // First call to write() triggers the backup
 
   is(Services.prefs.getCharPref(PREF_UPGRADE), test.buildID, "upgrade backup should be set");
 
   is((await OS.File.exists(Paths.upgradeBackup)), true, "upgrade backup file has been created");
 
@@ -77,19 +77,21 @@ add_task(async function test_upgrade_bac
   await OS.File.writeAtomic(Paths.clean, newContents, {encoding: "utf-8", compression: "lz4"});
   await SessionFile.read(); // Reinitialize the SessionWorker
   await SessionFile.write(""); // Next call to write() shouldn't trigger the backup
   data = await OS.File.read(Paths.upgradeBackup, {compression: "lz4"});
   is(test.contents, (new TextDecoder()).decode(data), "upgrade backup hasn't changed");
 });
 
 add_task(async function test_upgrade_backup_removal() {
-  let test = await prepareTest();
+  let test = prepareTest();
   let maxUpgradeBackups = Preferences.get(PREF_MAX_UPGRADE_BACKUPS, 3);
   info("Let's see if we remove backups if there are too many");
+  await SessionFile.wipe();
+  await OS.File.makeDir(Paths.backups);
   await OS.File.writeAtomic(Paths.clean, test.contents, {encoding: "utf-8", compression: "lz4"});
 
   // if the nextUpgradeBackup already exists (from another test), remove it
   if (OS.File.exists(Paths.nextUpgradeBackup)) {
     await OS.File.remove(Paths.nextUpgradeBackup);
   }
 
   // create dummy backups
--- a/browser/components/sessionstore/test/unit/test_shutdown_cleanup.js
+++ b/browser/components/sessionstore/test/unit/test_shutdown_cleanup.js
@@ -23,23 +23,36 @@ const URL = "http://example.com/#";
 Cu.import("resource://testing-common/AppInfo.jsm", this);
 updateAppInfo({
   name: "SessionRestoreTest",
   ID: "{230de50e-4cd1-11dc-8314-0800200c9a66}",
   version: "1",
   platformVersion: "",
 });
 
-add_task(async function setup() {
-  let source = do_get_file("data/sessionstore_valid.js");
-  source.copyTo(profd, "sessionstore.js");
+var gSourceHandle;
+
+async function prepareWithLimit(back, fwd) {
+  await SessionFile.wipe();
+
+  if (!gSourceHandle) {
+    gSourceHandle = do_get_file("data/sessionstore_valid.js");
+  }
+  gSourceHandle.copyTo(profd, "sessionstore.js");
   await writeCompressedFile(Paths.clean.replace("jsonlz4", "js"), Paths.clean);
 
+  Services.prefs.setIntPref("browser.sessionstore.max_serialize_back", back);
+  Services.prefs.setIntPref("browser.sessionstore.max_serialize_forward", fwd);
+
   // Finish SessionFile initialization.
   await SessionFile.read();
+}
+
+add_task(async function setup() {
+  await SessionFile.read();
 
   // Reset prefs on cleanup.
   registerCleanupFunction(() => {
     Services.prefs.clearUserPref("browser.sessionstore.max_serialize_back");
     Services.prefs.clearUserPref("browser.sessionstore.max_serialize_forward");
   });
 });
 
@@ -49,78 +62,77 @@ function createSessionState(index) {
   let tabState = {entries: [], index};
   for (let i = 0; i < MAX_ENTRIES; i++) {
     tabState.entries.push({url: URL + i});
   }
 
   return {windows: [{tabs: [tabState]}]};
 }
 
-async function setMaxBackForward(back, fwd) {
-  Services.prefs.setIntPref("browser.sessionstore.max_serialize_back", back);
-  Services.prefs.setIntPref("browser.sessionstore.max_serialize_forward", fwd);
-  await SessionFile.read();
-}
-
 async function writeAndParse(state, path, options = {}) {
   await SessionWorker.post("write", [state, options]);
   return JSON.parse(await File.read(path, {encoding: "utf-8", compression: "lz4"}));
 }
 
 add_task(async function test_shistory_cap_none() {
   let state = createSessionState(5);
 
   // Don't limit the number of shistory entries.
-  await setMaxBackForward(-1, -1);
+  await prepareWithLimit(-1, -1);
 
   // Check that no caps are applied.
   let diskState = await writeAndParse(state, Paths.clean, {isFinalWrite: true});
   Assert.deepEqual(state, diskState, "no cap applied");
 });
 
 add_task(async function test_shistory_cap_middle() {
   let state = createSessionState(5);
-  await setMaxBackForward(2, 3);
+  await prepareWithLimit(2, 3);
 
   // Cap is only applied on clean shutdown.
   let diskState = await writeAndParse(state, Paths.recovery);
   Assert.deepEqual(state, diskState, "no cap applied");
 
   // Check that the right number of shistory entries was discarded
   // and the shistory index updated accordingly.
   diskState = await writeAndParse(state, Paths.clean, {isFinalWrite: true});
   let tabState = state.windows[0].tabs[0];
   tabState.entries = tabState.entries.slice(2, 8);
   tabState.index = 3;
   Assert.deepEqual(state, diskState, "cap applied");
 });
 
 add_task(async function test_shistory_cap_lower_bound() {
   let state = createSessionState(1);
-  await setMaxBackForward(5, 5);
+  await prepareWithLimit(5, 5);
 
   // Cap is only applied on clean shutdown.
   let diskState = await writeAndParse(state, Paths.recovery);
   Assert.deepEqual(state, diskState, "no cap applied");
 
   // Check that the right number of shistory entries was discarded.
   diskState = await writeAndParse(state, Paths.clean, {isFinalWrite: true});
   let tabState = state.windows[0].tabs[0];
   tabState.entries = tabState.entries.slice(0, 6);
   Assert.deepEqual(state, diskState, "cap applied");
 });
 
 add_task(async function test_shistory_cap_upper_bound() {
   let state = createSessionState(MAX_ENTRIES);
-  await setMaxBackForward(5, 5);
+  await prepareWithLimit(5, 5);
 
   // Cap is only applied on clean shutdown.
   let diskState = await writeAndParse(state, Paths.recovery);
   Assert.deepEqual(state, diskState, "no cap applied");
 
   // Check that the right number of shistory entries was discarded
   // and the shistory index updated accordingly.
   diskState = await writeAndParse(state, Paths.clean, {isFinalWrite: true});
   let tabState = state.windows[0].tabs[0];
   tabState.entries = tabState.entries.slice(3);
   tabState.index = 6;
   Assert.deepEqual(state, diskState, "cap applied");
 });
+
+add_task(async function cleanup() {
+  await SessionFile.wipe();
+  await SessionFile.read();
+});