Bug 918024 - Remove the synchronous fallback for reading in Session Store initialization. r=Yoric, f=ttaubert
authorSteven MacLeod <smacleod@mozilla.com>
Tue, 19 Nov 2013 14:57:46 -0500
changeset 170936 eb1b72a569881fb5086a8ab6777b846394950977
parent 170935 26df3bab1ab93d23a9c4d5686bdaee3c9fed7a5e
child 170937 bb1a8915f98034dd30990c9ca99da4e46565a6c5
push id3224
push userlsblakk@mozilla.com
push dateTue, 04 Feb 2014 01:06:49 +0000
treeherdermozilla-beta@60c04d0987f1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersYoric
bugs918024
milestone28.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 918024 - Remove the synchronous fallback for reading in Session Store initialization. r=Yoric, f=ttaubert
browser/components/nsBrowserContentHandler.js
browser/components/sessionstore/nsISessionStartup.idl
browser/components/sessionstore/src/SessionFile.jsm
browser/components/sessionstore/src/SessionWorker.js
browser/components/sessionstore/src/nsSessionStartup.js
browser/components/sessionstore/test/browser_833286_atomic_backup.js
browser/components/sessionstore/test/unit/test_startup_nosession_sync.js
browser/components/sessionstore/test/unit/test_startup_session_sync.js
browser/components/sessionstore/test/unit/xpcshell.ini
browser/components/test/browser_bug538331.js
toolkit/components/telemetry/Histograms.json
--- a/browser/components/nsBrowserContentHandler.js
+++ b/browser/components/nsBrowserContentHandler.js
@@ -544,17 +544,17 @@ nsBrowserContentHandler.prototype = {
     if (!gFirstWindow) {
       gFirstWindow = true;
       if (PrivateBrowsingUtils.isInTemporaryAutoStartMode) {
         return "about:privatebrowsing";
       }
     }
 
     var overridePage = "";
-    var haveUpdateSession = false;
+    var willRestoreSession = false;
     try {
       // Read the old value of homepage_override.mstone before
       // needHomepageOverride updates it, so that we can later add it to the
       // URL if we do end up showing an overridePage. This makes it possible
       // to have the overridePage's content vary depending on the version we're
       // upgrading from.
       let old_mstone = "unknown";
       try {
@@ -563,21 +563,25 @@ nsBrowserContentHandler.prototype = {
       let override = needHomepageOverride(prefb);
       if (override != OVERRIDE_NONE) {
         switch (override) {
           case OVERRIDE_NEW_PROFILE:
             // New profile.
             overridePage = Services.urlFormatter.formatURLPref("startup.homepage_welcome_url");
             break;
           case OVERRIDE_NEW_MSTONE:
-            // Check whether we have a session to restore. If we do, we assume
-            // that this is an "update" session.
+            // Check whether we will restore a session. If we will, we assume
+            // that this is an "update" session. This does not take crashes
+            // into account because that requires waiting for the session file
+            // to be read. If a crash occurs after updating, before restarting,
+            // we may open the startPage in addition to restoring the session.
             var ss = Components.classes["@mozilla.org/browser/sessionstartup;1"]
                                .getService(Components.interfaces.nsISessionStartup);
-            haveUpdateSession = ss.doRestore();
+            willRestoreSession = ss.isAutomaticRestoreEnabled();
+
             overridePage = Services.urlFormatter.formatURLPref("startup.homepage_override_url");
             if (prefb.prefHasUserValue("app.update.postupdate"))
               overridePage = getPostUpdateOverridePage(overridePage);
 
             overridePage = overridePage.replace("%OLD_VERSION%", old_mstone);
             break;
         }
       }
@@ -595,17 +599,17 @@ nsBrowserContentHandler.prototype = {
     } catch (e) {
       Components.utils.reportError(e);
     }
 
     if (startPage == "about:blank")
       startPage = "";
 
     // Only show the startPage if we're not restoring an update session.
-    if (overridePage && startPage && !haveUpdateSession)
+    if (overridePage && startPage && !willRestoreSession)
       return overridePage + "|" + startPage;
 
     return overridePage || startPage || "about:blank";
   },
 
   get startPage() {
     var uri = Services.prefs.getComplexValue("browser.startup.homepage",
                                              nsIPrefLocalizedString).data;
--- a/browser/components/sessionstore/nsISessionStartup.idl
+++ b/browser/components/sessionstore/nsISessionStartup.idl
@@ -5,36 +5,42 @@
 #include "nsISupports.idl"
 
 /**
  * nsISessionStore keeps track of the current browsing state - i.e.
  * tab history, cookies, scroll state, form data, POSTDATA and window features
  * - and allows to restore everything into one window.
  */
 
-[scriptable, uuid(51f4b9f0-f3d2-11e2-bb62-2c24dd830245)]
+[scriptable, uuid(6c79d4c1-f071-4c5c-a7fb-676adb144584)]
 interface nsISessionStartup: nsISupports
 {
   /**
    * Return a promise that is resolved once initialization
    * is complete.
    */
   readonly attribute jsval onceInitialized;
 
   // Get session state
   readonly attribute jsval state;
 
   /**
-   * Determines whether there is a pending session restore and makes sure that
-   * we're initialized before returning. If we're not yet this will read the
-   * session file synchronously.
+   * Determines whether there is a pending session restore. Should only be
+   * called after initialization has completed.
    */
   boolean doRestore();
 
   /**
+   * Determines whether automatic session restoration is enabled for this
+   * launch of the browser. This does not include crash restoration, and will
+   * return false if restoration will only be caused by a crash.
+   */
+  boolean isAutomaticRestoreEnabled();
+
+  /**
    * Returns whether we will restore a session that ends up replacing the
    * homepage. The browser uses this to not start loading the homepage if
    * we're going to stop its load anyway shortly after.
    *
    * This is meant to be an optimization for the average case that loading the
    * session file finishes before we may want to start loading the default
    * homepage. Should this be called before the session file has been read it
    * will just return false.
--- a/browser/components/sessionstore/src/SessionFile.jsm
+++ b/browser/components/sessionstore/src/SessionFile.jsm
@@ -33,44 +33,29 @@ Cu.import("resource://gre/modules/Servic
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
 Cu.import("resource://gre/modules/osfile.jsm");
 Cu.import("resource://gre/modules/osfile/_PromiseWorker.jsm", this);
 Cu.import("resource://gre/modules/Promise.jsm");
 Cu.import("resource://gre/modules/AsyncShutdown.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "TelemetryStopwatch",
   "resource://gre/modules/TelemetryStopwatch.jsm");
-XPCOMUtils.defineLazyModuleGetter(this, "NetUtil",
-  "resource://gre/modules/NetUtil.jsm");
-XPCOMUtils.defineLazyModuleGetter(this, "FileUtils",
-  "resource://gre/modules/FileUtils.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "Task",
   "resource://gre/modules/Task.jsm");
 XPCOMUtils.defineLazyServiceGetter(this, "Telemetry",
   "@mozilla.org/base/telemetry;1", "nsITelemetry");
-XPCOMUtils.defineLazyModuleGetter(this, "Deprecated",
-  "resource://gre/modules/Deprecated.jsm");
 
 this.SessionFile = {
   /**
    * Read the contents of the session file, asynchronously.
    */
   read: function () {
     return SessionFileInternal.read();
   },
   /**
-   * Read the contents of the session file, synchronously.
-   */
-  syncRead: function () {
-    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);
   },
   /**
    * 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.
@@ -156,70 +141,16 @@ let SessionFileInternal = {
    */
   _latestWrite: null,
 
   /**
    * |true| once we have decided to stop receiving write instructiosn
    */
   _isClosed: false,
 
-  /**
-   * Utility function to safely read a file synchronously.
-   * @param aPath
-   *        A path to read the file from.
-   * @returns string if successful, undefined otherwise.
-   */
-  readAuxSync: function (aPath) {
-    let text;
-    try {
-      let file = new FileUtils.File(aPath);
-      let chan = NetUtil.newChannel(file);
-      let stream = chan.open();
-      text = NetUtil.readInputStreamToString(stream, stream.available(),
-        {charset: "utf-8"});
-    } catch (e if e.result == Components.results.NS_ERROR_FILE_NOT_FOUND) {
-      // Ignore exceptions about non-existent files.
-    } catch (ex) {
-      // Any other error.
-      Cu.reportError(ex);
-    } finally {
-      return text;
-    }
-  },
-
-  /**
-   * Read the sessionstore file synchronously.
-   *
-   * This function is meant to serve as a fallback in case of race
-   * between a synchronous usage of the API and asynchronous
-   * initialization.
-   *
-   * In case if sessionstore.js file does not exist or is corrupted (something
-   * happened between backup and write), attempt to read the sessionstore.bak
-   * instead.
-   */
-  syncRead: function () {
-    // Start measuring the duration of the synchronous read.
-    TelemetryStopwatch.start("FX_SESSION_RESTORE_SYNC_READ_FILE_MS");
-    // First read the sessionstore.js.
-    let text = this.readAuxSync(this.path);
-    if (typeof text === "undefined") {
-      // If sessionstore.js does not exist or is corrupted, read sessionstore.bak.
-      text = this.readAuxSync(this.backupPath);
-    }
-    // Finish the telemetry probe and return an empty string.
-    TelemetryStopwatch.finish("FX_SESSION_RESTORE_SYNC_READ_FILE_MS");
-    text = text || "";
-
-    // The worker needs to know the initial state read from
-    // disk so that writeLoadStateOnceAfterStartup() works.
-    SessionWorker.post("setInitialState", [text]);
-    return text;
-  },
-
   read: function () {
     return SessionWorker.post("read").then(msg => {
       this._recordTelemetry(msg.telemetry);
       return msg.ok;
     });
   },
 
   write: function (aData) {
--- a/browser/components/sessionstore/src/SessionWorker.js
+++ b/browser/components/sessionstore/src/SessionWorker.js
@@ -69,40 +69,16 @@ let Agent = {
 
   // 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
-   * can be removed when we're not supporting synchronous SessionStore
-   * initialization anymore. When sessionstore.js is read from disk
-   * synchronously the state string must be supplied to the worker manually by
-   * calling this method.
-   */
-  setInitialState: function (aState) {
-    // SessionFile.syncRead() should not be called after startup has finished.
-    // Thus we also don't support any setInitialState() calls after we already
-    // wrote the loadState to disk.
-    if (this.hasWrittenLoadStateOnce) {
-      throw new Error("writeLoadStateOnceAfterStartup() must only be called once.");
-    }
-
-    // Initial state might have been filled by read() already but yet we might
-    // be called by SessionFile.syncRead() before SessionStore.jsm had a chance
-    // to call writeLoadStateOnceAfterStartup(). It's safe to ignore
-    // setInitialState() calls if this happens.
-    if (!this.initialState) {
-      this.initialState = aState;
-    }
-  },
-
-  /**
    * Read the session from disk.
    * In case sessionstore.js does not exist, attempt to read sessionstore.bak.
    */
   read: function () {
     for (let path of [this.path, this.backupPath]) {
       try {
         let durationMs = Date.now();
         let bytes = File.read(path);
--- a/browser/components/sessionstore/src/nsSessionStartup.js
+++ b/browser/components/sessionstore/src/nsSessionStartup.js
@@ -43,16 +43,19 @@ Cu.import("resource://gre/modules/Teleme
 Cu.import("resource://gre/modules/PrivateBrowsingUtils.jsm");
 Cu.import("resource://gre/modules/Promise.jsm");
 
 XPCOMUtils.defineLazyModuleGetter(this, "SessionFile",
   "resource:///modules/sessionstore/SessionFile.jsm");
 
 const STATE_RUNNING_STR = "running";
 
+// 'browser.startup.page' preference value to resume the previous session.
+const BROWSER_STARTUP_RESUME_SESSION = 3;
+
 function debug(aMsg) {
   aMsg = ("SessionStartup: " + aMsg).replace(/\S{80}/g, "$&\n");
   Services.console.logStringMessage(aMsg);
 }
 
 let gOnceInitializedDeferred = Promise.defer();
 
 /* :::::::: The Service ::::::::::::::: */
@@ -133,17 +136,17 @@ SessionStartup.prototype = {
           debug("The session file contained un-eval-able JSON: " + ex);
           corruptFile = true;
         }
       }
       Services.telemetry.getHistogramById("FX_SESSION_RESTORE_CORRUPT_FILE").add(corruptFile);
 
       let doResumeSessionOnce = Services.prefs.getBoolPref("browser.sessionstore.resume_session_once");
       let doResumeSession = doResumeSessionOnce ||
-            Services.prefs.getIntPref("browser.startup.page") == 3;
+            Services.prefs.getIntPref("browser.startup.page") == BROWSER_STARTUP_RESUME_SESSION;
 
       // If this is a normal restore then throw away any previous session
       if (!doResumeSessionOnce)
         delete this._initialState.lastSessionState;
 
       let resumeFromCrash = Services.prefs.getBoolPref("browser.sessionstore.resume_from_crash");
       let lastSessionCrashed =
         this._initialState && this._initialState.session &&
@@ -221,27 +224,39 @@ SessionStartup.prototype = {
    * Get the session state as a jsval
    */
   get state() {
     this._ensureInitialized();
     return this._initialState;
   },
 
   /**
-   * Determines whether there is a pending session restore and makes sure that
-   * we're initialized before returning. If we're not yet this will read the
-   * session file synchronously.
+   * Determines whether there is a pending session restore. Should only be
+   * called after initialization has completed.
+   * @throws Error if initialization is not complete yet.
    * @returns bool
    */
   doRestore: function sss_doRestore() {
     this._ensureInitialized();
     return this._willRestore();
   },
 
   /**
+   * Determines whether automatic session restoration is enabled for this
+   * launch of the browser. This does not include crash restoration. In
+   * particular, if session restore is configured to restore only in case of
+   * crash, this method returns false.
+   * @returns bool
+   */
+  isAutomaticRestoreEnabled: function () {
+    return Services.prefs.getBoolPref("browser.sessionstore.resume_session_once") ||
+           Services.prefs.getIntPref("browser.startup.page") == BROWSER_STARTUP_RESUME_SESSION;
+  },
+
+  /**
    * Determines whether there is a pending session restore.
    * @returns bool
    */
   _willRestore: function () {
     return this._sessionType == Ci.nsISessionStartup.RECOVER_SESSION ||
            this._sessionType == Ci.nsISessionStartup.RESUME_SESSION;
   },
 
@@ -270,30 +285,22 @@ SessionStartup.prototype = {
   /**
    * Get the type of pending session store, if any.
    */
   get sessionType() {
     this._ensureInitialized();
     return this._sessionType;
   },
 
-  // Ensure that initialization is complete.
-  // If initialization is not complete yet, fall back to a synchronous
-  // initialization and kill ongoing asynchronous initialization
+  // Ensure that initialization is complete. If initialization is not complete
+  // yet, something is attempting to use the old synchronous initialization,
+  // throw an error.
   _ensureInitialized: function sss__ensureInitialized() {
-    try {
-      if (this._initialized) {
-        // Initialization is complete, nothing else to do
-        return;
-      }
-      let contents = SessionFile.syncRead();
-      this._onSessionFileRead(contents);
-    } catch(ex) {
-      debug("ensureInitialized: could not read session " + ex + ", " + ex.stack);
-      throw ex;
+    if (!this._initialized) {
+      throw new Error("Session Store is not initialized.");
     }
   },
 
   /* ........ QueryInterface .............. */
   QueryInterface : XPCOMUtils.generateQI([Ci.nsIObserver,
                                           Ci.nsISupportsWeakReference,
                                           Ci.nsISessionStartup]),
   classID:          Components.ID("{ec7a6c20-e081-11da-8ad9-0800200c9a66}")
--- a/browser/components/sessionstore/test/browser_833286_atomic_backup.js
+++ b/browser/components/sessionstore/test/browser_833286_atomic_backup.js
@@ -99,36 +99,26 @@ function testReadBackup() {
   // Read latest sessionstore.js.
   array = yield OS.File.read(path);
   gSSData = gDecoder.decode(array);
 
   // Read sessionstore.js with SessionFile.read.
   let ssDataRead = yield SessionFile.read();
   is(ssDataRead, gSSData, "SessionFile.read read sessionstore.js correctly.");
 
-  // Read sessionstore.js with SessionFile.syncRead.
-  ssDataRead = SessionFile.syncRead();
-  is(ssDataRead, gSSData,
-    "SessionFile.syncRead read sessionstore.js correctly.");
-
   // Remove sessionstore.js to test fallback onto sessionstore.bak.
   yield OS.File.remove(path);
   ssExists = yield OS.File.exists(path);
   ok(!ssExists, "sessionstore.js should be removed now.");
 
   // Read sessionstore.bak with SessionFile.read.
   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(testBackupUnchanged);
 }
 
 function testBackupUnchanged() {
   // Ensure sessionstore.bak is backed up only once.
 
   // Read sessionstore.bak data.
   let array = yield OS.File.read(backupPath);
deleted file mode 100644
--- a/browser/components/sessionstore/test/unit/test_startup_nosession_sync.js
+++ /dev/null
@@ -1,15 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-
-// Test nsISessionStartup.sessionType in the following scenario:
-// - no sessionstore.js;
-// - the session store has not been loaded yet, so we have to trigger
-//    synchronous fallback
-
-function run_test() {
-  do_get_profile();
-  let startup = Cc["@mozilla.org/browser/sessionstartup;1"].
-    getService(Ci.nsISessionStartup);
-  do_check_eq(startup.sessionType, Ci.nsISessionStartup.NO_SESSION);
-}
\ No newline at end of file
deleted file mode 100644
--- a/browser/components/sessionstore/test/unit/test_startup_session_sync.js
+++ /dev/null
@@ -1,17 +0,0 @@
-/* Any copyright is dedicated to the Public Domain.
-   http://creativecommons.org/publicdomain/zero/1.0/ */
-
-
-// Test nsISessionStartup.sessionType in the following scenario:
-// - valid sessionstore.js;
-// - the session store has not been loaded yet, so we have to trigger
-//    synchronous fallback
-
-function run_test() {
-  let profd = do_get_profile();
-  let source = do_get_file("data/sessionstore_valid.js");
-  source.copyTo(profd, "sessionstore.js");
-  let startup = Cc["@mozilla.org/browser/sessionstartup;1"].
-    getService(Ci.nsISessionStartup);
-  do_check_eq(startup.sessionType, Ci.nsISessionStartup.DEFER_SESSION);
-}
\ No newline at end of file
--- a/browser/components/sessionstore/test/unit/xpcshell.ini
+++ b/browser/components/sessionstore/test/unit/xpcshell.ini
@@ -1,12 +1,10 @@
 [DEFAULT]
 head = head.js
 tail =
 firefox-appdir = browser
 support-files = data/sessionstore_valid.js
 
 [test_backup.js]
 [test_backup_once.js]
-[test_startup_nosession_sync.js]
 [test_startup_nosession_async.js]
-[test_startup_session_sync.js]
 [test_startup_session_async.js]
--- a/browser/components/test/browser_bug538331.js
+++ b/browser/components/test/browser_bug538331.js
@@ -113,16 +113,20 @@ this.__defineGetter__("gBG", function() 
                     getService(Ci.nsIBrowserGlue).
                     QueryInterface(Ci.nsIObserver);
 });
 
 function test()
 {
   waitForExplicitFinish();
 
+  // Reset the startup page pref since it may have been set by other tests
+  // and we will assume it is default.
+  Services.prefs.clearUserPref('browser.startup.page');
+
   if (gPrefService.prefHasUserValue(PREF_MSTONE)) {
     gOriginalMStone = gPrefService.getCharPref(PREF_MSTONE);
   }
 
   if (gPrefService.prefHasUserValue(PREF_OVERRIDE_URL)) {
     gOriginalOverrideURL = gPrefService.getCharPref(PREF_OVERRIDE_URL);
   }
 
--- a/toolkit/components/telemetry/Histograms.json
+++ b/toolkit/components/telemetry/Histograms.json
@@ -2799,23 +2799,16 @@
   },
   "FX_SESSION_RESTORE_READ_FILE_MS": {
     "kind": "exponential",
     "high": "3000",
     "n_buckets": 10,
     "extended_statistics_ok": true,
     "description": "Session restore: Time to read the session data from the file on disk (ms)"
   },
-  "FX_SESSION_RESTORE_SYNC_READ_FILE_MS": {
-    "kind": "exponential",
-    "high": "3000",
-    "n_buckets": 10,
-    "extended_statistics_ok": true,
-    "description": "Session restore: Time to read the session data from the file on disk, using the synchronous fallback (ms)"
-  },
   "FX_SESSION_RESTORE_WRITE_FILE_MS": {
     "kind": "exponential",
     "high": "3000",
     "n_buckets": 10,
     "extended_statistics_ok": true,
     "description": "Session restore: Time to write the session data to the file on disk (ms)"
   },
   "FX_SESSION_RESTORE_WRITE_FILE_LONGEST_OP_MS": {