Add fallback for emval_get_global to support Web Workers #9723
Conversation
|
Thank you for submitting a pull request! If this is your first PR, make sure to add yourself to AUTHORS. |
There was a problem hiding this comment.
Looks reasonable, given that the surrounding code seems totally bonkers.
Do we actually need to assign $$$embind_global$$$ = self; if the condition is true, in any of these if cases? Doesn't testGlobal do that for us?
I'm completely okay with leaving that well enough alone, I just don't think I understand that part of the existing code.
src/embind/emval.js
Outdated
| @@ -222,6 +222,8 @@ var LibraryEmVal = { | |||
| $$$embind_global$$$ = global; | |||
| } else if (typeof window === 'object' && testGlobal(window)) { | |||
| $$$embind_global$$$ = window; | |||
| } else if (typeof self === 'object' && testGlobal(self)) { | |||
| $$$embind_global$$$ = self; | |||
There was a problem hiding this comment.
I wouldn't have guessed that self was for workers, though that might just be me. Might be worth a comment here?
There was a problem hiding this comment.
I agree. I've just added it for the sake of clarity.
There was a problem hiding this comment.
It would be good to know how far back self works. caniuse isn't clear: https://caniuse.com/#feat=mdn-api_window_self
The safe thing might be to check ENVIRONMENT_IS_WEB vs _WORKER, at least in LEGACY_VM_SUPPORT mode.
There was a problem hiding this comment.
@kripken I've added ENVIRONMENT_IS_WORKER precondition as a safety measure.
There was a problem hiding this comment.
self has existed basically forever (like top, parent, etc.). In fact, we don't even need a separate branch for window as self is more universal and works both in windows and in workers.
There was a problem hiding this comment.
Ok, sgtm to do it that way, thanks @RReverser
@osman-turan sorry for the back-and-forth! But sounds like it'll be better to do it like that.
There was a problem hiding this comment.
@kripken No problem. I've just changed that way with a comment. Please feel free to tell me if we need further improvement.
osman-turan
force-pushed
the
incoming
branch
2 times, most recently
from
770a141 to
f1a801d
Compare
Yes, during testing in |
|
Looks good, thanks @osman-turan! The CI test errors are because older incoming was broken, sorry about that. Please merge in latest incoming to here, and tests should pass. |
In generated WebAssembly modules, `emval_get_global` is used for accessing global variables. This function tries to get `globalThis` property with a fallback mechanism. Unfortunately, fallback mechanism does not use Web Workers environment global object (`self`). This commit fixes that problem.
|
@kripken No problem. It can be expected in such a huge code base. I've updated my pull-request with a clean state. |
|
Perfect, thanks again @osman-turan! |
…core#9723) In generated WebAssembly modules, `emval_get_global` is used for accessing global variables. This function tries to get `globalThis` property with a fallback mechanism. Unfortunately, fallback mechanism does not use Web Workers environment global object (`self`). This commit fixes that problem.
In generated WebAssembly modules,
emval_get_globalis used for accessing global variables. This function tries to getglobalThisproperty with a fallback mechanism. Unfortunately, fallback mechanism does not use Web Workers environment global object (self). This commit fixes that problem.