961861 - [e10s] Session store content script not always loaded

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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?

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: message-manager-fix

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

(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

[Tim Taubert [:ttaubert] (inactive)]

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."?

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/97630f83892b

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/97630f83892b

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks Bill. We will check tomorrow if we can re-enable our session store test over on bug 923424.

Comment 11

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1652db1c136e

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1652db1c136e