Closed Bug 961861 Opened 6 years ago Closed 6 years ago

[e10s] Session store content script not always loaded

Categories

(Firefox :: Session Restore, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29

People

(Reporter: billm, Assigned: billm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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?
(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 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+
Duplicate of this bug: 961929
https://hg.mozilla.org/mozilla-central/rev/97630f83892b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
(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)
Thanks Bill. We will check tomorrow if we can re-enable our session store test over on bug 923424.
https://hg.mozilla.org/mozilla-central/rev/1652db1c136e
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.