[testharness.js] Report error for worker errors

[zcorpan]

In resources/test/tests/functional/worker-dedicated.html remote_done
ends up being called twice, and the second time message_target is
null, which causes an exception to be thrown.

[Ms2ger]

remote_done is called once from the complete message (which is sent because of the "error" event dispatched in the worker) and then another time because another "error" event is dispatched on the Worker. Alternative fix:

--- a/resources/testharness.js
+++ b/resources/testharness.js
@@ -2982,16 +2982,17 @@ policies and contribution forms [3].
                 test.phase = test.phases.HAS_RESULT;
                 test.done();
             } else if (!tests.allow_uncaught_exception) {
                 tests.status.status = tests.status.ERROR;
                 tests.status.message = e.message;
                 tests.status.stack = stack;
             }
             done();
+            e.preventDefault();
         };
 
         addEventListener("error", error_handler, false);
         addEventListener("unhandledrejection", function(e){ error_handler(e.reason); }, false);
     }
 
     test_environment.on_tests_ready();
 

[zcorpan]

Thanks, used that instead.

[zcorpan]

@Ms2ger ping

[gsnedders]

I think that looks good?

[gsnedders]

blah

[wpt-pr-bot]

There are no owners 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!

[Ms2ger]

This seems like it may be genuine:

::Dedicated Worker Tests <- . FAILED                                     [ 51%]
E     Differing items:
E     {'summarized_status': {'message': None, 'status_string': 'TIMEOUT'}} != {'summarized_status': {'message': 'Error: This failure is expected.', 'status_string': 'ERROR'}}
E     Full diff:
E     - {u'summarized_status': {u'message': None, u'status_string': u'TIMEOUT'},
E     + {u'summarized_status': {u'message': u'Error: This failure is expected.',
E     +                         u'status_string': u'ERROR'},

[zcorpan]

I think what happens is that the error worker runs first and sets the harness status to ERROR. Fix for that is #11405

[zcorpan]

@jugglinmike see #11400 (comment)

[jugglinmike]

It looks as though the behavior in master and the test are both incorrect.

In master, the script worker-error.js produces a harness state of "ERROR".
I believe thie is a mistake. While it's true that the script produces an
uncaught exception, it does this after loading testharness.js and before
defining other tests. That means testharness.js should consider it a failing
single-page test, and that should not cause a harness error. Complicating
matters is the fact that the script is executed alongside another worker
executing worker.js. That second script is expected to time out.

To summarize: In master, we're seeing an OK (from a single-page test in one
worker) and a TIMEOUT (from a typical test in another worker) incorrectly
reported as an ERROR.

To fix the tests, I've added a sub-test to worker-error.js so that it
actually produces a harness error. I've created another worker script named
worker-uncaught-single.js for use in a new test. The new test verifies that
uncaught errors in workers which are single-page tests do not cause a harness
error.

Although @Ms2ger's recommended solution satisfies both of these tests, I have
also modified the change to testharness.js itself. Rather than preventing
event bubbling from the workers, I'm suggesting that we detach the onerror
event handler immediately following an uncaught error. This approach seems
preferable because it is localized to the situation which motivates it. The
removal of an uneeded event handler is also slightly more hygenic, and it's in
keeping with similar "cleanup" logic that is already present in the
remote_done method.

@zcorpan will be out of office for a few more weeks. @Ms2ger Would you mind
taking another look?

[Ms2ger]

Do squash the commits before merging, and update the PR title.

[Ms2ger]

therefore

[jugglinmike]

Thanks, @Ms2ger. Typically, I would land this as a series of distinct commits to preserve authorship information. However, because the final changeset does not contain @zcorpan's modification, I've squashed it down into a single commit. And because GitHub.com has been configured to disallow commits pushed from local repositories, I've force-pushed the result to this branch.

For posterity, the pre-squashed version is available here:

zcorpan/remote-done-twice-error...bocoup:zcorpan/remote-done-twice-error

[jugglinmike]

Looks like I forgot to use git's --author flag when squashing, so the commit in master is incorrectly attributed to @zcorpan. Sorry about that, Simon!