1580766 - Expose a "session id" on CanonicalBrowsingContext, preserved when a tab's top level BC is replaced

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, task, P2)

Description

[Tomislav Jovanovic :zombie]

We need a stable ID that would represent a tab across BrowserContext getting replaced.

Comment 1

[Tomislav Jovanovic :zombie]

Wrong dependency.

Comment 2

[Chris Peterson [:cpeterson]]

Tentatively moving all bugs whose summaries mention "Fission" (or other Fission-related keywords) but are not assigned to a Fission Milestone to the "?" triage milestone.

This will generate a lot of bugmail, so you can filter your bugmail for the following UUID and delete them en masse:

0ee3c76a-bc79-4eb2-8d12-05dc0b68e732

Comment 3

[Chris Peterson [:cpeterson]]

@ Dave, are you working on this bug? kmag said you may have started working on this bug already.

Tracking this bug for Fission dogfooding (milestone M5) because kmag says people keep running into this problem with BrowsingContexts getting replaced.

Comment 5

[Dave Townsend [:mossop]]

Yes, didn't realise there was a bug already on file. Hopefully have a patch up today or tomorrow.

Comment 6

[Dave Townsend [:mossop]]

Attachment: Bug 1580766: Add a unique ID for the BrowsingContext tree inside a browser element. r=kmag

Adds a browserId property to all browsing contexts which the same for the
entire tree of contexts inside a frame element. If a new top-level context is
created for the frame then it is assigned the same value.

This allows identifying the frame element for a given browsing context.

Currently this is only done for XUL frame elements (browser/iframe). Not sure
if we want this for others.

Comment 7

[Chris Peterson [:cpeterson]]

Dave, are you still working on this browserId patch? This patch will unblock some Fission work. kmag provided some review feedback to be addressed.

Comment 8

[Dave Townsend [:mossop]]

(In reply to Chris Peterson [:cpeterson] from comment #7)

Dave, are you still working on this browserId patch? This patch will unblock some Fission work. kmag provided some review feedback to be addressed.

I will probably have time to pick this back up sometime this week. If it needs to be finished sooner feel free to find someone else to pick it up.

Comment 9

[Chris Peterson [:cpeterson]]

mossop's patch is waiting for r?kmag.

Zombie wants this feature.

Comment 10

[Chris Peterson [:cpeterson]]

Dave, what work is still needed for this session id bug? Phabricator says the review state is "Changes Planned".

Comment 11

[Dave Townsend [:mossop]]

(In reply to Chris Peterson [:cpeterson] from comment #10)

Dave, what work is still needed for this session id bug? Phabricator says the review state is "Changes Planned".

The issue is that detailed at https://phabricator.services.mozilla.com/D56245#2191328. Unfortunately this bug is no longer a priority for me as it doesn't block anything I'm working on and so I'm having difficulty finding the time to finish it. If someone else wants to take it then that would be fine.

Comment 12

[Chris Peterson [:cpeterson]]

(In reply to Dave Townsend [:mossop] (he/him) from comment #11)

If someone else wants to take it then that would be fine.

OK. I'll find another engineer to continue your work in progress.

Moving this bug to Fission Nightly M6a because Zombie says it doesn't need to block Fission dogfooding but recommends we finish it soon.

Comment 13

[Chris Peterson [:cpeterson]]

Assigning to Kashav because he volunteered to finish this bug.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/30c061da89d8
Add a unique ID for the BrowsingContext tree inside a browser element. r=kmag

Comment 15

[Razvan Maries]

Backed out for perma failures on BrowsingContext.cpp.

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=linux%2C18.04%2Cx64%2Casan%2Copt%2Cmochitests%2Ctest-linux1804-64-asan%2Fopt-mochitest-devtools-chrome-e10s-6%2Cm%28dt6%29&revision=30c061da89d8f18feeb85e593c8891f8be7ed5b1&selectedTaskRun=XuG2HDdURM-vMBkBAa7YpQ-0

Logs: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304582925&repo=autoland&lineNumber=8321
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304582937&repo=autoland&lineNumber=2331
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=304593675&repo=autoland&lineNumber=4774

Backout: https://hg.mozilla.org/integration/autoland/rev/5d2d0c265789f64157d927aec5adf3f93ec9cdd8

Comment 16

[u608768]

This is failing for the old rdm (which has been disabled in nightly, but is still the default for tests).

We want to swap the browser Ids in nsFrameLoader::SwapWithOtherRemoteLoader, but mOwnerContent is an HTMLIFrameElement for the rdm frame, and we can't get a XULFrameElement from that.

I'm not sure how long we plan to keep the old rdm around, so it might not be worth it to fix this. Maybe we can run the failing tests with the new pref? I'll get more information tomorrow.

Comment 17

[u608768]

Attachment: Bug 1580766 - Add a unique ID for the BrowsingContext tree inside a browser element. r=kmag

Depends on D56245

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3722901b6309
Add a unique ID for the BrowsingContext tree inside a browser element. r=kmag
https://hg.mozilla.org/integration/autoland/rev/6b9926a5ab88
Move BrowserId to nsFrameLoaderOwner, r=kmag

Comment 19

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1580766) for XPCshell failures in unit/test_browsing_context_structured_clone.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=305553813&repo=autoland&lineNumber=3830

Also fails on DT:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=305552308&repo=autoland&lineNumber=9791

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=6b9926a5ab889fc4d5545eda5698d529affaed2f

Backout:
https://hg.mozilla.org/integration/autoland/rev/58449fd030570f570e7c05ef77bff9c72e93ae25

Comment 20

[u608768]

The assertions added in CreateBrowsingContext didn't account for content-initiated frame adoptions. We end up in a situation where we're creating a BrowsingContext for a frame that has an Id from its previous BrowsingContext. It might be possible to detect if we're adopting and make those assertions conditional, or just reset the browser Id when we start adopting, but the easiest solution is probably just to remove the assertions.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/585e0230b2d5
Add a unique ID for the BrowsingContext tree inside a browser element. r=kmag

Comment 22

[Razvan Maries]

Backout: https://hg.mozilla.org/integration/autoland/rev/a989012952aeddfb4190b40220e7e853f4ed6384

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b75b8bc4d1ac
Add a unique ID for the BrowsingContext tree inside a browser element. r=kmag

Comment 24

[Razvan Maries]

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