2011326 - Deduplicate process selection logic between E10SUtils.sys.mjs and ProcessIsolation.cpp

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, enhancement)

Description

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

Currently there are 2 distinct pieces of code which are generally implementing roughly the same decision making for process selection - E10SUtils.sys.mjs is used to predict the remoteType to use for a <browser> element as it is being added to the DOM, while ProcessIsolation is used to actually perform the process isolation during navigations.

This bug is to track removing the separate E10SUtils.sys.mjs logic, instead exposing a similar interface from the ProcessIsolation.cpp code, such that the backend can be shared and future changes to the process isolation logic don't need to be replicated in both code paths.

Comment 1

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

Attachment: Bug 2011326 - Part 1: Remove test for separatePrivilegedContentProcess=false, r=#firefox-desktop-core-reviewers!

As noted by bug 2011309, the browser is quite broken if it is disabled already,
and the prev should be removed entirely. This also removes the slighly
confusing comment, which seems to no longer be relevant (likely due to changes
in how the flag is implemented as it became a stronger requirement).

Comment 2

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

Attachment: Bug 2011326 - Part 2: Predict remote types with ProcessIsolation, r=smaug!,#firefox-desktop-core-reviewers!

This replaces the existing remote type prediction backend, which has been
implemented in E10SUtils.sys.mjs. This completes the transition which was
started when process selection decisions started being made in C++ for Fission
by removing the core logic completely from E10SUtils.sys.mjs.

Calls in JS are still done through a wrapped in E10SUtils.sys.mjs, because they
benefit from fuzzy behaviour which has been implemented in that wrapper.
Implementing this kind of argument punning directly in C++ would have been more
verbose, and likely trickier to maintain.

Comment 3

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

Attachment: Bug 2011326 - Part 3: Remove the test-only canLoadURIInRemoteType function, r=#firefox-desktop-core-reviewers!

This function is somewhat redundant nowadays, and is only used by tests, so
moving the logic out of E10SUtils and into the tests may make more sense.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c1a0f5aee3f3
https://hg.mozilla.org/integration/autoland/rev/7edc95201888
Part 1: Remove test for separatePrivilegedContentProcess=false, r=firefox-desktop-core-reviewers ,tabbrowser-reviewers,mossop,sthompson
https://github.com/mozilla-firefox/firefox/commit/b0d8bab44685
https://hg.mozilla.org/integration/autoland/rev/b3818f5deac2
Part 2: Predict remote types with ProcessIsolation, r=whimboo,geckoview-reviewers,extension-reviewers,tabbrowser-reviewers,omc-reviewers,layout-reviewers,tcampbell,robwu,sthompson,emilio,mimi
https://github.com/mozilla-firefox/firefox/commit/b0ff387a8bf6
https://hg.mozilla.org/integration/autoland/rev/55c22e5d6c6e
Part 3: Remove the test-only canLoadURIInRemoteType function, r=firefox-desktop-core-reviewers ,tabbrowser-reviewers,mossop,sthompson

Comment 5

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/7edc95201888
https://hg.mozilla.org/mozilla-central/rev/b3818f5deac2
https://hg.mozilla.org/mozilla-central/rev/55c22e5d6c6e

Comment 6

[Magnus Melin [:mkmelin]]

Attachment: WIP: Port Bug 2011326 - Deduplicate process selection logic between E10SUtils.sys.mjs and ProcessIsolation.cpp. r=#thunderbird-reviewers

Comment 7

[Phabricator Automation]

Comment on attachment 9584100 [details]
WIP: Port Bug 2011326 - Deduplicate process selection logic between E10SUtils.sys.mjs and ProcessIsolation.cpp. r=#thunderbird-reviewers

Revision D299090 was moved to bug 2037815. Setting attachment 9584100 [details] to obsolete.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c2c6c88166a6
https://hg.mozilla.org/integration/autoland/rev/b57eca363331
Revert "Bug 2011326, Bug 2011307 - Part 3: Remove the test-only canLoadURIInRemoteType function, r=firefox-desktop-core-reviewers ,tabbrowser-reviewers,mossop,sthompson" for causing xpc failures at test_sitepolicies.js

