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)
Tracking
()
RESOLVED
FIXED
Firefox 20
People
(Reporter: morac, Assigned: morac)
References
Details
(Keywords: dataloss, regression)
Attachments
(2 files, 1 obsolete file)
1.42 KB,
application/x-xpinstall
|
Details | |
855 bytes,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
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/
Updated•11 years ago
|
Assignee: nobody → dteller
Comment 1•11 years ago
|
||
Unfortunately, I won't have time to work on this until Jan 7th.
Updated•11 years ago
|
Assignee: dteller → nobody
Assignee | ||
Comment 2•11 years ago
|
||
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.
Assignee | ||
Comment 3•11 years ago
|
||
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
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
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
Comment 6•11 years ago
|
||
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)
Assignee | ||
Comment 7•11 years ago
|
||
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)
Assignee | ||
Comment 8•11 years ago
|
||
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.
Assignee | ||
Comment 9•11 years ago
|
||
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+
Updated•11 years ago
|
Keywords: checkin-needed
Comment 11•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d070668a08e3
Keywords: checkin-needed
Comment 12•11 years ago
|
||
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.
Description
•