Closed Bug 1628307 Opened 4 months ago Closed 3 months ago

ExtensionTestUtils.loadContentPage(url, {extension}) uses incorrect remote setting when extensions run in-process

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox77 fixed)

RESOLVED FIXED
mozilla77
Tracking Status
firefox77 --- fixed

People

(Reporter: robwu, Assigned: mixedpuppy)

References

Details

Attachments

(1 file)

In https://bugzilla.mozilla.org/show_bug.cgi?id=1613796#c14 Shane found that the following test fails:

./mach xpcshell-test --log-mach-verbose --tag in-process-webextensions toolkit/components/extensions/test/xpcshell/test_ext_permissions.js

I ran it locally, and the failure is in a specific part of test_alreadyGranted in test_ext_permissions.js that uses an extension API from an extension page in a ContentPage. The error message is as follows:

JavaScript error: moz-extension://daa1899d-c62c-4875-9651-c92609526ed3/page.js, line 4: TypeError: can't access property "request", browser.permissions is undefined

Upon further investigation, I found that the relevant line is:

let page = await ExtensionTestUtils.loadContentPage(url, { extension });

When extensions run in-process, the ContentPage should be loaded in the main process. However, the ContentPage constructor defaults to using a content process when e10s is enabled. As a result, the extension page is sent to the wrong process, the extension API bindings are not exposed, and the test fails.

Summary: ExtensionTestUtils.loadContentPage(url, {extension}) chooses uses incorrect remote when extensions run in-process → ExtensionTestUtils.loadContentPage(url, {extension}) uses incorrect remote setting when extensions run in-process

This specific bug can be fixed by defaulting to extension.remote if remote are set, like this:

--- a/toolkit/components/extensions/ExtensionXPCShellUtils.jsm
+++ b/toolkit/components/extensions/ExtensionXPCShellUtils.jsm
@@ -177,11 +177,18 @@ function promiseBrowserLoaded(browser, url, redirectUrl) {
 
 class ContentPage {
   constructor(
-    remote = REMOTE_CONTENT_SCRIPTS,
+    remote = null,
     extension = null,
     privateBrowsing = false,
     userContextId = undefined
   ) {
+    if (remote === null) {
+      if (extension) {
+        remote = extension.remote;
+      } else {
+        remote = REMOTE_CONTENT_SCRIPTS;
+      }
+    }
     this.remote = remote;
     this.extension = extension;
     this.privateBrowsing = privateBrowsing;

Not sure if that is worth the effort though, especially since we're probably going to drop support for in-process extensions soonish (bug 1613141). GeckoView has switched to out-of-process extensions (bug 1535365), which means that the only situation with in-process extensions is when e10s is disabled altogether. That condition matches with the current default value for remote in the ContentPage constructor.

I think that long-term, we should remove the extension parameter from the constructor, and have a common implementation for process switching (bug 1580811) (not just for the initial load, but also for any redirects/navigations).

Depends on: 1580811
See Also: → 1613141

The problem is that the default for "browser.tabs.remote.autostart" has changed from false to true (or something runtime is doing that). There is a comment in head_e10s about the default being false for xpcshell, so when running in-process we just assume it is false. Explicitly setting it to false fixes the problem without the change in comment 1.

Assignee: nobody → mixedpuppy
Status: NEW → ASSIGNED
Blocks: 1622917
Priority: -- → P1
Pushed by scaraveo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f1d8fcb50da0
fix running webextension in-process tests r=robwu
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla77
You need to log in before you can comment on or make changes to this bug.