Comment 9

[rperta]

Backed out for causing xpc failures at test_sitepolicies.js
Backout link
Push with failures
Failure log(s)

Comment 10

[pstanciu]

Backout merged to central link

Comment 11

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

Attachment: Bug 2011326 - Part 4: Switch test_sitepolicies to a data URI, r=#firefox-desktop-core-reviewers!

We never actually directly load moz-nullprincipal URIs, so this new
approach is more representative of the actual way we end up exercising
this code.

This fixes an inaccuracy in the test where the test was asserting that
we do isolate null principal URIs in such a way that we would disable
the JIT, but that is not the case - before the changes in bug 1729352 we
had no mechanism for making a generic "web" content process have
disableJit specified.

I am updating bug 1729352 to also remove this special case which was
added to allow the test to pass.

Comment 12

[agoloman]

Backed out for causing bc failures @browser_aboutURLs.js.

• Backout link
• Push with failures
• Failure Log
• Failure log @ContentParent.cpp

Comment 13

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

Huh I have no idea how I missed this particular failure in my try pushes. Should be fixed now, but I'm doing another try push before I land again. https://treeherder.mozilla.org/jobs?repo=try&landoInstance=lando-prod-2025&landoCommitID=48932

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e89827b3fe9a
https://hg.mozilla.org/integration/autoland/rev/e5e4ac6640d2
Part 1: Remove test for separatePrivilegedContentProcess=false, r=firefox-desktop-core-reviewers ,tabbrowser-reviewers,mossop,sthompson
https://github.com/mozilla-firefox/firefox/commit/d147446fc76a
https://hg.mozilla.org/integration/autoland/rev/0a7ec537da9c
Part 2: Predict remote types with ProcessIsolation, r=whimboo,geckoview-reviewers,extension-reviewers,tabbrowser-reviewers,omc-reviewers,layout-reviewers,tcampbell,robwu,sthompson,emilio,mimi
https://github.com/mozilla-firefox/firefox/commit/090bbd76980e
https://hg.mozilla.org/integration/autoland/rev/e4f9a6650fdd
Part 3: Remove the test-only canLoadURIInRemoteType function, r=firefox-desktop-core-reviewers ,tabbrowser-reviewers,mossop,sthompson
https://github.com/mozilla-firefox/firefox/commit/08fe0809f008
https://hg.mozilla.org/integration/autoland/rev/3c9cfbb02363
Part 4: Switch test_sitepolicies to a data URI, r=mossop

Comment 15

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/e5e4ac6640d2
https://hg.mozilla.org/mozilla-central/rev/0a7ec537da9c
https://hg.mozilla.org/mozilla-central/rev/e4f9a6650fdd
https://hg.mozilla.org/mozilla-central/rev/3c9cfbb02363

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/879157d4f751
https://hg.mozilla.org/releases/mozilla-beta/rev/bce4a7fec37c
Part 1: Remove test for separatePrivilegedContentProcess=false, r=firefox-desktop-core-reviewers ,tabbrowser-reviewers,mossop,sthompson
https://github.com/mozilla-firefox/firefox/commit/df59bd116ee4
https://hg.mozilla.org/releases/mozilla-beta/rev/cf9f510157ab
Part 2: Predict remote types with ProcessIsolation, r=whimboo,geckoview-reviewers,extension-reviewers,tabbrowser-reviewers,omc-reviewers,layout-reviewers,tcampbell,robwu,sthompson,emilio,mimi
https://github.com/mozilla-firefox/firefox/commit/8ce4078eed63
https://hg.mozilla.org/releases/mozilla-beta/rev/dde01337e91a
Part 3: Remove the test-only canLoadURIInRemoteType function, r=firefox-desktop-core-reviewers ,tabbrowser-reviewers,mossop,sthompson
https://github.com/mozilla-firefox/firefox/commit/8eab4714b1e5
https://hg.mozilla.org/releases/mozilla-beta/rev/529da692fb3c
Part 4: Switch test_sitepolicies to a data URI, r=mossop