Bug 188265 – Worker should support unhandled promise rejections

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Yusuke Suzuki]

After microtask handling is adopted.

[Yusuke Suzuki]

Created attachment 346718 [details]
Patch

[Yusuke Suzuki]

Created attachment 346719 [details]
Patch

[Yusuke Suzuki]

Created attachment 346720 [details]
Patch

[Yusuke Suzuki]

Created attachment 346721 [details]
Patch

[Yusuke Suzuki]

Created attachment 346970 [details]
Patch

[Yusuke Suzuki]

Created attachment 346971 [details]
Patch

[Yusuke Suzuki]

Created attachment 346972 [details]
Patch

[Yusuke Suzuki]

Created attachment 346973 [details]
Patch

[EWS Watchlist]

Comment on attachment 346973 [details]
Patch

Attachment 346973 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/8832122

New failing tests:
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/ecdh_bits.https.worker.html
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/ecdh_keys.https.worker.html

[EWS Watchlist]

Created attachment 346975 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 346973 [details]
Patch

Attachment 346973 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8832181

New failing tests:
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/ecdh_bits.https.worker.html
imported/w3c/web-platform-tests/streams/readable-streams/tee.serviceworker.https.html
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/ecdh_keys.https.worker.html

[EWS Watchlist]

Created attachment 346976 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[EWS Watchlist]

Comment on attachment 346973 [details]
Patch

Attachment 346973 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8832183

New failing tests:
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/ecdh_bits.https.worker.html
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/ecdh_keys.https.worker.html

[EWS Watchlist]

Created attachment 346977 [details]
Archive of layout-test-results from ews115 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 346973 [details]
Patch

Attachment 346973 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/8832662

New failing tests:
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/ecdh_bits.https.worker.html
imported/w3c/web-platform-tests/streams/readable-streams/tee.serviceworker.https.html
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/ecdh_keys.https.worker.html

[EWS Watchlist]

Created attachment 346980 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 346973 [details]
Patch

Attachment 346973 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/8832686

New failing tests:
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/ecdh_bits.https.worker.html
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/ecdh_keys.https.worker.html

[EWS Watchlist]

Created attachment 346982 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 346973 [details]
Patch

Attachment 346973 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/8832700

New failing tests:
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/ecdh_bits.https.worker.html
imported/w3c/web-platform-tests/streams/readable-streams/tee.serviceworker.https.html
imported/w3c/web-platform-tests/WebCryptoAPI/derive_bits_keys/ecdh_keys.https.worker.html

[EWS Watchlist]

Created attachment 346983 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Yusuke Suzuki]

Created attachment 346985 [details]
Patch

[Yusuke Suzuki]

Created attachment 346986 [details]
Patch

[Yusuke Suzuki]

Created attachment 346992 [details]
Patch

[Darin Adler]

Comment on attachment 346992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346992&action=review

> Source/WebCore/dom/Microtasks.h:73
> +    WEBCORE_EXPORT static MicrotaskQueue& queueForScriptExecutionContext(ScriptExecutionContext*);

This should take a reference, not a pointer. I believe it does not allow passing a nullptr.

I’m not a huge fan of writing out the entire class name of an argument as part of a function name: it’s common in Objective-C but doesn’t seem right in C++ where we have much stronger type checking. I could imagine just naming this function "queue" or "contextQueue" or maybe "queueForContext" or "queueForExecutionContext".

> Source/WebCore/workers/WorkerThread.cpp:304
> +            workerGlobalScope.removeRejectedPromiseTracker();

I’m starting to think that this long list of things to stop in WorkerGlobalScope might belong inside that class, not in this WorkerThread function.

[Joseph Pecoraro]

Comment on attachment 346992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346992&action=review

Awesome! r=me as well.

> Source/WebCore/ChangeLog:51
> +        * dom/ScriptExecutionContext.cpp:
> +        (WebCore::ScriptExecutionContext::removeRejectedPromiseTracker):
> +        * dom/ScriptExecutionContext.h:

