Closed
Bug 961861
Opened 10 years ago
Closed 10 years ago
[e10s] Session store content script not always loaded
Categories
(Firefox :: Session Restore, defect)
Tracking
()
RESOLVED
FIXED
Firefox 29
People
(Reporter: billm, Assigned: billm)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
4.92 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
While playing with session restore in electrolysis, I discovered that the content-sessionStore.js file isn't always getting loaded into every tab. Normally the script is loaded in onTabAdd with the delayed parameter set to false. However, we have a crazy function in e10s that removes a <browser> element and adds it back again with a different value for the remote attribute: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#1341 When _updateBrowserRemoteness is called, the session restore content script won't be loaded back into the <browser> element. This patch fixes the problem by using the window's message manager to load content scripts with delayed = true. I think this is a lot more reliable. We used to load the script onto the window, but I moved the load to onTabAdd because that's when the message listeners were added. This patch adds the message listeners to the window as well, which I think is more robust. One issue with this patch is that now we'll be attaching the content script to <browser> elements that aren't tabs (although only ones added to a normal Firefox window). This should be uncommon I think--some <iframe mozbrowser>s during tests, and maybe some add-ons? In any case, I added code so that we ignore messages from <browser>s that aren't tabs. Eventually I think it would be nice if the frontend could create its own hierarchy of message managers. That way we could, for example, create a message manager for just the tabs in a window, and then another message manager for all tabs in Firefox. What do you think of the idea, Tim?
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8362709 -
Flags: review?(ttaubert)
Comment 2•10 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #0) > Eventually I think it would be nice if the frontend could create its own > hierarchy of message managers. That way we could, for example, create a > message manager for just the tabs in a window, and then another message > manager for all tabs in Firefox. What do you think of the idea, Tim? That indeed sounds like a fancy thing to have :)
Comment 3•10 years ago
|
||
Comment on attachment 8362709 [details] [diff] [review] message-manager-fix Review of attachment 8362709 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks! ::: browser/components/sessionstore/src/SessionStore.jsm @@ +610,5 @@ > var browser = aMessage.target; > var win = browser.ownerDocument.defaultView; > + let tab = this._getTabForBrowser(browser); > + if (!tab) { > + // Ignore messages from <browser> elements that are not tabs. Nit: maybe "... that aren't assigned to tabs."?
Attachment #8362709 -
Flags: review?(ttaubert) → review+
Assignee | ||
Comment 5•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/97630f83892b
https://hg.mozilla.org/mozilla-central/rev/97630f83892b
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
Assignee | ||
Comment 7•10 years ago
|
||
I backed this out because of bug 962767, which looks like it will be nontrivial to fix. https://hg.mozilla.org/mozilla-central/rev/b9d9649e7ec0
Assignee | ||
Updated•10 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 8•10 years ago
|
||
Bill, on bug 923424 I also experience problems with SessionStore while running Mozmill tests. I found the following stack and I wonder if it is the same underlying cause as for this bug. But keep in mind that we do not run Firefox with remote tabs. console.error: Message: TypeError: docShell is null Stack: SessionHistoryInternal.collect@resource://app/modules/sessionstore/SessionHistory.jsm:48 this.SessionHistory<.collect@resource://app/modules/sessionstore/SessionHistory.jsm:28 SyncHandler.collectSessionHistory@chrome://browser/content/content-sessionStore.js:200 TabStateInternal._collectSyncUncached@resource://app/modules/sessionstore/TabState.jsm:329 TabStateInternal.collectSync@resource://app/modules/sessionstore/TabState.jsm:242 this.TabState<.collectSync@resource://app/modules/sessionstore/TabState.jsm:57 ssi_onTabClose@resource:///modules/sessionstore/SessionStore.jsm:1417 ssi_handleEvent@resource:///modules/sessionstore/SessionStore.jsm:765
Status: REOPENED → ASSIGNED
Flags: needinfo?(wmccloskey)
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #8) > Bill, on bug 923424 I also experience problems with SessionStore while > running Mozmill tests. I found the following stack and I wonder if it is the > same underlying cause as for this bug. But keep in mind that we do not run > Firefox with remote tabs. Yes, this is the same problem. It should have been fixed by the backout.
Flags: needinfo?(wmccloskey)
Comment 10•10 years ago
|
||
Thanks Bill. We will check tomorrow if we can re-enable our session store test over on bug 923424.
Assignee | ||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1652db1c136e
Comment 12•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1652db1c136e
Status: ASSIGNED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•