Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add experimental support for EM_JS code in side modules #3676

Closed
wants to merge 1 commit into from

Conversation

hoodmane
Copy link
Member

@hoodmane hoodmane commented Mar 20, 2023

Based on my discussion with @szabolcsdombi in #3671, I thought it would be fun to experiment with enabling EM_JS in side modules. The EM_JS is put into a file called blah.so.js which contains code like:

globalThis.__tmpMergeLibsFunction = function(Module) {
	function function_implemented_in_javascript(argument) { console.log('hello', argument); }
	
	Module.mergeLibSymbols(
		{function_implemented_in_javascript}
	);
}

we load it with loadScript, call __tmpMergeLibsFunction and then delete it. We need a lock around this code to ensure that another library doesn't write over the __tmpMergeLibsFunction global before the first is used (we need to use a global because of classic workers).

I tested it manually against @szabolcsdombi's example:
https://github.com/szabolcsdombi/pyodide-custom-module-imports/commit/3d51d923cb89941024ea31cc38b8df73a2a199ad
and it seems to work.

@sbc100

Checklists

  • Add a CHANGELOG entry
  • Add / update tests
  • Add new / update outdated documentation

Comment on lines +174 to +178
libstr = "data:text/javascript," + libstr;
await loadScript(libstr);
let gt = globalThis as any;
gt.__tmpMergeLibsFunction(Module);
delete gt.__tmpMergeLibsFunction;
Copy link
Member Author

@hoodmane hoodmane Mar 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: There should be an async lock around this code.

@sbc100
Copy link

sbc100 commented Mar 20, 2023

For EM_ASM this was fixed in emscripten-core/emscripten#18228. Here use new Function after extracting the JS string from linear memory. Would that approach work for you?

@hoodmane
Copy link
Member Author

hoodmane commented Mar 20, 2023

It would be nice to support pages with a content security policy that does not allow unsafe-eval (we obviously need wasm-unsafe-eval).

Though I'm not entirely sure that this is possible. I'm not sure whether import("data:text/javascript,blah") or importScripts("data:text/javascript,blah") is allowed, since it is just eval in different clothes. There isn't any mention of it in the spec:

The following JavaScript execution sinks are gated on the "unsafe-eval" source expression:

https://www.w3.org/TR/CSP3/#directive-script-src

@sbc100
Copy link

sbc100 commented Mar 20, 2023

It would be nice to support pages with a content security policy that does not allow unsafe-eval (we obviously need wasm-unsafe-eval).

Though I'm not entirely sure that this is possible. I'm not sure whether import("data:text/javascript,blah") or ``("data:text/javascript,blah")is allowed, since it is justeval` in different clothes. There isn't any mention of it in the spec:

The following JavaScript execution sinks are gated on the "unsafe-eval" source expression:

https://www.w3.org/TR/CSP3/#directive-script-src

Upstream, we will probably provide "out-of-the-box" support for using new Function initially, and then we can look into importScripts as a followup if folks run into issues with it.

@hoodmane
Copy link
Member Author

hoodmane commented Mar 20, 2023

I guess another feature that is missing from the approach I'm using here is the closure. You write:

Its very common for EM_ASM code to assume it can call internal functions like out and err as well as any JS library functions.

emscripten-core/emscripten#18228 (comment)

Which is certainly true. It is possible to say Module.out everywhere but it would be nice not to have to do it. Too bad with is not allowed in strict mode we could use with(Module) 😆.

@ryanking13
Copy link
Member

Cool, it would be really interesting if we can move some JS parts into packages so we can reduce the size of the main module.

@hoodmane hoodmane closed this Jul 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants