Bug 887780 - Stop writing state immediately after startup and switch to CrashMonitor for crash detection. r=Yoric, r=ttaubert

X-Git-Commit-ID: bb02f51399059295041176769438b0f451f983bf

browser/components/sessionstore/nsISessionStartup.idl

@@ -10,7 +10,7 @@
  * - and allows to restore everything into one window.
  */
 
-[scriptable, uuid(6c79d4c1-f071-4c5c-a7fb-676adb144584)]
+[scriptable, uuid(934697e4-3807-47f8-b6c9-6caa8d83ccd1)]
 interface nsISessionStartup: nsISupports
 {
   /**
@@ -62,4 +62,5 @@ interface nsISessionStartup: nsISupports
   const unsigned long DEFER_SESSION = 3;
 
   readonly attribute unsigned long sessionType;
+  readonly attribute bool previousSessionCrashed;
 };

browser/components/sessionstore/src/SessionFile.jsm

@@ -74,13 +74,6 @@ this.SessionFile = {
   gatherTelemetry: function(aData) {
     return SessionFileInternal.gatherTelemetry(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.
-   */
-  writeLoadStateOnceAfterStartup: function (aLoadState) {
-    SessionFileInternal.writeLoadStateOnceAfterStartup(aLoadState);
-  },
   /**
    * Create a backup copy, asynchronously.
    * This is designed to perform backup on upgrade.
@@ -181,13 +174,6 @@ let SessionFileInternal = {
     }.bind(this));
   },
 
-  writeLoadStateOnceAfterStartup: function (aLoadState) {
-    SessionWorker.post("writeLoadStateOnceAfterStartup", [aLoadState]).then(msg => {
-      this._recordTelemetry(msg.telemetry);
-      return msg;
-    }, console.error);
-  },
-
   createBackupCopy: function (ext) {
     return SessionWorker.post("createBackupCopy", [ext]);
   },

browser/components/sessionstore/src/SessionStore.jsm

@@ -15,9 +15,6 @@ const STATE_STOPPED = 0;
 const STATE_RUNNING = 1;
 const STATE_QUITTING = -1;
 
-const STATE_STOPPED_STR = "stopped";
-const STATE_RUNNING_STR = "running";
-
 const TAB_STATE_NEEDS_RESTORE = 1;
 const TAB_STATE_RESTORING = 2;
 
@@ -404,7 +401,10 @@ let SessionStoreInternal = {
     this._initialized = true;
   },
 
-  initSession: function ssi_initSession() {
+  /**
+   * Initialize the session using the state provided by SessionStartup
+   */
+  initSession: function () {
     let state;
     let ss = gSessionStartup;
 
@@ -437,10 +437,7 @@ let SessionStoreInternal = {
           // restore it
           LastSession.setState(state.lastSessionState);
 
-          let lastSessionCrashed =
-            state.session && state.session.state &&
-            state.session.state == STATE_RUNNING_STR;
-          if (lastSessionCrashed) {
+          if (ss.previousSessionCrashed) {
             this._recentCrashes = (state.session &&
                                    state.session.recentCrashes || 0) + 1;
 
@@ -829,11 +826,6 @@ let SessionStoreInternal = {
           let overwrite = this._isCmdLineEmpty(aWindow, aInitialState);
           let options = {firstWindow: true, overwriteTabs: overwrite};
           this.restoreWindow(aWindow, aInitialState, options);
-
-          // _loadState changed from "stopped" to "running". Save the session's
-          // load state immediately so that crashes happening during startup
-          // are correctly counted.
-          SessionFile.writeLoadStateOnceAfterStartup(STATE_RUNNING_STR);
         }
       }
       else {
@@ -2230,7 +2222,6 @@ let SessionStoreInternal = {
       ix = -1;
 
     let session = {
-      state: this._loadState == STATE_RUNNING ? STATE_RUNNING_STR : STATE_STOPPED_STR,
       lastUpdate: Date.now(),
       startTime: this._sessionStartTime,
       recentCrashes: this._recentCrashes

browser/components/sessionstore/src/SessionWorker.js

@@ -54,13 +54,6 @@ 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
@@ -83,10 +76,9 @@ let Agent = {
         let durationMs = Date.now();
         let bytes = File.read(path);
         durationMs = Date.now() - durationMs;
-        this.initialState = Decoder.decode(bytes);
 
         return {
-          result: this.initialState,
+          result: Decoder.decode(bytes),
           telemetry: {FX_SESSION_RESTORE_READ_FILE_MS: durationMs,
                       FX_SESSION_RESTORE_FILE_SIZE_BYTES: bytes.byteLength}
         };
@@ -140,37 +132,6 @@ let Agent = {
     return Statistics.collect(stateString);
   },
 
-  /**
-   * 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.
-   */
-  writeLoadStateOnceAfterStartup: function (loadState) {
-    if (this.hasWrittenLoadStateOnce) {
-      throw new Error("writeLoadStateOnceAfterStartup() must only be called once.");
-    }
-
-    if (!this.initialState) {
-      throw new Error("writeLoadStateOnceAfterStartup() must not be called " +
-                      "without a valid session state or before it has been " +
-                      "read from disk.");
-    }
-
-    // Make sure we can't call this function twice.
-    this.hasWrittenLoadStateOnce = true;
-
-    let state;
-    try {
-      state = JSON.parse(this.initialState);
-    } finally {
-      this.initialState = null;
-    }
-
-    state.session = state.session || {};
-    state.session.state = loadState;
-    return this._write(JSON.stringify(state));
-  },
-
   /**
    * Write a stateString to disk
    */

browser/components/sessionstore/src/nsSessionStartup.js

@@ -14,12 +14,10 @@
  * mode is active, however, the session is never restored.
  *
  * Crash Detection
- * The session file stores a session.state property, that
- * indicates whether the browser is currently running. When the browser shuts
- * down, the field is changed to "stopped". At startup, this field is read, and
- * if its value is "running", then it's assumed that the browser had previously
- * crashed, or at the very least that something bad happened, and that we should
- * restore the session.
+ * The CrashMonitor is used to check if the final session state was successfully
+ * written at shutdown of the last session. If we did not reach
+ * 'sessionstore-final-state-write-complete', then it's assumed that the browser
+ * has previously crashed and we should restore the session.
  *
  * Forced Restarts
  * In the event that a restart is required due to application update or extension
@@ -47,6 +45,8 @@ XPCOMUtils.defineLazyModuleGetter(this, "console",
   "resource://gre/modules/devtools/Console.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "SessionFile",
   "resource:///modules/sessionstore/SessionFile.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "CrashMonitor",
+  "resource://gre/modules/CrashMonitor.jsm");
 
 const STATE_RUNNING_STR = "running";
 
@@ -72,6 +72,9 @@ SessionStartup.prototype = {
   _sessionType: Ci.nsISessionStartup.NO_SESSION,
   _initialized: false,
 
+  // Stores whether the previous session crashed.
+  _previousSessionCrashed: null,
+
 /* ........ Global Event Handlers .............. */
 
   /**
@@ -99,71 +102,78 @@ SessionStartup.prototype = {
     return string;
   },
 
-  _onSessionFileRead: function sss_onSessionFileRead(aStateString) {
-    if (this._initialized) {
-      // Initialization is complete, nothing else to do
+  /**
+   * Complete initialization once the Session File has been read
+   *
+   * @param stateString
+   *        string The Session State string read from disk
+   */
+  _onSessionFileRead: function (stateString) {
+    this._initialized = true;
+
+    // Let observers modify the state before it is used
+    let supportsStateString = this._createSupportsString(stateString);
+    Services.obs.notifyObservers(supportsStateString, "sessionstore-state-read", "");
+    stateString = supportsStateString.data;
+
+    // No valid session found.
+    if (!stateString) {
+      this._sessionType = Ci.nsISessionStartup.NO_SESSION;
+      Services.obs.notifyObservers(null, "sessionstore-state-finalized", "");
+      gOnceInitializedDeferred.resolve();
       return;
     }
-    try {
-      this._initialized = true;
 
-      // Let observers modify the state before it is used
-      let supportsStateString = this._createSupportsString(aStateString);
-      Services.obs.notifyObservers(supportsStateString, "sessionstore-state-read", "");
-      aStateString = supportsStateString.data;
+    this._initialState =  this._parseStateString(stateString);
 
-      // No valid session found.
-      if (!aStateString) {
-        this._sessionType = Ci.nsISessionStartup.NO_SESSION;
-        return;
+    let shouldResumeSessionOnce = Services.prefs.getBoolPref("browser.sessionstore.resume_session_once");
+    let shouldResumeSession = shouldResumeSessionOnce ||
+          Services.prefs.getIntPref("browser.startup.page") == BROWSER_STARTUP_RESUME_SESSION;
+
+    // If this is a normal restore then throw away any previous session
+    if (!shouldResumeSessionOnce)
+      delete this._initialState.lastSessionState;
+
+    let resumeFromCrash = Services.prefs.getBoolPref("browser.sessionstore.resume_from_crash");
+
+    CrashMonitor.previousCheckpoints.then(checkpoints => {
+      if (checkpoints) {
+        // If the previous session finished writing the final state, we'll
+        // assume there was no crash.
+        this._previousSessionCrashed = !checkpoints["sessionstore-final-state-write-complete"];
+      } else {
+        // If the Crash Monitor could not load a checkpoints file it will
+        // provide null. This could occur on the first run after updating to
+        // a version including the Crash Monitor, or if the checkpoints file
+        // was removed.
+        //
+        // If this is the first run after an update, sessionstore.js should
+        // still contain the session.state flag to indicate if the session
+        // crashed. If it is not present, we will assume this was not the first
+        // run after update and the checkpoints file was somehow corrupted or
+        // removed by a crash.
+        //
+        // If the session.state flag is present, we will fallback to using it
+        // for crash detection - If the last write of sessionstore.js had it
+        // set to "running", we crashed.
+        let stateFlagPresent = (this._initialState &&
+                                this._initialState.session &&
+                                this._initialState.session.state);
+
+
+        this._previousSessionCrashed = !stateFlagPresent ||
+                                       (this._initialState.session.state == STATE_RUNNING_STR);
       }
 
-      // parse the session state into a JS object
-      // remove unneeded braces (added for compatibility with Firefox 2.0 and 3.0)
-      if (aStateString.charAt(0) == '(')
-        aStateString = aStateString.slice(1, -1);
-      let corruptFile = false;
-      try {
-        this._initialState = JSON.parse(aStateString);
-      }
-      catch (ex) {
-        debug("The session file contained un-parse-able JSON: " + ex);
-        // This is not valid JSON, but this might still be valid JavaScript,
-        // as used in FF2/FF3, so we need to eval.
-        // evalInSandbox will throw if aStateString is not parse-able.
-        try {
-          var s = new Cu.Sandbox("about:blank", {sandboxName: 'nsSessionStartup'});
-          this._initialState = Cu.evalInSandbox("(" + aStateString + ")", s);
-        } catch(ex) {
-          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") == 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 &&
-        this._initialState.session.state &&
-        this._initialState.session.state == STATE_RUNNING_STR;
-
       // Report shutdown success via telemetry. Shortcoming here are
       // being-killed-by-OS-shutdown-logic, shutdown freezing after
       // session restore was written, etc.
-      Services.telemetry.getHistogramById("SHUTDOWN_OK").add(!lastSessionCrashed);
+      Services.telemetry.getHistogramById("SHUTDOWN_OK").add(!this._previousSessionCrashed);
 
       // set the startup type
-      if (lastSessionCrashed && resumeFromCrash)
+      if (this._previousSessionCrashed && resumeFromCrash)
         this._sessionType = Ci.nsISessionStartup.RECOVER_SESSION;
-      else if (!lastSessionCrashed && doResumeSession)
+      else if (!this._previousSessionCrashed && shouldResumeSession)
         this._sessionType = Ci.nsISessionStartup.RESUME_SESSION;
       else if (this._initialState)
         this._sessionType = Ci.nsISessionStartup.DEFER_SESSION;
@@ -175,11 +185,33 @@ SessionStartup.prototype = {
       if (this._sessionType != Ci.nsISessionStartup.NO_SESSION)
         Services.obs.addObserver(this, "browser:purge-session-history", true);
 
-    } finally {
       // We're ready. Notify everyone else.
       Services.obs.notifyObservers(null, "sessionstore-state-finalized", "");
       gOnceInitializedDeferred.resolve();
+    });
+  },
+
+
+  /**
+   * Convert the Session State string into a state object
+   *
+   * @param stateString
+   *        string The Session State string read from disk
+   * @returns {State} a Session State object
+   */
+  _parseStateString: function (stateString) {
+    let state = null;
+    let corruptFile = false;
+
+    try {
+      state = JSON.parse(stateString);
+    } catch (ex) {
+      debug("The session file contained un-parse-able JSON: " + ex);
+      corruptFile = true;
     }
+    Services.telemetry.getHistogramById("FX_SESSION_RESTORE_CORRUPT_FILE").add(corruptFile);
+
+    return state;
   },
 
   /**
@@ -292,6 +324,14 @@ SessionStartup.prototype = {
     return this._sessionType;
   },
 
+  /**
+   * Get whether the previous session crashed.
+   */
+  get previousSessionCrashed() {
+    this._ensureInitialized();
+    return this._previousSessionCrashed;
+  },
+
   // Ensure that initialization is complete. If initialization is not complete
   // yet, something is attempting to use the old synchronous initialization,
   // throw an error.

browser/components/sessionstore/test/browser_665702-state_session.js

@@ -18,7 +18,7 @@ function test() {
   ok(currentState.session, "session data returned by getBrowserState");
 
   let keys = Object.keys(currentState.session);
-  let expectedKeys = ["state", "lastUpdate", "startTime", "recentCrashes"];
+  let expectedKeys = ["lastUpdate", "startTime", "recentCrashes"];
   ok(compareArray(keys.sort(), expectedKeys.sort()),
      "session object from getBrowserState has correct keys");
 }

browser/components/sessionstore/test/unit/data/sessionCheckpoints_all.json

@@ -0,0 +1 @@
+{"profile-after-change":true,"final-ui-startup":true,"sessionstore-windows-restored":true,"quit-application-granted":true,"quit-application":true,"sessionstore-final-state-write-complete":true,"profile-change-net-teardown":true,"profile-change-teardown":true,"profile-before-change":true}

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

@@ -17,10 +17,17 @@ let afterSessionStartupInitialization =
         do_throw(ex);
       }
     };
+
+    // We need the Crash Monitor initialized for sessionstartup to run
+    // successfully.
+    Components.utils.import("resource://gre/modules/CrashMonitor.jsm");
+    CrashMonitor.init();
+
+    // Start sessionstartup initialization.
     let startup = Cc["@mozilla.org/browser/sessionstartup;1"].
       getService(Ci.nsIObserver);
     Services.obs.addObserver(startup, "final-ui-startup", false);
     Services.obs.addObserver(startup, "quit-application", false);
     Services.obs.notifyObservers(null, "final-ui-startup", "");
     Services.obs.addObserver(observer, "sessionstore-state-finalized", false);
-};
+};

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

@@ -4,13 +4,17 @@
 
 // Test nsISessionStartup.sessionType in the following scenario:
 // - valid sessionstore.js;
-// - the session store has been loaded, so no need to go
-//    through the synchronous fallback
+// - valid sessionCheckpoints.json with all checkpoints;
+// - the session store has been loaded
 
 function run_test() {
   let profd = do_get_profile();
-  let source = do_get_file("data/sessionstore_valid.js");
-  source.copyTo(profd, "sessionstore.js");
+
+  let sourceSession = do_get_file("data/sessionstore_valid.js");
+  sourceSession.copyTo(profd, "sessionstore.js");
+
+  let sourceCheckpoints = do_get_file("data/sessionCheckpoints_all.json");
+  sourceCheckpoints.copyTo(profd, "sessionCheckpoints.json");
 
   do_test_pending();
   let startup = Cc["@mozilla.org/browser/sessionstartup;1"].
@@ -20,4 +24,4 @@ function run_test() {
     do_check_eq(startup.sessionType, Ci.nsISessionStartup.DEFER_SESSION);
     do_test_finished();
   });
-}
+}

browser/components/sessionstore/test/unit/xpcshell.ini

@@ -2,7 +2,9 @@
 head = head.js
 tail =
 firefox-appdir = browser
-support-files = data/sessionstore_valid.js
+support-files =
+  data/sessionCheckpoints_all.json
+  data/sessionstore_valid.js
 
 [test_backup.js]
 [test_backup_once.js]