Add WPTs for parse error handling in SharedWorkers #22185

elkurin · 2020-03-11T08:55:11Z

Corresponds to #5347

This pull request adds web-platform-tests to check if parse error events invoked by shared workers are successfully caught from the constructor.

annevk · 2020-03-11T12:51:14Z

I think we should have tests for multiple SharedWorker instances as well.

elkurin · 2020-03-12T07:13:42Z

As we discussed in #5347, the behavior of shared workers (not only about error handling) when the one shared worker is constructed several times simultaneously is not defined correctly in the spec because of the raciness you mentioned, so I'm not sure about adding the test cases with multiple SharedWorker instances.

Or did you mean differently? If so, I would appreciate if you give me a brief example.

workers/shared-worker-parse-error-failure.html

domenic · 2020-03-12T19:56:22Z

workers/shared-worker-parse-error-failure.html

+  const scriptURL = 'modules/resources/syntax-error.js';
+  const worker = new SharedWorker(scriptURL, { name: 'classic',
+                                               type: 'classic' });
+  return new Promise(resolve => worker.onerror = resolve);


It would be good to test that the value passed to onerror is an ErrorEvent. Also, worth testing that there are no other arguments, as window.onerror gets extra arguments but worker.onerror should not.

Maybe that is scope creep for this test... But if we don't want to test those things, then we should change the test description to say error event instead of ErrorEvent.

I think the event we get here is not defined as ErrorEvent in the spec but Event or ErrorEvent.
https://html.spec.whatwg.org/multipage/indices.html#event-error
I seems better to modify the description as you said.

FYI: FireFox returns Event here.
https://bugzilla.mozilla.org/show_bug.cgi?id=1610438

Event is correct. The spec says "fire an event" with no constructor arg, and step 1 of https://dom.spec.whatwg.org/#concept-event-fire says to use Event.

The index is a non-normative summary of every event named error in the entire HTML spec. You have to look at individual "fire an event" sites to see which constructor they use.

Upon reviewing the spec PR I think it's actually important that we test the type (and arguments) here. This is test of new functionality and it's easy to get it wrong. We should be sure that it gets test coverage.

I added the test to check if the event's type is 'error'.

Sorry, by "type" I actually meant "constructor". Please test its .constructor property, and also check the number of arguments sent to onerror.

Sorry for taking it in the wrong way. I modified as you suggested.
I found the test to check onerror for dedicated workers:
https://github.com/web-platform-tests/wpt/blob/master/workers/constructors/Worker/AbstractWorker.onerror.html
and it seems that this test should be also merged into wpt/workers/modules/dedicated-worker-parse-error-failure.html so that we can test parse error occurred in statically imported scripts (but not in this PR).

annevk · 2020-03-13T10:46:39Z

I guess I'm okay splitting things up.

domenic · 2020-03-13T14:23:11Z

workers/modules/shared-worker-parse-error-failure.html

+    worker.onerror = reject;
+  });
+  assert_array_equals(msg.data, ['export-on-load-script.js']);
+  if (msg.data.length == 1 && msg.data[0] === 'export-on-load-script.js') {


If the assert fails then this line will not be reached, so this if condition is redundant.

I think the best way to do this is to convert this all into a promise_setup().

promise_setup(async () => { const worker = new SharedWorker('resources/static-import-worker.js', { type: 'module' }); const supportsModuleWorkers = await new Promise((resolve, reject) => { worker.port.onmessage = e => resolve(e.data.length === 1 && e.data[0] == 'export-on-load-script.js'); worker.onerror = () => resolve(false); }); assert_precondition(supportsModuleWorkers, "Static import must be supported on module shared worker to run this test."); });

Then you can remove the assert_precondition from each individual test. Also it makes it so that there are only 2 tests, both with precondition failures, instead of 1 test with a failure and 2 tests with precondition failures.

Thanks. I modified in this way.

wpt-pr-bot · 2020-03-19T05:47:25Z

There are no reviewers for this pull request. Please reach out on W3C's irc server (irc.w3.org, port 6665) on channel #testing (web client) to get help with this. Thank you!

elkurin · 2020-03-19T07:58:35Z

@domenic Sorry, I did something wrong with with GIt commits, and got many non-related labels. Could remove those?

domenic · 2020-03-19T16:44:01Z

workers/modules/shared-worker-parse-error-failure.html

+  assert_equals(typeof args.a.message, 'string', 'Event.message');
+  assert_equals(args.a.filename, location.href + '/resources/syntax-error.js'),
+                'Event.filename');
+  assert_equals(args.a.lineno, 1, 'Event.lineno');


This doesn't seem right. Event does not have messsage, filename, or lineno properties. Does this test somehow pass in Chrome?

I'd expectt the test to check that these properties do not exist...

Sorry, I didn't understand the difference between Event and ErrorEvent.
Chromium doesn't have this message, filename or lineno value for shared workers but has on dedicated workers.
I removed it.

domenic · 2020-03-19T16:45:10Z

workers/modules/shared-worker-parse-error-failure.html

+  const scriptURL = 'resources/syntax-error.js';
+  const worker = new SharedWorker(scriptURL, { type: 'module' });
+  const args = await new Promise(resolve =>
+      worker.onerror = (a, b, c) => resolve({a, b, c}));


This could be a little simpler as

const args = await new Promise(resolve => worker.onerror = (...args) => resolve(args));

and then args will be an array, which avoids the awkward a/b/c properties and lets the checkArguments function do something like assert_equals(args.length, 1).

Thanks. Modified.

domenic · 2020-03-19T16:46:57Z

workers/modules/shared-worker-parse-error-failure.html

+  );
+});
+
+const checkArguments = (args) => {


To deduplicate this you may want to move it to a file in workers/resources, change it to window.checkErrorArguments = ..., and then use e.g. <script src="resources/check-error-arguments.js"></script> to get the function in both files.

I created workers/support/check-error-arguments.js.

domenic

LGTM with nits. Thank you for your patience!

domenic · 2020-03-23T17:12:29Z

workers/modules/shared-worker-parse-error-failure.html

+    worker.port.onmessage = e => {
+      resolve(e.data.length == 1 && e.data[0] == 'export-on-load-script.js');
+    };
+    worker.onerror = () => reject(false);


Suggested change

worker.onerror = () => reject(false);

worker.onerror = () => resolve(false);

domenic · 2020-03-23T17:13:26Z

workers/support/check-error-arguments.js

+window.checkErrorArguments = args => {
+  assert_equals(args.length, 1);
+  assert_equals(args[0].constructor, Event);
+}


Suggested change

}

};

elkurin · 2020-03-23T17:20:23Z

Thanks so much @domenic for a lot of comments!

This CL handles parse error events in modules shared workers. By this change, parse error events invoked by top-level scripts and statically imported scripts can be handled by AbstractWorker.onerror. The HTML spec defines script parsing should occur during the fetch step and invoke a parse error event at that point if needed. This CL obeys this behavior for module script workers, but not for classic script workers because of an implementation reason that classic script workers are supposed to parse the script during the evaluation step. Therefore, the timing to catch parse error events differs. In this CL, parse error handling is only implemented for module shared workers, not for classic. This is discussed in the html spec issue: whatwg/html#5323 The wpt to check this feature will be added from external github: web-platform-tests/wpt#22185 Bug: 1058259 Change-Id: I2397a7de8e2ae732fb0b29aea8d8703dd2a79a05 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2100058 Reviewed-by: Hiroki Nakagawa <nhiroki@chromium.org> Commit-Queue: Eriko Kurimoto <elkurin@google.com> Cr-Commit-Position: refs/heads/master@{#753185}

wpt-pr-bot added wg-webapps workers labels Mar 11, 2020

wpt-pr-bot assigned jdm Mar 11, 2020

