395488 - Session restore restores blank windows if Firefox was shut down too quickly

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[Simon Bünzli]

Steps to Reproduce
1. Open at least a dozen windows with non-blank content (the default homepage will do).
2. Set Firefox to resume the session at startup
3. Quit and reopen Firefox
4. Quit again _ASAP_ and reopen Firefox again

Actual result:
Many if not most windows contain only blank tabs.

Expected result:
Everything is restored after point 4 as after point 3.

See bug 368676 for a 10-line patch to fix this issue.

Comment 1

[Wayne Mery (:wsmwk)]

so the patch in bug 368676 is not useful?

Comment 2

[Simon Bünzli]

Attachment: hold on to the data to be restored until it actually has

This also fixes bug 409129 (mostly a DUPE of this one) and bug 451292.

Comment 3

[Simon Bünzli]

Attachment: unbitrotted

Minor change: We first save 500ms after restoring the session at startup so that the new about:sessionrestore has time to store the data of the crashed session in a text field (else we might not allow to restore a previously crashed session on consecutive crashes).

Comment 4

[Simon Bünzli]

Attachment: unbitrotted

Minor change: Passing the crashed session state to about:sessionstore through sss_restoreWindow allows us to instantly save afterwards (obsoleting the 500ms delay from comment #3).

Comment 5

[Simon Bünzli]

Attachment: unbitrotted

Comment 6

[Simon Bünzli]

This bug should be fixed for Firefox 3.1, as otherwise we might get people in an endless crashing loop if Firefox crashes before it can update sessionstore.js to tell itself to display about:sessionrestore at the next startup.

Comment 7

[Sylvain Pasche]

I'd add that can also hurt if you have some pages that freeze Firefox on startup. Recently I had such pages in my session, consuming 100% cpu until I had to kill the application and the about:sessionrestore page would never show after restarting the browser (indefinitely).

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 343051 [details] [diff] [review]
unbitrotted

>@@ -471,61 +482,72 @@ SessionStoreService.prototype = {
>   /**
>    * On window close...
>    * - remove event listeners from tabs
>    * - save all window data
>    * @param aWindow
>    *        Window reference
>    */
>   onClose: function sss_onClose(aWindow) {
>+    // this window was about to be restored - conserve its original data, if any
>+    let isFullyLoaded = !aWindow.__SS_restoreID;
>+    if (!isFullyLoaded) {

i'd prefer this to be abstracted into a isWindowLoaded(aWindow) method, and used everywhere where you're currently checking for __SS_restoreID. this centralizes implementation, as well as resulting in self-documenting code.

r=me otherwise

Comment 9

[Simon Bünzli]

Attachment: for check-in [Checkin: Comment 10]

Comment 10

[Serge Gautherie (:sgautherie)]

Comment on attachment 345929 [details] [diff] [review]
for check-in
[Checkin: Comment 10]

http://hg.mozilla.org/mozilla-central/rev/957c90a0ee80

Comment 11

[Misak Khachatryan]

I don't know, it's related or not, but i noticed that after checkin and my port of this bug to SeaMonkey, error console outputs "Error: [Exception... "Component is not available"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: file:///home/misak/workspace/src/suite-opt/mozilla/dist/bin/components/nsSessionStore.js :: sss_saveState :: line 2215"  data: no]
Source File: file:///home/misak/workspace/src/suite-opt/mozilla/dist/bin/components/nsSessionStore.js
Line: 2215", which points to line "stateString.data = oState.toSource();"
Here is surrounding code:

  saveState: function sss_saveState(aUpdateAll) {
    // if crash recovery is disabled, only save session resuming information
    if (!this._resume_from_crash && this._loadState == STATE_RUNNING)
      return;
    
    var oState = this._getCurrentState(aUpdateAll);
    oState.session = {
      state: this._loadState == STATE_RUNNING ? STATE_RUNNING_STR : STATE_STOPPED_STR,
      lastUpdate: Date.now()
    };
    if (this._recentCrashes)
      oState.session.recentCrashes = this._recentCrashes;
    
    var stateString = Components.classes["@mozilla.org/supports-string;1"].
                        createInstance(Components.interfaces.nsISupportsString);
    stateString.data = oState.toSource();
    
    var observerService = Components.classes["@mozilla.org/observer-service;1"].
                          getService(Components.interfaces.nsIObserverService);
    observerService.notifyObservers(stateString, "sessionstore-state-write", "");
    
    // don't touch the file if an observer has deleted all state data
    if (stateString.data)
      this._writeFile(this._sessionFile, stateString.data);

    this._lastSaveTime = Date.now();
  },

Also "Error: uncaught exception: 2147500034" happens more frequently.

Comment 12

[Simon Bünzli]

(In reply to comment #11)
That's one of the dependencies of bug 429414 (hopefully fixed by bug 407110).

Comment 13

[Karl Tomlinson (:karlt)]

Could this be responsible for bug 462973?

Comment 14

[Dietrich Ayala (:dietrich)]

backed out, due to bug 462973: http://hg.mozilla.org/mozilla-central/rev/6682e271edff

will re-land with the patch in that bug once we have confirmation of the regression.

Comment 15

[Dietrich Ayala (:dietrich)]

relanded today, should stick: https://bugzilla.mozilla.org/show_bug.cgi?id=462973#c20

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dietrich, so we need approval for 1.9.1 now or was it only backed-out on trunk?

Comment 17

[Simon Bünzli]

(In reply to comment #16)
This was never backed out of the 1.9.1 branch, only mozilla-central (see comment #14 and bug 462973 comment #17 and following).

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks Simon. I just wanted to have a confirmation here. A FFT testcase for Litmus cannot hurt. Adding flag for now.

I'm not able to see it with builds on OS X and Windows:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090310 Minefield/3.2a1pre Ubiquity/0.1.5 ID:20090310030639

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090309 Shiretoko/3.1b4pre ID:20090309020654

Comment 19

[u279076]

in-litmus+
https://litmus.mozilla.org/show_test.cgi?id=7797