1072198 - [e10s] updateBrowserRemoteness() assigns a new .permanentKey to browsers

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

Turns out that listening for "oop-browser-crashed" in SessionStore and then adding |browser.permanentKey| to a WeakSet didn't work because |updateBrowserRemoteness()| assigns a new permanentKey. I assume that tearing down the <browser> and re-inserting into the tree somehow causes the field to be re-initialized.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: 0001-Bug-1072198-updateBrowserRemoteness-needs-to-keep-.p.patch

Comment 2

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

Comment on attachment 8494411 [details] [diff] [review]
0001-Bug-1072198-updateBrowserRemoteness-needs-to-keep-.p.patch

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

::: browser/base/content/tabbrowser.xml
@@ +1461,5 @@
>              parent.removeChild(aBrowser);
>              aBrowser.setAttribute("remote", aShouldBeRemote ? "true" : "false");
> +            // Tearing down the browser gives a new permanentKey but we want to
> +            // keep the old one. Re-set it explicitly after unbinding from DOM.
> +            aBrowser.permanentKey = permanentKey;

I'm actually surprised this works. I would have thought that we'd have to change the permanentKey after adding the element back into the DOM. I guess this is fine if it works, though.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Yeah, me too. But it does. That reminds me I should add a test to ensure that :)

I did that explicitly before putting it back into the DOM to have a correct .permanentKey before the first setupSyncHandler message arrives, which I believe would be synchronous at least with an InProcessTabChildGlobal.

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/e16c4c4eb07a

Extended browser_e10s_switchbrowser.js to ensure this works and doesn't break in the future. Ensured locally that the test does indeed break without the patch applied.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e16c4c4eb07a