1831324 - DedicatedWorker should fire an event of type "Event" instead of "ErrorEvent" when the script has error to rethrow.

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect, P3)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

https://html.spec.whatwg.org/multipage/workers.html#run-a-worker
Step 14

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

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

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

        Run the environment discarding steps for inside settings.

        Abort these steps.

and
https://dom.spec.whatwg.org/#concept-event-fire

1. If eventConstructor is not given, then let eventConstructor be Event. 

Currently, our implementation will create an ErrorEvent for this

This causes the WPT failure in /workers/modules/dedicated-worker-parse-error-failure.html

We cannot simply change it to use Event because in other cases we need ErrorEvent.
See https://html.spec.whatwg.org/multipage/workers.html#runtime-script-errors-2

The other bug for SharedWorker is in bug 1627938

Comment 1

[Thomas Wisniewski [:twisniewski]]

This is true for both of these WPTs, so for both shared and dedicated workers:

• https://wpt.fyi/results/workers/modules/dedicated-worker-parse-error-failure.html
• https://wpt.fyi/results/workers/modules/shared-worker-parse-error-failure.html

Comment 2

[Thomas Wisniewski [:twisniewski]]

Ah, I didn't realize we have separate bugs for the shared and dedicated workers, I'll change the bug's title back.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1831324 - Part 1: Check if the module worker script has a parse error or an error to rethrow.

According to the spec,
https://html.spec.whatwg.org/#run-a-worker

14. In both cases, let onComplete given script be the following steps:
15. 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.

Check if the module worker script has a parse error or an error to
rethrow. If it has, assign an error code to mRv and return false.

The error will be reported in CompileScriptRunnable
https://searchfox.org/mozilla-central/rev/892475f3ba2b959aeaef19d1d8602494e3f2ae32/dom/workers/WorkerPrivate.cpp#417

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1831324 - Part 2: Check if classic worker script can be compiled.

According to the spec,
https://html.spec.whatwg.org/#run-a-worker

14. In both cases, let onComplete given script be the following steps:
15. 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.

Split EvaluateSourceBuffer into JS::Compile and JS_ExecuteScript.
So if we found the script cannot be compiled and it is a top-level script,
we assign a non-JS-Exception error code to it.

If the script cannot be compiled and it's not a top-level script, this
means it's a script which is loaded by importScripts(), according to the
spec, https://html.spec.whatwg.org/#import-scripts-into-worker-global-scope
the exception will be rethrown.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1831324 - Part 3: Throw a runtime error in AbstractWorker.onerror.js.

AbstractWorker.onerror.js has a SyntaxError, so according to the spec
"Event" should be fired instead of "ErrorEvent".

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1831324 - Part 4: Update test_importScripts_3rdparty.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Bug 1831324 - Part 5: Remove failed wpt ini files.

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Oh, asked HTML spec author about the correctness of the spec,
there is an open issue https://github.com/whatwg/html/issues/2289, which is discussing whether fire an Event or an ErrorEvent.
https://github.com/whatwg/html/issues/2289#issuecomment-1670679677

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] from comment #8)

Oh, asked HTML spec author about the correctness of the spec,
there is an open issue https://github.com/whatwg/html/issues/2289, which is discussing whether fire an Event or an ErrorEvent.
https://github.com/whatwg/html/issues/2289#issuecomment-1670679677

Domenic just confirms the spec is correct in https://github.com/whatwg/html/issues/2289#issuecomment-1676721412
Although he still doesn't explain the failed WPT test

https://wpt.fyi/results/workers?label=master&label=experimental&aligned&q=worker-parse-error-failure
Even Chrome only passes the WPTs in worker/modules
https://wpt.fyi/results/workers/modules?label=master&label=experimental&aligned&q=worker-parse-error-failure
For the classic worker tests, all browsers still failed.
https://wpt.fyi/results/workers?label=master&label=experimental&aligned&q=worker-parse-error-failure

AbstractWorker.onerror.html
https://wpt.fyi/results/workers/constructors/Worker/AbstractWorker.onerror.html?label=experimental&label=master&aligned
which is using ErrorEvent, but is passed in all browsers.

Hi smaug
So what do you think in this case?
You said maybe the spec is wrong, but now the spec author confirms the spec is correct......

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

"that spec is correct" :) That is just an opinion.
Domenic doesn't say what is correct and what is not. It is browser engine vendors who say, at least two out of tree. (He does represent one)

Using Event should be fine, but we need to ask other browser vendors if they are ok to change the behavior. It is after all a bit unusual to break a case where all the engines work the same way.
But in practice, since Google seems to favor Event, and so do we, we could just change the WPTs and our behavior.
And I don't think we ever report anything useful in these ErrorEvents so breaking existing websites should be unlikely.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1695ada26bb4
Part 1: Check if the module worker script has a parse error or an error to rethrow. r=jonco,dom-worker-reviewers,smaug
https://hg.mozilla.org/integration/autoland/rev/df2d96840c43
Part 2: Check if classic worker script can be compiled. r=dom-worker-reviewers,smaug
https://hg.mozilla.org/integration/autoland/rev/47a3e3dc971e
Part 3: Throw a runtime error in AbstractWorker.onerror.js. r=dom-worker-reviewers,smaug
https://hg.mozilla.org/integration/autoland/rev/14e55a816cfa
Part 4: Update test_importScripts_3rdparty. r=dom-worker-reviewers,smaug
https://hg.mozilla.org/integration/autoland/rev/ff922cf6a8ea
Part 5: Remove failed wpt ini files. r=dom-worker-reviewers,smaug

Comment 12

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/41484 for changes under testing/web-platform/tests

Comment 13

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/1695ada26bb4
https://hg.mozilla.org/mozilla-central/rev/df2d96840c43
https://hg.mozilla.org/mozilla-central/rev/47a3e3dc971e
https://hg.mozilla.org/mozilla-central/rev/14e55a816cfa
https://hg.mozilla.org/mozilla-central/rev/ff922cf6a8ea

Comment 15

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot