Closed Bug 395488 Opened 12 years ago Closed 11 years ago

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

Categories

(Firefox :: Session Restore, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 3.6a1

People

(Reporter: zeniko, Assigned: zeniko)

References

Details

(Keywords: verified1.9.1)

Attachments

(1 file, 4 obsolete files)

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.
so the patch in bug 368676 is not useful?
OS: Windows XP → All
Hardware: PC → All
This also fixes bug 409129 (mostly a DUPE of this one) and bug 451292.
Attachment #335051 - Flags: review?(dietrich)
Assignee: nobody → zeniko
Attached patch unbitrotted (obsolete) — Splinter Review
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).
Attachment #335051 - Attachment is obsolete: true
Attachment #342771 - Flags: review?(dietrich)
Attachment #335051 - Flags: review?(dietrich)
Attached patch unbitrotted (obsolete) — Splinter Review
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).
Attachment #342771 - Attachment is obsolete: true
Attachment #342895 - Flags: review?(dietrich)
Attachment #342771 - Flags: review?(dietrich)
Attached patch unbitrotted (obsolete) — Splinter Review
Attachment #342895 - Attachment is obsolete: true
Attachment #343051 - Flags: review?(dietrich)
Attachment #342895 - Flags: review?(dietrich)
Blocks: 451292
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.
Flags: blocking-firefox3.1?
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).
Whiteboard: [has patch][needs review dietrich]
Target Milestone: --- → Firefox 3.1
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
Attachment #343051 - Flags: review?(dietrich) → review+
Attachment #343051 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [has patch][needs review dietrich]
Target Milestone: Firefox 3.1 → Firefox 3.1b2
Comment on attachment 345929 [details] [diff] [review]
for check-in
[Checkin: Comment 10]

http://hg.mozilla.org/mozilla-central/rev/957c90a0ee80
Attachment #345929 - Attachment description: for check-in → for check-in [Checkin: Comment 10]
Status: NEW → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Version: unspecified → Trunk
Flags: blocking-firefox3.1?
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.
(In reply to comment #11)
That's one of the dependencies of bug 429414 (hopefully fixed by bug 407110).
Could this be responsible for bug 462973?
Depends on: redheadedstepchild
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 3.1b2 → ---
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Dietrich, so we need approval for 1.9.1 now or was it only backed-out on trunk?
Target Milestone: --- → Firefox 3.2a1
(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).
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
Status: RESOLVED → VERIFIED
Flags: in-litmus?
Keywords: verified1.9.1
in-litmus+
https://litmus.mozilla.org/show_test.cgi?id=7797
Flags: in-litmus? → in-litmus+
You need to log in before you can comment on or make changes to this bug.