Skip to content

Exception due to trying to access document from a worker #7222

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

Closed
cobarx opened this issue Oct 5, 2018 · 2 comments
Closed

Exception due to trying to access document from a worker #7222

cobarx opened this issue Oct 5, 2018 · 2 comments

Comments

@cobarx
Copy link
Contributor

cobarx commented Oct 5, 2018

We are seeing an exception coming from shell.js because the worker accesses document.currentScript.

From src/shell.js#231

if (ENVIRONMENT_IS_WEB || ENVIRONMENT_IS_WORKER) {
  if (ENVIRONMENT_IS_WEB) {
    if (document.currentScript) {
      scriptDirectory = document.currentScript.src;
    }
  } else { // worker
    scriptDirectory = self.location.href;
  }

I've tested and at least in our environment, the worker has ENVIRONMENT_IS_WEB and ENVIRONMENT_IS_WORKER true. If this is always the case, the else clause is a no-op. I'm not completely familiar with this code but would suggest this becomes:

if (ENVIRONMENT_IS_WEB || ENVIRONMENT_IS_WORKER) {
  if (ENVIRONMENT_IS_WORKER) {
    scriptDirectory = self.location.href;
  } else if (document.currentScript) { // web
    scriptDirectory = document.currentScript.src;
  }

The other approach would be:

if (ENVIRONMENT_IS_WEB || ENVIRONMENT_IS_WORKER) {
  if (ENVIRONMENT_IS_WEB) {
    if (typeof document !== 'undefined' && document.currentScript) {
      scriptDirectory = document.currentScript.src;
    }
  } else { // worker
    scriptDirectory = self.location.href;
  }

The code was introduced in #6894, maybe @kripken can provide some insight on the right approach. Let me know if you'd like me to open a PR.

@kripken
Copy link
Member

kripken commented Oct 5, 2018

I've tested and at least in our environment, the worker has ENVIRONMENT_IS_WEB and ENVIRONMENT_IS_WORKER true.

Interesting, that should not be the case. But we do define

ENVIRONMENT_IS_WEB = typeof window === 'object';

which means that if window is defined in a worker (for polyfill purposes, perhaps) then that would become true. So yes, we do need to fix this.

I think your first proposal is a good solution here, thanks, please open a PR with that if you can.

kripken pushed a commit that referenced this issue Oct 10, 2018
Fixes #7222

In some situations, the window object may be defined in a worker context, for example if window is defined for a polyfill. In these cases, the worker would report ENVIRONMENT_IS_WEB true and attempt to access document.currentScript. This would cause the worker to crash.

This PR inverts that check so that we check if we're a worker rather than web. This shouldn't cause any changes to functionality.
@kitten23
Copy link

Hello,
I'm tring to call the .js from a worker. The file path is like:
'.../workers/api-worker.js' (the worker)
'.../wasm/api.js' (created by emcc)
'.../wasm/api.wasm' (created by emcc)

when i call importScripts('.../wasm/api.js'); in 'api-worker.js'
self.location.href points to '.../workers/api-worker.js' andscriptDirectory was set to '.../workers'.
then function locateFile set wasmBinaryFile to '.../workers/api.wasm' and can not fetch the binary file (the real path is '.../wasm/api.wasm').
so i move 'api-worker.js' to '.../wasm/' to make it work.

is this the correct way?

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

No branches or pull requests

3 participants