1362033 - clients-matchall-client-types.https.html times out if some assertions fail

Status: RESOLVED WONTFIX

(Core :: DOM: Service Workers, enhancement)

Description

[Ben Kelly [:bkelly, not reviewing]]

The clients-matchall-client-types.https.html WPT test times out if certain assertions fail.  We should fix this to report a failure instead.  My changes in bug 1293277 cause us to trigger these failures.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Make clients-matchall-client-types.https.html report failure instead of timeout when assertions fail. r=asuth

This patch just fixes some issues with the WPT test not reporting assertion failures correctly.  We should report the failure instead of causing a timeout.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8864522 [details] [diff] [review]
Make clients-matchall-client-types.https.html report failure instead of timeout when assertions fail. r=asuth

Actually, I have more changes in another patch I should include here.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8864522 [details] [diff] [review]
Make clients-matchall-client-types.https.html report failure instead of timeout when assertions fail. r=asuth

Actually, I will do the extra changes as a separate patch so the commit log is better when its upstreamed.  See previous comments for description of this patch.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Distinguish between undefined and missing elements in SharedWorker client property list. r=asuth

The test gets a Client object in the worker, creates an array, and postMessages it back to the main window like this:

          message.push([client.visibilityState,
                        client.focused,
                        client.url,
                        client.type,
                        frame_type]);

Things like client.visibilityState are undefined on a worker Client.

The test assertion was trying to compare the resulting structured clone array to empty fields.  Instead it should be comparing to undefined values for those array elements.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1079cb2a98d91c72507161d974efe5adf70feeaa

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8864530 [details] [diff] [review]
P2 Distinguish between undefined and missing elements in SharedWorker client property list. r=asuth

Review of attachment 8864530 [details] [diff] [review]:
-----------------------------------------------------------------

Restating: These changes are necessary because assert_array_equals uses hasOwnProperty(index) when comparing array contents.  It returns false on the sparse indices from omitted values, but true when there's an explicit undefined value, triggering the assertion.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8864522 [details] [diff] [review]
Make clients-matchall-client-types.https.html report failure instead of timeout when assertions fail. r=asuth

Review of attachment 8864522 [details] [diff] [review]:
-----------------------------------------------------------------

Restating the problem: The onmessage handler is not wrapped in step_func so the assert* calls produce and throw Errors but the test framework does not see the errors so no error is reported and done() is not automatically called.  Additionally, because the assert throws, the promise never gets (erroneously) resolved so the .then() chain in the test does not progress.

Although this change does not directly wrap onmessage in a step_func, it does reject the underlying error.  unreached_rejection(t) returns a function(error) {} that will massage and propagate the error, reporting it so the test completes.  I'm presuming the choice of catching and rejecting was made for consistency with calling reject() on any error string received over the MessageChannel.  (And noting that the second promise_test was missing the unreached_rejection capper, now resolved.)

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

I used try/catch because I don't really understand the step_func stuff.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Actually, some of this has changed already upstream:

https://github.com/w3c/web-platform-tests/commit/71f6f5417af2a43e62ddbbd3526748a1750e4a90

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

The patches here are no longer needed after the fixes uplifted from blink.  They made equivalent changes.