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

[wasm] proper startup sequence in dotnet.worker.js #70891

Closed
2 of 3 tasks
pavelsavara opened this issue Jun 17, 2022 · 5 comments
Closed
2 of 3 tasks

[wasm] proper startup sequence in dotnet.worker.js #70891

pavelsavara opened this issue Jun 17, 2022 · 5 comments
Assignees
Labels
arch-wasm WebAssembly architecture area-Build-mono
Milestone

Comments

@pavelsavara
Copy link
Member

pavelsavara commented Jun 17, 2022

  • pass configuration from main thread
  • run relevant parts of startup sequence
  • make worker long lived that that we could schedule mono jobs in worker and use promises

We compile dotnet.worker.js with -s EXPORT_ES6, so it used import(uri).then(callback) dynamic imports to bring in scripts.

There is still a polyfill for Node in the worker JS file that uses require and eval. It could be compiled out if we change the threaded build to not set node as an environment when we build for threading, see

https://github.com/emscripten-core/emscripten/blob/6577be0d782368893b8d0c2dbaac3908fc425ef6/src/settings.js#L615-L640


Original issue

  • the dotnet.worker.js has require in it, which makes it not ES6 ready
  • the dotnet.worker.js has eval in it which makes it CSP issue

We could implement some post-link build step, which would append and prepend code to the emscripten's code in dotnet.worker.js4
Or we could just have our own version, which we would have to keep in sync with upstream.

@pavelsavara pavelsavara added arch-wasm WebAssembly architecture area-Build-mono labels Jun 17, 2022
@pavelsavara pavelsavara self-assigned this Jun 17, 2022
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 17, 2022
@ghost
Copy link

ghost commented Jun 17, 2022

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details
  • the dotnet.worker.js has require in it, which makes it not ES6 ready
  • the dotnet.worker.js has eval in it which makes it CSP issue

We could implement some post-link build step, which would append and prepend code to the emscripten's code in dotnet.worker.js4
Or we could just have our own version, which we would have to keep in sync with upstream.

Author: pavelsavara
Assignees: pavelsavara
Labels:

arch-wasm, area-Build-mono

Milestone: -

@pavelsavara pavelsavara added this to the 7.0.0 milestone Jun 17, 2022
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jun 17, 2022
@lambdageek
Copy link
Member

lambdageek commented Jun 23, 2022

Couple of things:

  1. If you compile with -s EXPORT_ES6, the worker.js is built differently. it does this

    https://github.com/emscripten-core/emscripten/blob/25591f37aa77c415af9c54bc74d073f4653d22d7/src/worker.js#L154-L160

  2. We might still need our own version of worker.js since we may need to customize Module before it's passed to createDotnetRuntime, as we noted here [wasm-mt] Add MessageChannel between browser thread and new pthreads #70908 (comment)

@lambdageek
Copy link
Member

worker.js seems to only add require if ENVIRONMENT_MAY_BE_NODE is true, which seems like it is set here
https://github.com/emscripten-core/emscripten/blob/2391bfcacbd0396a1cda21843cf3c6a216e6a142/emcc.py#L294

settings.ENVIRONMENT_MAY_BE_NODE = not settings.ENVIRONMENT or 'node' in environments

so we could omit node from -s ENVIRONMENTS=... when we build with threading

@lambdageek
Copy link
Member

Just want to add now that the #70746 ES6 PR is merged that dotnet.worker.js is able to load the ES6 dotnet.js module, the node code at the beginning that uses require is skipped.

So the things left to address are to make sure we initialize Module with all the values that our code receives when we load dotnet.js on the main browser thread.

@pavelsavara pavelsavara changed the title [wasm] fix dotnet.worker.js [wasm] fix require in dotnet.worker.js Jul 11, 2022
@pavelsavara pavelsavara modified the milestones: 7.0.0, 8.0.0 Aug 10, 2022
@pavelsavara
Copy link
Member Author

@lambdageek I moved this to 8, please let me know if you think we need it in net7

@pavelsavara pavelsavara changed the title [wasm] fix require in dotnet.worker.js [wasm] proper startup sequence in dotnet.worker.js Sep 30, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Development

No branches or pull requests

2 participants