require e10s if webextension.remote=true

VERIFIED FIXED in Firefox 56

Status

WebExtensions
General
P1
normal
VERIFIED FIXED
11 months ago
a month ago

People

(Reporter: Sören Hentzschel, Assigned: mixedpuppy)

Tracking

(Blocks: 1 bug)

Trunk
mozilla57
Dependency tree / graph

Firefox Tracking Flags

(firefox56 verified, firefox57 verified)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

11 months ago
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?
(Reporter)

Comment 1

11 months ago
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!
Blocks: 1190679
Summary: WebExtension context not found → webext-oop: WebExtension context not found
Comment hidden (mozreview-request)
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.
(Reporter)

Updated

11 months ago
status-firefox56: --- → affected

Updated

11 months ago
Assignee: nobody → aswan
Priority: -- → P2
I think this is important, but I don't have the cycles to finish it in the next few weeks.
Assignee: aswan → nobody
(Reporter)

Comment 5

11 months ago
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…
(Assignee)

Comment 6

11 months ago
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.
(Assignee)

Comment 7

11 months ago
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?
Flags: needinfo?(cadeyrn)
(Assignee)

Comment 8

11 months ago
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.
(Reporter)

Comment 9

11 months ago
> 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.
Flags: needinfo?(cadeyrn)
(Assignee)

Comment 10

11 months ago
Ok! We'll just close this then.  Glad the problem is solved.
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → WORKSFORME

Comment 11

11 months ago
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!
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
(Reporter)

Comment 12

11 months ago
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.
Duplicate of this bug: 1394807

Comment 14

11 months ago
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.

Updated

11 months ago
Priority: P2 → P1

Updated

11 months ago
Assignee: nobody → mixedpuppy
Comment hidden (mozreview-request)
(Assignee)

Comment 16

11 months ago
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

11 months ago
mozreview-review
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.
Attachment #8905685 - Flags: review?(kmaglione+bmo)
Comment hidden (mozreview-request)

Comment 19

11 months ago
mozreview-review
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.
Attachment #8905685 - Flags: review?(kmaglione+bmo) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Attachment #8905685 - Flags: review?(wmccloskey)

Updated

10 months ago
Duplicate of this bug: 1398956
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.
Attachment #8905685 - Flags: review+
(Assignee)

Updated

10 months ago
Attachment #8905685 - Flags: review?(wmccloskey)
Comment hidden (mozreview-request)
(Assignee)

Comment 24

10 months ago
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 hidden (mozreview-request)

Comment 26

10 months ago
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

10 months ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/eae22389b267
prevent remote extensions when e10s is off, r=bz,kmag
(Assignee)

Updated

10 months ago
Summary: webext-oop: WebExtension context not found → require e10s if webextension.remote=true
(Assignee)

Comment 28

10 months ago
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
Attachment #8905685 - Flags: approval-mozilla-release?

Comment 29

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/eae22389b267
Status: REOPENED → RESOLVED
Last Resolved: 11 months ago10 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Depends on: 1400391
Andrei, can your team try to verify the fix in Nightly? Meanwhile we will go ahead with the uplift.
Flags: needinfo?(andrei.vaida)
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
Attachment #8905685 - Flags: approval-mozilla-release? → approval-mozilla-release+
(Assignee)

Comment 33

10 months ago
Please block uplift on bug 1400391
Comment on attachment 8905685 [details]
Bug 1393150 prevent remote extensions when e10s is off,

Resetting the approval to wait on the test fix
Attachment #8905685 - Flags: approval-mozilla-release+ → approval-mozilla-release?
(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.
Flags: needinfo?(andrei.vaida) → needinfo?(vasilica.mihasca)
Created attachment 8909249 [details]
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.
Flags: needinfo?(vasilica.mihasca)
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
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?
Flags: needinfo?(mixedpuppy)
(Reporter)

Comment 38

10 months ago
It's probably bug 1389734.
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.
Attachment #8905685 - Flags: approval-mozilla-release? → approval-mozilla-release+
(Assignee)

Comment 41

10 months ago
I don't believe comment 37 is related to this.  See bug 1389734.
Flags: needinfo?(mixedpuppy)
Created attachment 8911738 [details]
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.

Updated

4 months ago
Depends on: 1446655

Updated

4 months ago
No longer depends on: 1446655

Updated

a month ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.