Dynamic module import doesn't work in webextension content scripts
Categories
(WebExtensions :: General, enhancement, P3)
Tracking
(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;
Updated•5 years ago
|
Comment 1•5 years ago
|
||
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.
Comment 2•5 years ago
|
||
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
Reporter | ||
Updated•5 years ago
|
Comment 3•5 years ago
|
||
This doesn't work with background scripts either. All one gets is
SyntaxError: import declarations may only appear at top level of a module
Comment 4•4 years ago
|
||
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"
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.
Comment 7•4 years ago
|
||
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
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!!!
Comment 10•3 years ago
|
||
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.
Comment 11•3 years ago
|
||
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 | ||
Updated•3 years ago
|
Assignee | ||
Comment 12•3 years ago
|
||
Assignee | ||
Comment 13•3 years ago
|
||
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
Assignee | ||
Comment 14•3 years ago
|
||
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.
Assignee | ||
Comment 15•3 years ago
|
||
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.
Comment hidden (obsolete) |
Comment 17•3 years ago
|
||
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.
Assignee | ||
Comment 18•3 years ago
|
||
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?
Assignee | ||
Comment 19•3 years ago
|
||
Depends on D107074
Comment 20•3 years ago
|
||
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.
Comment 21•3 years ago
|
||
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.
Reporter | ||
Comment 22•3 years ago
|
||
(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?
Assignee | ||
Comment 23•3 years ago
|
||
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
.
Comment 24•3 years ago
|
||
(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 amoz-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).
Assignee | ||
Comment 25•3 years ago
|
||
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.
Comment 26•3 years ago
|
||
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.
Assignee | ||
Comment 27•3 years ago
|
||
Depends on D107076
Reporter | ||
Comment 28•3 years ago
|
||
(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.
Assignee | ||
Comment 29•3 years ago
|
||
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.
Reporter | ||
Comment 30•3 years ago
|
||
(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.
Assignee | ||
Comment 31•3 years ago
•
|
||
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.
Assignee | ||
Comment 32•3 years ago
|
||
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
Updated•3 years ago
|
Comment 33•3 years ago
|
||
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
Assignee | ||
Updated•3 years ago
|
Comment 34•3 years ago
|
||
Note for dev-doc-needed (once the patch sticks), to be submitted to https://github.com/mdn/content
- add to https://github.com/mdn/content/blob/592cdbf806edfc986431e57c4384b1b289fd5c4c/files/en-us/mozilla/add-ons/webextensions/content_scripts/index.html#L47 (rendered at https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Content_scripts#loading_content_scripts )
- add to release notes at files/en-us/mozilla/firefox/releases/89/index.html (to be created soonish? will be rendered at https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/89 )
Comment 35•3 years ago
|
||
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.
Comment 36•3 years ago
|
||
Backout by csabou@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b650c30f27ec Backed out 3 changesets for causing valgrind failures.
Comment 37•3 years ago
|
||
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
Comment 38•3 years ago
|
||
Backed out 3 changesets (bug 1536094) for causing bug 1700228.
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
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Comment 39•3 years ago
|
||
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
Assignee | ||
Updated•3 years ago
|
Comment 40•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c578a7dd691c
https://hg.mozilla.org/mozilla-central/rev/ddba49efbd9f
https://hg.mozilla.org/mozilla-central/rev/ba4c269cea68
Updated•3 years ago
|
Updated•3 years ago
|
Description
•