Closed Bug 824107 Opened 11 years ago Closed 11 years ago

Session Data lost on startup if nsISessionStartup.sessionType is referenced before async load complete

Categories

(Firefox :: Session Restore, defect)

x86
Windows XP
defect
Not set
major

Tracking

()

RESOLVED FIXED
Firefox 20

People

(Reporter: morac, Assigned: morac)

References

Details

(Keywords: dataloss, regression)

Attachments

(2 files, 1 obsolete file)

My add-on, Session Manager, currently reads the nsISessionStartup.sessionType from a component during the "final-ui-startup" to determine if a browser restart is being done.

Doing this kills the async init in nsISessionStartup (see bug 52150 comment 46).  

The problem is that this appears to cause Session Restore to fail.  I tested setting Firefox to restore window and tabs, restarting and crashing the browser.

In all cases a blank tab is loaded if nsISessionStartup.sessionType is read.  If it's not read, the browser behaves normally.

The 0.9.7.2 version of the Session Manager add-on will trigger this.  I've marked it as not compatible with Firefox 20.a1, but you should be able to manually install.  The latest development version (2012-12-22) removes the sessionType read and it works.   
https://addons.mozilla.org/en-US/firefox/addon/session-manager/
Assignee: nobody → dteller
Unfortunately, I won't have time to work on this until Jan 7th.
Assignee: dteller → nobody
I took a brief glance at this and I'm pretty sure SessionStore is accessing gSessionStartup.onceInitialized after SessionStart's _onSessionFileRead function has executed in this case since SessionStore's initService is called after the first browser window loads and Session Manager was accessing SessionStart prior to the first window opening.

That shouldn't matter since gOnceInitializedDeferred.promise should simply immediately call SessionStore's initSession function upon SessionStore reading gSessionStartup.onceInitialized.  

I'm not sure this is happening though since even the deferred session data is lost.  It's almost as if initSession is never getting called.  Since the session data gets saved on shutdown though that means it must be being called as _sessionInitialized must be true, so that would leave _initialState being blank.

I'll have some time next week to look at this further so I should have a conclusive answer then.
Attached file Test Case add-on
I threw together a bare bones test case add-on that triggers this problem.

Also looking at the error console when the bug it occurs I see:

SessionStartup: _ensureInitialized: null
SessionStartup: onSessionFileRead 
SessionStartup: _ensureInitialized: [object Object]
SessionStartup: init starting
SessionStartup: init launched
SessionStartup: onSessionFileRead 
SessionStartup: onSessionFileRead: Initialization is already complete
SessionStartup: _ensureInitialized: [object Object]
SessionStartup: _ensureInitialized: [object Object]
SessionStartup: _ensureInitialized: null
SessionStartup: _ensureInitialized: null

As opposed to without the add-on installed:

SessionStartup: init starting
SessionStartup: init launched
SessionStartup: onSessionFileRead 
SessionStartup: _ensureInitialized: [object Object]
SessionStartup: _ensureInitialized: [object Object]
SessionStartup: _ensureInitialized: [object Object]
SessionStartup: _ensureInitialized: [object Object]
SessionStartup: _ensureInitialized: null
SessionStartup: _ensureInitialized: null
Okay I figured out what's happening.

_sessionType gets set to null in init, which is overwriting the value that was set in onSessionFileRead when an sync read is done.

It seems like an easy fix would be to simply comment out the line to set _sessionType to null since that's not a valid value anyway and it's already set to Ci.nsISessionStartup.NO_SESSION at component initialization. When I did that resuming worked fine.

If there's some reason for setting _sessionType to null, maybe a new "UNINITIALIZED" value could be added to nsISessionStartup.idl or something.
Just added the "dataloss" keyword since it causes all session data to be lost and to "squeak the wheel" since there is a potential fix to this.  

I'll wait till after David's return date before making any more updates.
Keywords: dataloss
Ok, thanks for the investigation. I double-checked and just removing this |null| should be sufficient. If you want to write the patch, I'll review it.
Flags: needinfo?(morac99-firefox2)
Attached patch Patch (v1) (obsolete) — Splinter Review
Okay, I simply took out the line setting _sessionType to null (and the associated comment).  _sessionType will get set when attempting to read it prior to initialization anyway so I don't see any downsides to this.
Assignee: nobody → morac99-firefox2
Attachment #697919 - Flags: review?(dteller)
Flags: needinfo?(morac99-firefox2)
Odd it didn't like my patch file.  I don't have the HG pull of moz-central, but I can usually "fake" this by using diff and it works. I'm going to see if I can figure out why it doesn't like it.
Attached patch Proper patchSplinter Review
Okay I created a proper hg diff patch so hopefully it likes it.
Attachment #697919 - Attachment is obsolete: true
Attachment #697919 - Flags: review?(dteller)
Attachment #697976 - Flags: review?(dteller)
Comment on attachment 697976 [details] [diff] [review]
Proper patch

Review of attachment 697976 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. Here's the Try: https://tbpl.mozilla.org/?tree=Try&rev=49610151fe4c
Attachment #697976 - Flags: review?(dteller) → review+
https://hg.mozilla.org/mozilla-central/rev/d070668a08e3
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 20
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: