1599579 - [meta] Make all moz-extension: loads COOP loads, and place them in separate BrowsingContextGroups

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[Kris Maglione [:kmag]]

There are probably a lot of reasons to force extension pages to use COOP, but the most pressing is that all privileged extension pages can directly access any other privileged pages from the same extension. The problem with this is that we also have an invariant that any two content pages which can directly access each other must be in the same BrowsingContextGroup, which in the case of extension pages, does not actually hold.

The only way to fix the problem is to force the BrowsingContext for any extension page that we create into the same BrowsingContextGroup as any other pages that it can access. And in the cases where the page isn't the first thing loaded into a FrameLoader, the only way to do that is to completely replace any existing BrowsingContext with a new one, in the appropriate group. This is essentially the same behavior required for COOP, just with the added complexity of also controlling the group of that BrowsingContext.

Comment 1

[Chris Peterson [:cpeterson]]

Moving P2 M5 bugs to M5b milestone

Comment 2

[Chris Peterson [:cpeterson]]

M5c

Morphing this bug into a meta bug because Nika has WIP patches for part of this bug. kmag will file new blocking bugs for the other parts.

Comment 3

[Chris Peterson [:cpeterson]]

Nika recommends unassigning kmag for now. We should fix this bug for M5 Dogfooding, but it's not the most urgent bug.

Comment 4

[Chris Peterson [:cpeterson]]

Nika says 99% of this is implemented.
Affects non-Fission, too.
Every extension should have its own BrowsingContextGroup.

Comment 5

[Chris Peterson [:cpeterson]]

kmag says this bug doesn’t need to block Fission M5 Dogfooding because this bug affects non-Fission users, too.

Comment 6

[Neha Kochar [:neha]]

kmag said Nika is taking over this one.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1599579 - Part 1: Add the ability to specify a specific BrowsingContextGroup during process switch,

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1599579 - Part 2: Reorganize RemoteType logic in MaybeTriggerProcessSwitch,

The goal with this is to avoid having multiple booleans and other values
computed in arbitrary places and used around the method, and instead pre-compute
common shared information, and group each remoteType special-case together.

Hopefully, this should make it easier to extend the behaviour in
MaybeTriggerProcessSwitch in the future.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1599579 - Part 3: Load toplevel extension frames into a per-extension BrowsingContextGroup,

This is done by tracking a specific BrowsingContextGroup ID on the
WebExtensionPolicy in the parent process. Whenever a load is done with that
policy, the browsing context is replaced to ensure it is loaded in the correct
BrowsingContextGroup.

This patch also ensures that extension iframes are always loaded in the same
process as their embedder document, even if the frame was previously remote.

Comment 10

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1599579 - Part 4: Expose BrowsingContextGroup.id to JS,

Comment 11

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1599579 - Part 5: Ensure all views have matching BrowsingContextGroup IDs in getViews test,

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ebd2eb155da
Part 1: Add the ability to specify a specific BrowsingContextGroup during process switch, r=kmag
https://hg.mozilla.org/integration/autoland/rev/a4e7334f8ce6
Part 2: Reorganize RemoteType logic in MaybeTriggerProcessSwitch, r=mattwoodrow,necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/085558fe56dc
Part 3: Load toplevel extension frames into a per-extension BrowsingContextGroup, r=kmag
https://hg.mozilla.org/integration/autoland/rev/c2828aec4caf
Part 4: Expose BrowsingContextGroup.id to JS, r=kmag
https://hg.mozilla.org/integration/autoland/rev/053229a30ef1
Part 5: Ensure all views have matching BrowsingContextGroup IDs in getViews test, r=kmag

Comment 13

[Narcis Beleuzu [:NarcisB]]

Backed out for twinopen failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/b1146cce5053bf690a8b19f8da7ca97b1f37438d
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=307151935&repo=autoland&lineNumber=2308

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1599579 - Part 6: Handle process switches in talos pageloader.js,

This is done by reloading framescripts and re-attaching message listeners when
process switch events are fired.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b4bdd8759433
Part 1: Add the ability to specify a specific BrowsingContextGroup during process switch, r=kmag
https://hg.mozilla.org/integration/autoland/rev/14698ab42ee4
Part 2: Reorganize RemoteType logic in MaybeTriggerProcessSwitch, r=mattwoodrow,necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/73f6c956fc30
Part 3: Load toplevel extension frames into a per-extension BrowsingContextGroup, r=kmag
https://hg.mozilla.org/integration/autoland/rev/cfdd8d5973d4
Part 4: Expose BrowsingContextGroup.id to JS, r=kmag
https://hg.mozilla.org/integration/autoland/rev/c5bc0ea42193
Part 5: Ensure all views have matching BrowsingContextGroup IDs in getViews test, r=kmag
https://hg.mozilla.org/integration/autoland/rev/b767f26b10ed
Part 6: Handle process switches in talos pageloader.js, r=mconley,perftest-reviewers,Bebe

Comment 16

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/99571b1367d3
Backed out 6 changesets for bc failures at test_chrome_ext_contentscript_data_uri.html.

Comment 17

[Atila Butkovits]

Backed outfor bc failures at test_chrome_ext_contentscript_data_uri.html.

Backout link: https://hg.mozilla.org/integration/autoland/rev/99571b1367d3474c2aef37d99e9408243f34585c

Push with failure:https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=b767f26b10ed8286756fb17c3c0306823bb22f59&selectedTaskRun=BIqRZvt_RDWwR7TtZHulwQ.1&searchStr=Linux%2C18.04%2Cx64%2Casan%2Copt%2CMochitests%2Cwithout%2Ce10s%2Ctest-linux1804-64-asan%2Fopt-mochitest-chrome-1proc-2%2CM-1proc%28c2%29

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308057987&repo=autoland&lineNumber=112496

Comment 18

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1599579 - Part 7: Fix failing contentscript test,

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ae775aac9f7
Part 1: Add the ability to specify a specific BrowsingContextGroup during process switch, r=kmag
https://hg.mozilla.org/integration/autoland/rev/e6252187b0a8
Part 2: Reorganize RemoteType logic in MaybeTriggerProcessSwitch, r=mattwoodrow,necko-reviewers,valentin
https://hg.mozilla.org/integration/autoland/rev/871123e639a8
Part 3: Load toplevel extension frames into a per-extension BrowsingContextGroup, r=kmag
https://hg.mozilla.org/integration/autoland/rev/4c1d7b36f035
Part 4: Expose BrowsingContextGroup.id to JS, r=kmag
https://hg.mozilla.org/integration/autoland/rev/a89a0e4bb31e
Part 5: Ensure all views have matching BrowsingContextGroup IDs in getViews test, r=kmag
https://hg.mozilla.org/integration/autoland/rev/f817fd7a0a6d
Part 6: Handle process switches in talos pageloader.js, r=mconley,perftest-reviewers,Bebe
https://hg.mozilla.org/integration/autoland/rev/2e058695addc
Part 7: Fix failing contentscript test, r=kmag

Comment 20

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/8ae775aac9f7
https://hg.mozilla.org/mozilla-central/rev/e6252187b0a8
https://hg.mozilla.org/mozilla-central/rev/871123e639a8
https://hg.mozilla.org/mozilla-central/rev/4c1d7b36f035
https://hg.mozilla.org/mozilla-central/rev/a89a0e4bb31e
https://hg.mozilla.org/mozilla-central/rev/f817fd7a0a6d
https://hg.mozilla.org/mozilla-central/rev/2e058695addc

Comment 21

[Shane Hughes [:aminomancer]]

can we do something like a webextensions permission to exempt an extension so it doesn't break the urlbar focus on custom NTPs