Closed Bug 1536094 Opened 5 years ago Closed 3 years ago

Dynamic module import doesn't work in webextension content scripts

Categories

(WebExtensions :: General, enhancement, P3)

enhancement

Tracking

(firefox67 wontfix, firefox89 fixed)

RESOLVED FIXED
89 Branch
Tracking Status
firefox67 --- wontfix
firefox89 --- fixed

People

(Reporter: jonco, Assigned: evilpie)

References

Details

(Keywords: dev-doc-complete)

Attachments

(4 files, 2 obsolete files)

From bug 1517546 comment 6:

Hi, maybe I'm doing something wrong, but I'm not able to import a module in a webextension content script via the dynamic import. I also tried this version which works in chrome: https://github.com/otiai10/chrome-extension-es6-import

By trying it myself I got the following errors:
Error: TelemetryStopwatch: key "WEBEXT_CONTENT_SCRIPT_INJECTION_MS" was already initialized ExtensionTelemetry.jsm:106:31
Error: TelemetryStopwatch: key "WEBEXT_CONTENT_SCRIPT_INJECTION_MS_BY_ADDONID" was already initialized ExtensionTelemetry.jsm:110:41
Error: TelemetryStopwatch: requesting elapsed time for nonexisting stopwatch. Histogram: "WEBEXT_CONTENT_SCRIPT_INJECTION_MS", key: "" ExtensionTelemetry.jsm:106:31
Error: TelemetryStopwatch: requesting elapsed time for nonexisting stopwatch. Histogram: "WEBEXT_CONTENT_SCRIPT_INJECTION_MS_BY_ADDONID"

However the provided add-on above does not produce any errors, but still doesn't log anything to the console.

Firefox Developer Edition 66.0b14 (64-Bit), Linux with enabled javascript.options.dynamicImport pref;

Priority: -- → P3

It would be great if this bug could be fixed, since there is currently no other method to use ES6 modules in content scripts (in contrast to background scripts, option pages etc.).

In addition dynamic import would serve as a good workaround for https://bugzilla.mozilla.org/show_bug.cgi?id=1451545

I've attached a simple extension in order to better test the problem.

While the previous Firefox version did not output any errors Firefox 67.0b1 now outputs the following error message:

Error: No ScriptLoader found for the current context
Type: defect → enhancement
Component: DOM: Core & HTML → General
Product: Core → WebExtensions
Summary: Problem with dynamic module import in web extension → Dynamic module import doesn't work in webextension content scripts

This doesn't work with background scripts either. All one gets is

SyntaxError: import declarations may only appear at top level of a module

This bug has made its way on to StackOverflow.

Our bundling tool implements code-splitting and uses dynamic imports for that. Works great in Chrome, but no way to adapt for Firefox.

For background script, you can use a background page file, then use <script type=module> in it.

But there is no way to use ES Modules for content script. Using static import will throw, dynamic import will throw "No ScriptLoader found for the current context"

Depends on: 1587336

Never thought this could be a bug and now we have to roll back😂. Please support dynamic import as there is no other way AFAIK to dynamically load modules to content script.

This is a frustrating omission of functionality as it makes sharing classes/functions between content scripts and popups more difficult than it needs to be.

Chrome doesn't properly support this either but there is a workaround: https://stackoverflow.com/a/53033388/471227

See Also: → 1451545

Why is this marked as an enhancement rather than a defect? Dynamic imports are generally supported, but broken in content scripts; that seems like a bug to me. This Firefox-specific issue is preventing me from cleaning up our extension to take advantage of ES6 modules, as dynamic imports in content scripts on Chromium-based browsers are working as expected.

This is a BLOCKER for deloyment of module-based javascript content scripts. Since it works fine in background scripts (using <script type=module> in background.html), I think there is no reason why it should not work in content scripts. FYI: Dynamic imports work fine in Chrome and Edge. Hope this gets fixed soon. Thanks!!!

This is a source of frustration for me as well. The cross-browser solution I've got for my project relies on eval, which won't be allowed in Chrome Manifest v3 - not sure what I'll do if this bug isn't fixed by the time v2 is phased out.

There is a workaround which works for me and that's to replace the content script with a thin wrapper that passes messages between the extension and a module loaded on the page:

browser.runtime.onMessage.addListener(msg => {
    window.dispatchEvent(new CustomEvent('myext:in', {
        detail: JSON.stringify(msg)
    }))
})

window.addEventListener('myext:out', e => {
    browser.runtime.sendMessage(event.detail)
})

const s = document.createElement('script');
s.type = 'module'
s.src = browser.extension.getURL(`src/page.js?ts=${Date.now()}`)
document.body.append(s)
Assignee: nobody → evilpies

Firstly we need to find a usable ScriptLoader for code in the content script sandbox,
for that we use the normal ScriptLoader associated with DOMWindow wrapped by the sandbox.

Secondly we need to execute the module in the global of the sandbox instead of the
"ScriptGlobal" the ScriptLoader is actually associated with. The main
behavior change here comes from using xpc::NativeGlobal in HostImportModuleDynamically
and passing that global around inside ModuleLoadRequest.

Depends on D107074

I guess we should also block data: URLs from WebExtensions as suggested by bug 1587336. Requiring the modules to be web_accessible_resources is also not great and should be fixed.

Jon rightly points out that there could be potential problems when a webpage tries to load a module that was already loaded by the WebExtension.
I think in the end this is going to to force us to abandon this approach. We probably need to wait for whatever simplification we get from supporting modules in workers and then we can "just" give the Sandbox its own ScriptLoader.

So I want to point out that Devin Bayer's workaround is actually wrong. The code is running in the main world instead of the content script.

I think I might have found a solution for the problem of a webpage loading a module loaded by a content-script before hand. We simply disallow loading modules from a different principal. I added a check to ModuleLoadRequest::ModuleLoaded that seems to work, but I am not sure if this covers all places. Do we always go through that function?

This means the modules importable by a webpage and a content-script are completely distinct. They can't share modules in either direction.
Is this feasible?

Flags: needinfo?(jcoppeard)

Depends on D107074

Depends on: 1696756

Comment on attachment 9206666 [details]
Bug 1536094 - Add SandboxWindowOrNull to get Window wrapped by Sandbox. r?smaug

Revision D107074 was moved to bug 1696756. Setting attachment 9206666 [details] to obsolete.

Attachment #9206666 - Attachment is obsolete: true

Comment on attachment 9207250 [details]
Bug 1536094 - Log promise rejections from sandboxed code to the right window

Revision D107374 was moved to bug 1696756. Setting attachment 9207250 [details] to obsolete.

Attachment #9207250 - Attachment is obsolete: true

(In reply to Tom Schuster [:evilpie] from comment #18)

This means the modules importable by a webpage and a content-script are completely distinct.

That sounds good. But won't disallowing modules with a different principal break things when the page and the extension both try to load the same module?

Flags: needinfo?(jcoppeard)

I think this might turn out not be an issue. Bug 1587336 suggests limiting content-scripts to importing moz-extension URLs, so they won't be able to import anything from a normal webpage. Webpages importing scripts from WebExtensions also is quite limited, it requires using web_accessible_resources.

(In reply to Tom Schuster [:evilpie] from comment #23)

I think this might turn out not be an issue. Bug 1587336 suggests limiting content-scripts to importing moz-extension URLs, so they won't be able to import anything from a normal webpage. Webpages importing scripts from WebExtensions also is quite limited, it requires using web_accessible_resources.

What is the behavior in the following situations (before and after your patch):

  • web page uses import() with a moz-extension:-URL
  • web page uses import() with a URL that is redirected to a moz-extension:-URL via the webRequest API.

The last one is probably not uncommon. There are add-ons that redirect script requests to a local script URL (e.g. uBlock Origin, Decentraleyes, LocalCDN).

Those imports from the web page should continue to work normally. Unless the same URL was previously imported by a content script in that web page. I doubt this affects any of the extensions you mentioned.

I think the main word script and content scripts should totally not share the module graph.

Those imports from the web page should continue to work normally. Unless the same URL was previously imported by a content script in that web page.

This should not happen.

(In reply to Tom Schuster [:evilpie] from comment #23)

I think this might turn out not be an issue. Bug 1587336 suggests limiting content-scripts to importing moz-extension URLs

That only talks about dynamic import. Do content scripts not statically import 'normal' JS modules? (I really don't know much about extensions.)

Sorry if I wasn't sufficiently clear before: I think that using the same loader for both types of modules is a bad idea that could lead to security or privacy issues. Maybe there is a way to mitigate that, but this is a large and complex piece of code and I don't think it's an approach we should be taking.

That only talks about dynamic import. Do content scripts not statically import 'normal' JS modules? (I really don't know much about extensions.)

Content scripts (currently at least) can no be run as module scripts, so they only have dynamic imports. The dynamically imported modules can of course have static imports.

Sorry if I wasn't sufficiently clear before: I think that using the same loader for both types of modules is a bad idea that could lead to security or privacy issues. Maybe there is a way to mitigate that, but this is a large and complex piece of code and I don't think it's an approach we should be taking.

I generally I agree and it would be nice to give content scripts their own ScriptLoader. However the ScriptLoader and Document are quite tied together so changing this is going to be a lot of work.

I think I have come up with an approach that allows this to work without having to do too much work. We basically key the module map by <URI, Global> instead of just <URI>. That means everything imported from the content script's global is completely independent from what the webpage is importing.

(In reply to Tom Schuster [:evilpie] from comment #29)
I didn't realise why I wrote my comment that the patch keyed the module map off the global too. This is not ideal, but I don't see any obvious problem with it either. So I think this approach could be OK after all.

There are a lot of leaks that I have to investigate: https://treeherder.mozilla.org/jobs?repo=try&revision=7320e8bd672b4b368fa1d4df351824433ec72703. Probably related to the nsIGlobalObject that we are referencing from the scripts.

And html/semantics/scripting-1/the-script-element/module/dynamic-import/string-compilation-other-document.html fails which might point to the module map approach not being viable.

I put this in a different patch so the changes are more obvious.
I have basically no idea what I am doing ...

Depends on D107076

Attachment #9209305 - Attachment description: Bug 1536094 - WIP: CC changes. r?smaug → Bug 1536094 - CC changes. r?smaug
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/863933e887e8
Support dynamic import from content scripts (sandboxed code) r=smaug,jonco
https://hg.mozilla.org/integration/autoland/rev/c73979442002
CC changes. r=smaug
https://hg.mozilla.org/integration/autoland/rev/dfe051a9c91a
Add test for importing from WebExtension content scripts r=smaug,robwu
Keywords: dev-doc-needed

This question is only partially related to this bug, but I didn't know any better place to ask it.

When I run dynamic import at document_start can I somehow ensure that the modules will also run at document_start / before the page scripts? This may be required because the module needs to register some event listeners before the page does. I suppose this is the only limitation of dynamically loading modules.

Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b650c30f27ec
Backed out 3 changesets for causing valgrind failures.
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d566c1c9e82f
Support dynamic import from content scripts (sandboxed code) r=smaug,jonco
https://hg.mozilla.org/integration/autoland/rev/3462cb6573b1
CC changes. r=smaug
https://hg.mozilla.org/integration/autoland/rev/464143c2b6ac
Add test for importing from WebExtension content scripts r=smaug,robwu
Regressions: 1700228

Backed out 3 changesets (bug 1536094) for causing bug 1700228.

Push with failure: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&selectedTaskRun=IjPZb7BGRq6ufHnB9mXuVg.0&searchStr=linux%2Cx64%2Cdebug%2Chazard-linux64-haz%2Fdebug%2Ch&revision=d19ecd7a3f2cfa52e7fe8f639473385b4fb13e81

Backout link: https://hg.mozilla.org/integration/autoland/rev/cc317dd90f8a72a852a64f4ba9febfd5d5912f84

Failure log: https://treeherder.mozilla.org/logviewer?job_id=334048156&repo=autoland&lineNumber=18769

[task 2021-03-22T22:28:04.967Z] Running heapwrites to generate heapWriteHazards.txt
[task 2021-03-22T22:28:04.967Z] PATH="/builds/worker/fetches/gcc/bin:/builds/worker/fetches/sixgill/usr/bin:${PATH}" ANALYZED_OBJDIR='/builds/worker/workspace/obj-analyzed-browser' XDB='/builds/worker/fetches/sixgill/usr/bin/xdb.so' SOURCE='/builds/worker/checkouts/gecko' /builds/worker/workspace/obj-haz-shell/dist/bin/js /builds/worker/checkouts/gecko/js/src/devtools/rootAnalysis/analyzeHeapWrites.js > heapWriteHazards.txt
[task 2021-03-22T22:28:41.207Z] + check_hazards /builds/worker/workspace/haz-browser
[task 2021-03-22T22:28:41.208Z] + set +e
[task 2021-03-22T22:28:41.208Z] ++ grep -c 'Function.*has unrooted.*live across GC call' /builds/worker/workspace/haz-browser/rootingHazards.txt
[task 2021-03-22T22:28:41.212Z] + NUM_HAZARDS=1
[task 2021-03-22T22:28:41.212Z] ++ grep -c '^Function.*takes unsafe address of unrooted' /builds/worker/workspace/haz-browser/refs.txt
[task 2021-03-22T22:28:41.214Z] + NUM_UNSAFE=224
[task 2021-03-22T22:28:41.214Z] ++ grep -c '^Function.* has unnecessary root' /builds/worker/workspace/haz-browser/unnecessary.txt
[task 2021-03-22T22:28:41.216Z] + NUM_UNNECESSARY=1217
[task 2021-03-22T22:28:41.216Z] ++ grep -c '^Dropped CFG' /builds/worker/workspace/haz-browser/build_xgill.log
[task 2021-03-22T22:28:41.245Z] + NUM_DROPPED=0
[task 2021-03-22T22:28:41.245Z] ++ perl -lne 'print $1 if m!found (\d+)/\d+ allowed errors!' /builds/worker/workspace/haz-browser/heapWriteHazards.txt
[task 2021-03-22T22:28:41.251Z] + NUM_WRITE_HAZARDS=0
[task 2021-03-22T22:28:41.252Z] ++ grep -c '^Function.*expected hazard.*but none were found' /builds/worker/workspace/haz-browser/rootingHazards.txt
[task 2021-03-22T22:28:41.254Z] + NUM_MISSING=0
[task 2021-03-22T22:28:41.254Z] + set +x
[task 2021-03-22T22:28:41.254Z] TinderboxPrint: rooting hazards<br/>1
[task 2021-03-22T22:28:41.254Z] TinderboxPrint: (unsafe references to unrooted GC pointers)<br/>224
[task 2021-03-22T22:28:41.254Z] TinderboxPrint: (unnecessary roots)<br/>1217
[task 2021-03-22T22:28:41.254Z] TinderboxPrint: missing expected hazards<br/>0
[task 2021-03-22T22:28:41.254Z] TinderboxPrint: heap write hazards<br/>0
[task 2021-03-22T22:28:41.256Z] TEST-UNEXPECTED-FAIL | hazards | unrooted 'object' of type 'JSObject*' live across GC call at dom/script/ScriptLoader.cpp:978
[task 2021-03-22T22:28:41.256Z] TEST-UNEXPECTED-FAIL | hazards | 1 rooting hazards detected
[task 2021-03-22T22:28:41.256Z] TinderboxPrint: documentation<br/><a href='https://wiki.mozilla.org/Javascript:Hazard_Builds#Diagnosing_a_rooting_hazards_failure'>static rooting hazard analysis failures</a>, visit "Inspect Task" link for hazard details
[task 2021-03-22T22:28:41.256Z] + grab_artifacts
Flags: needinfo?(evilpies)
Attachment #9206668 - Attachment description: Bug 1536094 - Support dynamic import from content scripts (sandboxed code) → WIP: Bug 1536094 - Support dynamic import from content scripts (sandboxed code)
Attachment #9208758 - Attachment description: Bug 1536094 - Add test for importing from WebExtension content scripts → WIP: Bug 1536094 - Add test for importing from WebExtension content scripts
Attachment #9208758 - Attachment description: WIP: Bug 1536094 - Add test for importing from WebExtension content scripts → Bug 1536094 - Add test for importing from WebExtension content scripts
Attachment #9206668 - Attachment description: WIP: Bug 1536094 - Support dynamic import from content scripts (sandboxed code) → Bug 1536094 - Support dynamic import from content scripts (sandboxed code)
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c578a7dd691c
Support dynamic import from content scripts (sandboxed code) r=smaug,jonco
https://hg.mozilla.org/integration/autoland/rev/ddba49efbd9f
CC changes. r=smaug
https://hg.mozilla.org/integration/autoland/rev/ba4c269cea68
Add test for importing from WebExtension content scripts r=smaug,robwu
Flags: needinfo?(evilpies)
Flags: in-testsuite+
See Also: → 1803950
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: