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

How should errors in cross-origin importScripts() sanitized when reported to WorkerGlobalScope error event? #5772

Closed
hiroshige-g opened this issue Jul 30, 2020 · 4 comments

Comments

@hiroshige-g
Copy link
Contributor

For

new Worker('worker.js');

with worker.js:

self.addEventListener("error", e => {...});
importScripts('https://cross-origin.example.com/syntax-error.js');

According to the spec IIUC, "NetworkError" DOMException is reported to the WorkerGlobalScrope's error event, while <script src="https://cross-origin.example.com/syntax-error.js"></script> would report "Script error.".

  • https://html.spec.whatwg.org/C/#run-a-classic-script is executed for the worker.js.
    • From Step 7 (ScriptEvaluation() for worker.js), importScripts() is executed, and thus inner https://html.spec.whatwg.org/C/#run-a-classic-script is executed for the syntax-error.js with muted errors = true.
      • evaluationStatus is set to the parse error of syntax-error.js, and
      • a "NetworkError" DOMException is thrown in Step 8.2.2 of the inner #run-a-classic-script.
    • After returning from inner #run-a-classic-script, #report-the-exception is executed in Step 8.3.1 of the outer #run-a-classic-script for worker.js.
      • The "NetworkError" DOMException is passed as e.error in the WorkerGlobalScope's error handler.
      • This is not sanitized to "Script error." because the script here is worker.js with muted errors flag unset.

Is this expected behavior?
Anyway the details of errors are not exposed so I don't think this is a major issue; I just want to double check, because I'm considering chaning Chromium's behavior from "Script error." to NetworkError DOMException, and this might be affected by #958.

Implementation behavior:

Chromium Firefox Safari
e.error "Script error." NetworkError DOMException "Error: Script error."
e.filename "" worker.js worker.js
e.lineno 0 lineno in worker.js colno in worker.js
e.colno 0 0 colno in worker.js

Investigation WPT: web-platform-tests/wpt#24810

@domenic
Copy link
Member

domenic commented Jul 30, 2020

This behavior is expected. In particular, consider a slight modification of your example:

try {
  importScripts('https://cross-origin.example.com/syntax-error.js');
} catch (e) {
  ...
}

The logic is that the ... code needs to be given a real JavaScript Error object. So we synthesize a new "NetworkError" DOMException for that purpose.

But, if you didn't catch that "NetworkError" DOMException, then it will reach the global scope, and the self.addEventListener("error", e => { ... }) handler.

So it's best to think of these errors as coming from the "same-origin" importScripts function, not from the cross-origin https://cross-origin.example.com/syntax-error.js script.

Thanks for working on web platform tests for this! Regarding the not-very-well-specified filename/lineno/colno fields, I think Firefox's results are more in line with the above philosophy, and I'd support adding tests for those. (Maybe even try making colno nonzero with something like false,importScripts(...).)

@hiroshige-g
Copy link
Contributor Author

Thanks, that makes sense!

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=1111750 for fixing Chromium behavior.

Firefox seems to sanitize colno anyway (errors at the beginning of a line result in colno = 1).

@hiroshige-g
Copy link
Contributor Author

Also updated the WPT according to the current spec's expectations.

@domenic
Copy link
Member

domenic commented Jul 31, 2020

Great! I will close this spec issue then.

@domenic domenic closed this as completed Jul 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants