Closed Bug 1072198 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set
normal
Points:
2

Tracking

()

RESOLVED FIXED
Firefox 35
Iteration:
35.2
Tracking Status
e10s m3+ ---

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

Attachments

(1 file)

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.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8494411 - Flags: review?(wmccloskey)
Component: General → Tabbed Browser
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.
Attachment #8494411 - Flags: review?(wmccloskey) → review+
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.
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.
Iteration: --- → 35.2
Points: --- → 2
Flags: qe-verify-
Flags: in-testsuite+
Flags: firefox-backlog+
https://hg.mozilla.org/mozilla-central/rev/e16c4c4eb07a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.