1393150 - require e10s if webextension.remote=true

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

[Sören Hentzschel]

A user of my add-on New Tab Override (WebExtension) reported that it does not work on Firefox 56 Beta. I was not able to reproduce the problem, it works for me with Firefox Stable, Beta and Nightly. But the really interesting thing is the console output of the user's Firefox:

> Attempt to create privileged extension parent from incorrect child process (ExtensionParent.jsm:646)

and

> Error: WebExtension context not found! (ExtensionParent.jsm:778:13)

I got no other report so it seems that only one user is affected not all of my users. But how is this error possible?

Comment 1

[Sören Hentzschel]

Oh, I can reproduce now with extensions.webextensions.remote = true. I am using macOS and I know it's not yet a supported configuration on macOS but the report came from a Windows user with default settings!

Comment 2

[Andrew Swan [:aswan]]

Attachment: Bug 1393150 Fix isParentProcess() logic

Review commit: https://reviewboard.mozilla.org/r/172010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/172010/

Comment 3

[Andrew Swan [:aswan]]

I think the attached patch fixes this, but I don't have the cycles to write a test for it right now.
Up for grabs for anybody who wants to pick it up.

Comment 4

[Andrew Swan [:aswan]]

I think this is important, but I don't have the cycles to finish it in the next few weeks.

Comment 5

[Sören Hentzschel]

Are OOP WebExtensions no longer planned for Firefox 56? It affects a lot of users (New Tab Override has already more than 100,000 users and I am sure other add-ons are also affected), so it should really be fixed in the next few days, because it's already late in the Firefox 56 beta cycle. I don't think it's an option to ship Firefox 56 with broken add-on support…

Comment 6

[Shane Caraveo (:mixedpuppy)]

I'm unable to reproduce on 57, but am able to on 56.  I'll see if the patch has any affect on the beta channel.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Strange.  Now I cannot reproduce in 56 in either my build (unpatched) or in the beta install I used before.  I also do not get the "WebExtension context not found" any longer, I did before.  I verified and re-verified with remote as false and true.  Tried with old existing profiles and new profiles.

My guess: my install updated and I didn't notice.  A recent change that was uplifted to beta fixed the problem.

Sören, can you try again with a nightly and beta build tomorrow?

Comment 8

[Shane Caraveo (:mixedpuppy)]

56b7 has a fix from Bug 1390346 that could be related, but it seems like an opposite of what is happening.  Actually, what is the addon doing?  It seems like it might be loading a moz-ext file then redirecting to whatever is configured.  It would be good to know for a test case if necessary.

Comment 9

[Sören Hentzschel]

> My guess: my install updated and I didn't notice.  A recent change that was uplifted to beta fixed the problem.
> Sören, can you try again with a nightly and beta build tomorrow?

Seems you're right! I was able to reproduce the issue before updating to the current Beta (Beta 5 was installed) and it's no longer reproducible with the current Beta 9.

Comment 10

[Shane Caraveo (:mixedpuppy)]

Ok! We'll just close this then.  Glad the problem is solved.

Comment 11

[Yanfang Liu]

Hi, this bug still can be reproduced with 'e10s disabled' and 'extensions.webextensions.remote = true' on Fx56b9. So I reopen this bug. Pls help to fix the problem, thanks!

Comment 12

[Sören Hentzschel]

I got another report which confirms that: it still does not work with e10s disabled.

So the question is: is e10s disabled + OOP WebExtensions enabled a supported configuration? If yes, there is still something to fix for Firefox 56. If no, OOP WebExtensions should not be enabled if e10s is disabled.

Comment 14

[Andy McKay]

By Firefox 57, e10s will be the only supported way of running Firefox. With Firefox 55 and 56 there's still scenarios where you can have e10s off.

If e10s is disabled, we should set remote to false for those scenarios.

Comment 15

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1393150 prevent remote extensions when e10s is off,

Review commit: https://reviewboard.mozilla.org/r/177480/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/177480/

Comment 16

[Shane Caraveo (:mixedpuppy)]

STR:

- ensure webextensions.remote=true in about:config
- install New Tab Override (WebExtension) from AMO
- click browser action button and configure newtab page
- try to open new tab (should show page you configured)
- in about:preferences uncheck "Enable multi-process ..." and restart
- try to open new tab

expect: the new tab after restart is correct

actual: the new tab is blank and on a moz-extension url

Comment 17

[Kris Maglione [:kmag]]

Comment on attachment 8905685 [details]
Bug 1393150 prevent remote extensions when e10s is off,

https://reviewboard.mozilla.org/r/177480/#review182538

::: toolkit/components/extensions/Extension.jsm:90
(Diff revision 1)
>                                        "extensions.webextensions.remote", false);
>  
> +XPCOMUtils.defineLazyGetter(
> +  this, "useRemoteWebExtensions",
> +  () => Services.appinfo.browserTabsRemoteAutostart &&
> +        isRemote);

This needs to be handled in `ExtensionPolicyService.cpp`, too. I'd rather we not reproduce this logic in multiple palces, so we should probably just check `remote = !WebExtensionPolicy.isExtensionProcess` in the parent.

Comment 18

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8905685 [details]
Bug 1393150 prevent remote extensions when e10s is off,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177480/diff/1-2/

Comment 19

[Kris Maglione [:kmag]]

Comment on attachment 8905685 [details]
Bug 1393150 prevent remote extensions when e10s is off,

https://reviewboard.mozilla.org/r/177480/#review182914

r=me with nits fixed, but this will also need review by a DOM peer for the WebIDL changes.

