search

whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
8.17k stars 2.69k forks source link

Fire an ErrorEvent instead an Event #2289

Open bakulf opened 7 years ago

bakulf commented 7 years ago

https://html.spec.whatwg.org/multipage/workers.html#worker-processing-model-top

If the algorithm asynchronously completes with null, queue a task to fire an event named error at worker, and abort these steps. Otherwise, continue the rest of these steps after the algorithm's asynchronous completion, with script being the asynchronous completion value.

Why are we firing a normal event and not an ErrorEvent? With an ErrorEvent we can specify a message and this can be useful.

zcorpan commented 7 years ago

What are the different cases where error would fire here? Just if fetching failed for some reason? Note that we can't expose reason for fetch failure to JS in general for security reasons (but browser devtools is OK I think).

bakulf commented 7 years ago

There are some WPT tests where we use ErrorEvent when, in theory, we should have Error. For instance:

. workers/constructors/Worker/same-origin.html . workers/constructors/SharedWorker/same-origin.html

Are they wrong? Or am I missing something?

zcorpan commented 7 years ago

Hmm. They look wrong per spec. But if more browsers fire ErrorEvent here then we should probably change the spec...

bzbarsky commented 7 years ago

Just if fetching failed for some reason?

Pretty much, yes. Gecko code has a codepath here for "URL parse failure", but I'm not sure that's really reachable.

So maybe there's nothing useful that can be communicated here...

zcorpan commented 7 years ago

The tests also allow either an exception or an error event, which seems bogus.

zcorpan commented 7 years ago

The spec's first step for Worker constructor says

The user agent may throw a "SecurityError" DOMException and abort these steps if the request violates a policy decision (e.g. if the user agent is configured to not allow the page to start dedicated workers).

but it's unclear to me it is a good idea to have these tests pass if a SecurityError is raised. In Chromium it seems SecurityError is raised for cross-origin URLs, but that is not really per spec (since the URL has not been parsed yet in the spec).

zcorpan commented 7 years ago

git blame points to

https://github.com/w3c/web-platform-tests/commit/98fdc494750be3801cd445d449bd2a270fbe9ce2

https://bugzilla.mozilla.org/show_bug.cgi?id=1218433

bzbarsky commented 7 years ago

Mmm. Yes, looks like the Firefox code there is not following the spec either, in that things like same-origin and "no mixed content" and "is this page even allowed to run workers" are all checked synchronously before the constructor returns, and lead to exceptions on failure.

hiroshige-g commented 2 years ago

As for the async Error event (aside from the sync SecurityError),

However, if the error is propagated from the WorkerGlobalScope (exceptions are not handled in WorkerGlobalScope.onerror), ErrorEvent is used (https://html.spec.whatwg.org/multipage/workers.html#runtime-script-errors-2).

The current browser behavior is:

Chrome Firefox Safari
fetch error (MIME type), classic Event Event Event
fetch error (MIME type), module Event Event Event
parse error, classic ErrorEvent (to be fixed by https://crbug.com/1071345) ErrorEvent ErrorEvent
parse error, module Event (fixed by https://crbug.com/1071345) N/A ErrorEvent
allstarschh commented 1 year ago

Hi, @domenic When a worker's script has an error to rethrow, according to the spec, it should fire an Event named error to the worker (instead of firing an ErrorEvent). Can you confirm that the spec is correct?

Specifically, in run a worker

  1. In both cases, let onComplete given script be the following steps:

    1. If script is null or if script's error to rethrow is non-null, then:

      1. Queue a global task on the DOM manipulation task source given worker's relevant global object to fire an event named error at worker.

      2. Run the environment discarding steps for inside settings.

      3. Abort these steps.

Because from WPT, some test testing with Event failed, whereas some test testing with ErrorEvent is passed among all browsers.

/workers/dedicated-worker-parse-error-failure.html, which is testing Event, is failed among all browsers https://wpt.fyi/results/workers/dedicated-worker-parse-error-failure.html?label=experimental&label=master&aligned https://github.com/web-platform-tests/wpt/blob/master/workers/dedicated-worker-parse-error-failure.html https://github.com/web-platform-tests/wpt/blob/master/workers/support/check-error-arguments.js#L3

There is another test, /workers/constructors/Worker/AbstractWorker.onerror.html which is passed in all browsers. https://wpt.fyi/results/workers/constructors/Worker/AbstractWorker.onerror.html?label=experimental&label=master&aligned But the test is still testing with ErrorEvent. https://github.com/web-platform-tests/wpt/blob/master/workers/constructors/Worker/AbstractWorker.onerror.html#L16

I've checked the commit history of AbstractWorker.onerror.html https://github.com/web-platform-tests/wpt/commits/master/workers/constructors/Worker/AbstractWorker.onerror.html The latest change for Event can be traced back to Jun, 2015 According to the spec back then

http://web.archive.org/web/20150604031622/https://html.spec.whatwg.org/multipage/workers.html#run-a-worker

  1. If the script was not compiled successfully, let the code entry-point be a no-op script, and act as if a corresponding uncaught script error had occurred.

http://web.archive.org/web/20150604031622/https://html.spec.whatwg.org/multipage/workers.html#runtime-script-errors-2

http://web.archive.org/web/20150604100312/https://html.spec.whatwg.org/multipage/webappapis.html#report-the-error

  1. Let event be a new trusted ErrorEvent object ...

So the test AbstractWorker.onerror.html looks like it was testing with the older spec, but not the current spec ?

And the commit to check if the script has an error to rethrow, is from https://github.com/whatwg/html/commit/c747ce0c403d6d3172432a8ee089263db4d74578

Although the fix was originally to fix SharedWorker, but I assume it also fixed DedicatedWorker?

Thanks

domenic commented 1 year ago

I cannot confirm the spec is correct. As attested by the fact that this issue is still open, there is a long history of discussion about what the correct type of event is to fire here.

domenic commented 1 year ago

After reviewing the exact location at which you inserted the note in https://github.com/whatwg/html/pull/9595, I think the current spec is correct. There's no way an ErrorEvent would make any sense, because the thrown error object only exists inside the WorkerGlobalScope's (unner) realm, not inside the Worker's (outer) realm. So there would be nothing we could populate errorEvent.error with.