This one could use an explanation. We don't do this teardown in documents but you are doing it in workers. Was there a particular cycle this prevents?

> Source/WebCore/html/HTMLAttributeNames.in:273
> +onrejectionhandled

I thought this file was specific to element attribute names. Meaning cases like `<element attrname="...">`.

Is it the case that onrejectionhandled/onunhandledrejection can be attribute names on a particular element? Or am I mis-understanding this file's purpose?

> LayoutTests/http/wpt/workers/promise-unhandled-rejection.any.worker-expected.txt:1
> +CONSOLE MESSAGE: line 1774: TypeError: null is not an object (evaluating 'this.message_target.removeEventListener')

Interesting, do you know what this message_target is and why we are failing these tests? Can always be done in a follow-up for sure, but its just weird.

[Darin Adler]

Comment on attachment 346992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346992&action=review

>> Source/WebCore/html/HTMLAttributeNames.in:273
>> +onrejectionhandled
> 
> I thought this file was specific to element attribute names. Meaning cases like `<element attrname="...">`.
> 
> Is it the case that onrejectionhandled/onunhandledrejection can be attribute names on a particular element? Or am I mis-understanding this file's purpose?

Oh, good point. I am not sure I understand why these needed to be added.

[Darin Adler]

Comment on attachment 346992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346992&action=review

> LayoutTests/http/wpt/workers/promise-unhandled-rejection.any.js:14
> +}, "UnhaneldRejection event occurs if a rejected promise is not handled.");

Just spotted a typo here: "Unhaneld"

[Yusuke Suzuki]

Comment on attachment 346992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346992&action=review

>> Source/WebCore/ChangeLog:51
>> +        * dom/ScriptExecutionContext.h:
> 
> This one could use an explanation. We don't do this teardown in documents but you are doing it in workers. Was there a particular cycle this prevents?

In Workers, VM bound to the worker will be destroyed when the worker finishes. So before destroying the VM, we need to destroy RejectedPromiseTracker.
Otherwise, RejectedPromiseTracker requires VM in its destruction. This is why we have the teardown procedure for workers.

>> Source/WebCore/dom/Microtasks.h:73
>> +    WEBCORE_EXPORT static MicrotaskQueue& queueForScriptExecutionContext(ScriptExecutionContext*);
> 
> This should take a reference, not a pointer. I believe it does not allow passing a nullptr.
> 
> I’m not a huge fan of writing out the entire class name of an argument as part of a function name: it’s common in Objective-C but doesn’t seem right in C++ where we have much stronger type checking. I could imagine just naming this function "queue" or "contextQueue" or maybe "queueForContext" or "queueForExecutionContext".

Make sense! I like contextQueue :) Renamed.
And passing reference here :)

>>> Source/WebCore/html/HTMLAttributeNames.in:273
>>> +onrejectionhandled
>> 
>> I thought this file was specific to element attribute names. Meaning cases like `<element attrname="...">`.
>> 
>> Is it the case that onrejectionhandled/onunhandledrejection can be attribute names on a particular element? Or am I mis-understanding this file's purpose?
> 
> Oh, good point. I am not sure I understand why these needed to be added.

Ah, right. It is unnecessary since they are only in Window/Worker. Removed.

>> Source/WebCore/workers/WorkerThread.cpp:304
>> +            workerGlobalScope.removeRejectedPromiseTracker();
> 
> I’m starting to think that this long list of things to stop in WorkerGlobalScope might belong inside that class, not in this WorkerThread function.

Sounds super nice! I've added `WorkerGlobalScope::prepareForTermination` and move these things to that function.

>> LayoutTests/http/wpt/workers/promise-unhandled-rejection.any.worker-expected.txt:1
>> +CONSOLE MESSAGE: line 1774: TypeError: null is not an object (evaluating 'this.message_target.removeEventListener')
> 
> Interesting, do you know what this message_target is and why we are failing these tests? Can always be done in a follow-up for sure, but its just weird.

This "message_target" is in WPT test harness. I guess this is a bug in WPT test harness, which shows this failure if worker reports unhandledrejection.

[Yusuke Suzuki]

Comment on attachment 346992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346992&action=review

>>> LayoutTests/http/wpt/workers/promise-unhandled-rejection.any.worker-expected.txt:1
>>> +CONSOLE MESSAGE: line 1774: TypeError: null is not an object (evaluating 'this.message_target.removeEventListener')
>> 
>> Interesting, do you know what this message_target is and why we are failing these tests? Can always be done in a follow-up for sure, but its just weird.
> 
> This "message_target" is in WPT test harness. I guess this is a bug in WPT test harness, which shows this failure if worker reports unhandledrejection.

Ah, I've found the bug. Our worker has a bug in reporting error messages to console.
WorkerGlobalScope::logExceptionToConsole emits error event to the host Worker object by using WorkerMessagingProxy::postExceptionToWorkerObject.
This code would be good for usual JS exception. But it is not correct for reporting "Unhandled Promise Rejection" to the host console.
Since this is existing bug, I'll fix this in a subsequent patch.

[Yusuke Suzuki]

Committed r234846: <https://trac.webkit.org/changeset/234846>

[Radar WebKit Bug Importer]

<rdar://problem/43282647>

[Radar WebKit Bug Importer]

<rdar://problem/43282650>

[Darin Adler]

Comment on attachment 346992 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=346992&action=review

A couple very small things I noticed after this was landed.

> Source/WebCore/bindings/js/JSExecState.cpp:39
>      ScriptExecutionContext* context = scriptExecutionContextFromExecState(exec);

Tiny picky point: If this result is guaranteed to be non-null, it would be great to put it in a reference rather than a pointer. All the other call sites for scriptExecutionContextFromExecState check for null, so this one is unusual in that it knows it’s not going to be null. Almost worth a comment explaining why we know. Or could consider changing this to silently do nothing in that case. Or possibly do all three: write comment about why it’s not null, assert that it’s not null, and silently do nothing in the release build.

> Source/WebCore/dom/Microtasks.cpp:59
> +    ASSERT(context->isWorkerGlobalScope());
> +    return downcast<WorkerGlobalScope>(*context).microtaskQueue();

This assertion is the same check that downcast does. There’s no need to assert just before calling downcast since that’s the entire purpose of downcast, to build the assertion into the cast.

[Yusuke Suzuki]

(In reply to Darin Adler from comment #33)
> Comment on attachment 346992 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346992&action=review
> 
> A couple very small things I noticed after this was landed.

Thank you, I'll land a follow-up patch to fix these points.

> 
> > Source/WebCore/bindings/js/JSExecState.cpp:39
> >      ScriptExecutionContext* context = scriptExecutionContextFromExecState(exec);
> 
> Tiny picky point: If this result is guaranteed to be non-null, it would be
> great to put it in a reference rather than a pointer. All the other call
> sites for scriptExecutionContextFromExecState check for null, so this one is
> unusual in that it knows it’s not going to be null. Almost worth a comment
> explaining why we know. Or could consider changing this to silently do
> nothing in that case. Or possibly do all three: write comment about why it’s
> not null, assert that it’s not null, and silently do nothing in the release
> build.

I think silently doing nothing is OK. When ScriptExecutionContext is gone, we do not need to drain microtasks.
Fixed.

> 
> > Source/WebCore/dom/Microtasks.cpp:59
> > +    ASSERT(context->isWorkerGlobalScope());
> > +    return downcast<WorkerGlobalScope>(*context).microtaskQueue();
> 
> This assertion is the same check that downcast does. There’s no need to
> assert just before calling downcast since that’s the entire purpose of
> downcast, to build the assertion into the cast.

Nice catch. Removed.

[Yusuke Suzuki]

Committed r234854: <https://trac.webkit.org/changeset/234854>