::: browser/modules/E10SUtils.jsm:14
(Diff revision 2)
>  XPCOMUtils.defineLazyPreferenceGetter(this, "useRemoteWebExtensions",
>                                        "extensions.webextensions.remote", false);

Thsi can be removed now.

::: toolkit/components/extensions/Extension.jsm:867
(Diff revision 2)
>  
>      if (["ADDON_UPGRADE", "ADDON_DOWNGRADE"].includes(startupReason)) {
>        StartupCache.clearAddonData(addonData.id);
>      }
>  
> -    this.remote = useRemoteWebExtensions;
> +    this.remote = WebExtensionPolicy.useRemoteWebExtensions;

Please use `!WebExtensionPolicy.isExtensionProcess` for this. What we care about here is that the extension is not running in this process, not that remote extensions are enabled.

Comment 20

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8905685 [details]
Bug 1393150 prevent remote extensions when e10s is off,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177480/diff/2-3/

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8905685 [details]
Bug 1393150 prevent remote extensions when e10s is off,

https://reviewboard.mozilla.org/r/177480/#review185138

r=me on the webidl bits, with the comment below addressed.

::: dom/webidl/WebExtensionPolicy.webidl:86
(Diff revision 3)
>     */
>    [Affects=Everything, SetterThrows]
>    attribute boolean active;
>  
>  
> +  static readonly attribute boolean useRemoteWebExtensions;

This could really use some documentation.

Comment 23

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8905685 [details]
Bug 1393150 prevent remote extensions when e10s is off,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177480/diff/3-4/

Comment 24

[Shane Caraveo (:mixedpuppy)]

Luca, there's a test failure in devtools/server/tests/mochitest/test_webextension-addon-debugging-connect.html with this patch.  I'm not familiar with the debugger isOOP stuff so not sure where to begin.

Comment 25

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8905685 [details]
Bug 1393150 prevent remote extensions when e10s is off,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177480/diff/4-5/

Comment 26

[Luca Greco [:rpl] [:luca] [:lgreco]]

As briefly discussed over IRC, that test file should only run in e10s mode because it is switching between remote webextension and non-remote webextensions, it was able to pass (by mistake) in "non e10s mode" thanks to the bug that we are fixing with the patch attached to this issue,
the failure is basically a further proof that the patch is working correctly.

Let's add a "skip-if = !e10s" in the chrome.ini with a comment that mention this bug number.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eae22389b267
prevent remote extensions when e10s is off, r=bz,kmag

Comment 28

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8905685 [details]
Bug 1393150 prevent remote extensions when e10s is off,

Approval Request Comment
[Feature/Bug causing the regression]: e10s (turned off)
[User impact if declined]: if remote extensions turned on and e10s is off, things stop working.
[Is this code covered by automated tests?]: a test that broke the rule above had to be disabled for !e10s.
[Has the fix been verified in Nightly?]: not yet, early request
[Needs manual test from QE? If yes, steps to reproduce]: good STR in comment 16
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: moderate as any late change
[Why is the change risky/not risky?]: new webidl binding, c++ code changed, but overall this should prevent worse stuff.
[String changes made/needed]: none

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

Comment 30

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5bc7f6a68a706bca73310f59e73d644b5571bd70

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

Andrei, can your team try to verify the fix in Nightly? Meanwhile we will go ahead with the uplift.

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8905685 [details]
Bug 1393150 prevent remote extensions when e10s is off,

Please uplift to m-r, we don't want users who may have turned off e10s-multi by hand to have their web extensions break. 
We should verify this fix before 56 release

Comment 33

[Shane Caraveo (:mixedpuppy)]

Please block uplift on bug 1400391

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8905685 [details]
Bug 1393150 prevent remote extensions when e10s is off,

Resetting the approval to wait on the test fix

Comment 35

[Andrei Vaida [:avaida]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #31)
> Andrei, can your team try to verify the fix in Nightly? Meanwhile we will go
> ahead with the uplift.

This request will be picked up by the Add-ons QA team. Vasilica will take point.

Comment 36

[Vasilica Tamas, QA [:vtamas]]

Attachment: Animation.gif

I was able to reproduce the initial issue on Firefox 57.0a1 (2017-08-23) under Windows 10 64-bit.
Verified as fixed on Firefox 57.0a1 ( 20170917220255) under Windows 10 64-bit and Mac OS X 10.12.3. The webextension is working as expected with e10s disabled + OOP WebExtensions enabled. See screencast.

Comment 37

[Vasilica Tamas, QA [:vtamas]]

I also noticed the following error in Browser Console while selecting about:blank, about:home, background color, Mozilla News (German) options:

TypeError: Argument 1 of StructuredCloneHolder.deserialize is not an object.[Learn More]  ExtensionChild.jsm:827:24

Is this something that we should be concerned of?

Comment 38

[Sören Hentzschel]

It's probably bug 1389734.

Comment 39

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8905685 [details]
Bug 1393150 prevent remote extensions when e10s is off,

Now that we have the fix for bug 1400391, let's uplift this to m-r
We don't want extensions to launch in e10s mode when a user has turned the main e10s-multi pref off.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/653105928c28 (FIREFOX_56b13_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/24caabac22e9

Comment 41

[Shane Caraveo (:mixedpuppy)]

I don't believe comment 37 is related to this.  See bug 1389734.

Comment 42

[Vasilica Tamas, QA [:vtamas]]

Attachment: Animation-Beta.gif

I confirm that this issue is fixed also on Firefox 56.0 RC build 4 (20170922200134) under Windows 10 64-bit and Mac OS X 10.12.3. The webextension is working as expected with e10s disabled and OOP WebExtensions enabled. See screencast.