After microtask handling is adopted.
Created attachment 346718 [details] Patch
Created attachment 346719 [details] Patch
Created attachment 346720 [details] Patch
Created attachment 346721 [details] Patch
Created attachment 346970 [details] Patch
Created attachment 346971 [details] Patch
Created attachment 346972 [details] Patch
Created attachment 346973 [details] Patch
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
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
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
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
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
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
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
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
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
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
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
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
Created attachment 346985 [details] Patch
Created attachment 346986 [details] Patch
Created attachment 346992 [details] Patch
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.
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.
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.
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"
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.
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.
Committed r234846: <https://trac.webkit.org/changeset/234846>
<rdar://problem/43282647>
<rdar://problem/43282650>
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.
(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.
Committed r234854: <https://trac.webkit.org/changeset/234854>