1568013 - loadContentPage: non-remote browsers no longer work with e10s

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P2)

Description

[Shane Caraveo (:mixedpuppy)]

loadContentPage will now fail on moz-extension protocol if e10s is true and the browser element is not remote. This happens in local tests if you run the in-process tag on test_ext_redirects.js due to some new loadURI restrictions. It does not happen on try because we disable in-process for non-android platforms.

ExtensionXPCShellTest should probabably set REMOTE_CONTENT_SCRIPTS to the value of the autostart pref. That at least allows "in-process" tests to pass locally if you don't specify the tag, or throw some execption about not being able to run tests in this mode.

https://searchfox.org/mozilla-central/rev/4436573fa3d5c4311822090e188504c10c916c6f/toolkit/components/extensions/ExtensionXPCShellUtils.jsm#89

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1568013 set initial value of REMOTE_CONTENT_SCRIPTS to the autostart pref

Comment 2

[:Gijs (he/him)]

FWIW, I filed bug 1565545 for this. I was under the impression that I fixed the relevant tests for now by setting the escape pref that allows this on infra. Which specific tests break when run locally (how)?

(In reply to Shane Caraveo (:mixedpuppy) from comment #0)

loadContentPage will now fail on moz-extension protocol if e10s is true and the browser element is not remote. This happens in local tests if you run the in-process tag on test_ext_redirects.js due to some new loadURI restrictions.

FWIW, from what I recall it should only fail if remote webextensions are also enabled. Do we ever load moz-extension things in the parent w/ e10s and remote webextensions enabled? I didn't think so...

Comment 3

[Shane Caraveo (:mixedpuppy)]

• I'd rather not set an escape pref because now those tests will always run with that.
• browser/ tests have no need to run without e10s/remote so lets just make those tests only run that way
• toolkit tests need to run without e10s on android, the patch here allows that to happen while letting the tests pass on desktop (they would never run in this mode on try infra, just a local run issue)

This doesn't seem to be related directly to whether remote webextensions are enabled or not. It has to do with whether the browser element is remote when e10s is on.

One specific test is ./mach xpcshell-test toolkit/components/extensions/test/xpcshell//test_ext_redirects.js

But probably any use of loadContentPage in some combination with moz-extension protocol will trigger this locally.

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to :Gijs (he/him) from comment #2)

FWIW, from what I recall it should only fail if remote webextensions are also enabled.

No, this is failing with the in-process-webextension tag. In that case, extensions.remote is false.

Comment 5

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)

• I'd rather not set an escape pref because now those tests will always run with that.
  Sure.

• browser/ tests have no need to run without e10s/remote so lets just make those tests only run that way

This probably wants a follow-up to actually remove the ability + infrastructure to run those tests in-process, then?

• toolkit tests need to run without e10s on android, the patch here allows that to happen while letting the tests pass on desktop (they would never run in this mode on try infra, just a local run issue)

Right. AIUI we can then also remove the pref flips in toolkit/components/extensions/test/xpcshell/ from https://hg.mozilla.org/mozilla-central/rev/3093028bdd97d359e4c1a1d150f4733e690561c4 .

This doesn't seem to be related directly to whether remote webextensions are enabled or not. It has to do with whether the browser element is remote when e10s is on.

There's an escape clause for moz-extension loads in the parent if remote webextensions are disabled ( https://searchfox.org/mozilla-central/rev/b9041f813de0a05bf6de95a145d4e25004499517/docshell/base/nsDocShell.cpp#9562-9567 ). So I'm still not quite clear why this test breaks, off-hand...

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #5)

There's an escape clause for moz-extension loads in the parent if remote webextensions are disabled ( https://searchfox.org/mozilla-central/rev/b9041f813de0a05bf6de95a145d4e25004499517/docshell/base/nsDocShell.cpp#9562-9567 ). So I'm still not quite clear why this test breaks, off-hand...

Oh, it seems the test depends on loading http(s) resources in the parent in addition to moz-extension. Yeah, that's not going to work.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7e3c94531962
set initial value of REMOTE_CONTENT_SCRIPTS to the autostart pref r=zombie

Comment 8

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/7e3c94531962

Comment 9

[Alex Cornestean]

Hello,

Will this fix require manual validation? If yes, please provide some steps to reproduce in order to correctly test it and also, please set the "qe-verify+" flag. Otherwise, could the "qe-verify-" flag be added? Thanks!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9079861 [details]
Bug 1568013 set initial value of REMOTE_CONTENT_SCRIPTS to the autostart pref

Per https://bugzilla.mozilla.org/show_bug.cgi?id=1560178#c34.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/c29c5aea7838