wpt-pr-bot requested review from annevk, caitp, jdm and zqzhang March 11, 2020 08:55

elkurin mentioned this pull request Mar 11, 2020

Let the SharedWorker constructor queue a task to fire an error when parse error occurs whatwg/html#5347

Merged

3 tasks

domenic reviewed Mar 12, 2020

View reviewed changes

domenic reviewed Mar 13, 2020

View reviewed changes

domenic mentioned this pull request Mar 13, 2020

new SharedWorker() should queue a task to fire an error when parse error occurs whatwg/html#5323

Open

moz-wptsync-bot added the mozilla:gecko-blocked label Mar 19, 2020

wpt-pr-bot added assets client-hints compat content-security-policy cookies css-animations css-borders css-cascade css-color css-color-adjust css-flexbox css-position css-text css-transforms css-transitions labels Mar 19, 2020

wpt-pr-bot requested review from astearns, bakulf, birtles, cbiesinger, cdumez, clelland, darrnshn, dbaron, deniak, dirkschulze, domfarolino and fantasai March 19, 2020 05:18

elkurin force-pushed the parse-error-in-shared-worker branch from 964a8a3 to f0ed797 Compare March 19, 2020 05:47

wpt-pr-bot added the status:needs-reviewers label Mar 19, 2020

Eriko Kurimoto added 2 commits March 19, 2020 16:49

shared-worker-parse-error-failure.html

482bc63

Parse error handling in shared workers

c97fe3d

bakulf approved these changes Mar 19, 2020

View reviewed changes

elkurin force-pushed the parse-error-in-shared-worker branch from f0ed797 to c97fe3d Compare March 19, 2020 07:52

moz-wptsync-bot removed the mozilla:gecko-blocked label Mar 19, 2020

domenic requested changes Mar 19, 2020

View reviewed changes

separate files

06d8c42

domenic approved these changes Mar 23, 2020

View reviewed changes

minor change

22d700a

dbaron removed their request for review March 23, 2020 17:22

domenic merged commit bd6c94e into web-platform-tests:master Mar 23, 2020

hiroshige-g mentioned this pull request Sep 7, 2022

Fire an ErrorEvent instead an Event whatwg/html#2289

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WPTs for parse error handling in SharedWorkers #22185

Add WPTs for parse error handling in SharedWorkers #22185

elkurin commented Mar 11, 2020 •

edited

annevk commented Mar 11, 2020

elkurin commented Mar 12, 2020

domenic Mar 12, 2020

elkurin Mar 13, 2020

elkurin Mar 13, 2020

domenic Mar 13, 2020

domenic Mar 13, 2020

elkurin Mar 17, 2020

domenic Mar 17, 2020

elkurin Mar 19, 2020 •

edited

annevk commented Mar 13, 2020

domenic Mar 13, 2020

elkurin Mar 17, 2020

wpt-pr-bot commented Mar 19, 2020

elkurin commented Mar 19, 2020

domenic Mar 19, 2020

elkurin Mar 23, 2020

domenic Mar 19, 2020

elkurin Mar 23, 2020

domenic Mar 19, 2020

elkurin Mar 23, 2020

domenic left a comment

domenic Mar 23, 2020

domenic Mar 23, 2020

elkurin commented Mar 23, 2020

	worker.onerror = () => reject(false);
	worker.onerror = () => resolve(false);

Add WPTs for parse error handling in SharedWorkers #22185

Add WPTs for parse error handling in SharedWorkers #22185

Conversation

elkurin commented Mar 11, 2020 • edited

annevk commented Mar 11, 2020

elkurin commented Mar 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elkurin Mar 19, 2020 • edited

Choose a reason for hiding this comment

annevk commented Mar 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpt-pr-bot commented Mar 19, 2020

elkurin commented Mar 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elkurin commented Mar 23, 2020

elkurin commented Mar 11, 2020 •

edited

elkurin Mar 19, 2020 •

edited