1536094 - Dynamic module import doesn't work in webextension content scripts

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

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;

Comment 1

[robbendebiene]

Attachment: Webextension to test dynmaic import in content scripts

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

[robbendebiene]

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

Comment 3

[Librem Grief]

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

[jacksteamdev]

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.

Comment 5

[jackworks]

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"

Comment 6

[CRIMX]

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

[Tom Butler]

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

Comment 8

[eritbh]

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.

Comment 9

[choksheak]

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

[April Sylph]

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

[Devin Bayer]

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)

Comment 12

[Tom S [:evilpie]]

Attachment: Bug 1536094 - Add SandboxWindowOrNull to get Window wrapped by Sandbox. r?smaug

Comment 13

[Tom S [:evilpie]]

Attachment: Bug 1536094 - Support dynamic import from content scripts (sandboxed code)

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

Comment 14

[Tom S [:evilpie]]

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.

Comment 15

[Tom S [:evilpie]]

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 16

[Tom Butler]

Since it doesn't look like this might be fixed soon, in addition to Devin's approach, here's my own workaround that doesn't inject code onto the page: It rewrites the code on the fly before loading it with eval (ugly I know).

function ffimport(url) {
	url =  browser.extension.getURL(url)
	return new Promise(resolve => {
		const xhr = new XMLHttpRequest();

		xhr.addEventListener('load', () => {
			let js = xhr.responseText;
			let __exportFFHack;
 			js = js.replace('export default', 'return ');


			js = js.replace(/import (.+?) from ([^;]+)\;/g, 'var $1 = await ffimport($2);');
			(async () => {
				 eval('__exportFFHack = (async () => { ' + js + '})();');
				 resolve(__exportFFHack);		
			})();
		});

		xhr.open('GET', url);
		xhr.send();
	});
}



(async () => {
	const src = browser.extension.getURL('contentMain.js');

	// (sort of) support FF and Chrome
	if (navigator.appCodeName && navigator.appCodeName == 'Mozilla') {
		var contentMain = await ffimport(src);	
	}
	else {
		var contentMain = await import(src);
	}


	contentMain();
})();

This loads the module contentMain.js and rewrites the code on the fly recursively to load all the modules. It only supports import x from 'y.js' and it only supports export default rather than individual exports since that's how I prefer to organize my code.

Might not be useful for everyone as-is but a starting point perhaps. Personally it was easier to do this than to write the extension with different features in different browsers.

Comment 17

[jackworks]

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.

Comment 18

[Tom S [:evilpie]]

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?

Comment 19

[Tom S [:evilpie]]

Attachment: Bug 1536094 - Log promise rejections from sandboxed code to the right window

Depends on D107074

Comment 20

[Phabricator Automation]

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

[Phabricator Automation]

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.

Comment 22

[Jon Coppeard (:jonco)]

(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?

Comment 23

[Tom S [:evilpie]]

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

[Rob Wu [:robwu]]

(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).

Comment 25

[Tom S [:evilpie]]

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

[jackworks]

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.

Comment 27

[Tom S [:evilpie]]

Attachment: Bug 1536094 - Add test for importing from WebExtension content scripts

Depends on D107076

Comment 28

[Jon Coppeard (:jonco)]

(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.

Comment 29

[Tom S [:evilpie]]

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.

Comment 30

[Jon Coppeard (:jonco)]

(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.

Comment 31

[Tom S [:evilpie]]

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.

Comment 32

[Tom S [:evilpie]]

Attachment: Bug 1536094 - CC changes. r?smaug

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

Comment 33

[Pulsebot]

Pushed by [e-mail]:
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

Comment 34

[Rob Wu [:robwu]]

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

[robbendebiene]

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

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b650c30f27ec
Backed out 3 changesets for causing valgrind failures.

Comment 37

[Pulsebot]

Pushed by [e-mail]:
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

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 39

[Pulsebot]

Pushed by [e-mail]:
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

Comment 40

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/c578a7dd691c
https://hg.mozilla.org/mozilla-central/rev/ddba49efbd9f
https://hg.mozilla.org/mozilla-central/rev/ba4c269cea68