Summary: | ServiceWorkers API should reject promises when calling objects inside detached frames | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | beidson, commit-queue, ews-watchlist, ggaren, rniwa, webkit-bug-importer, youennf | ||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2017-12-05 13:54:22 PST
Created attachment 328503 [details]
Patch
Comment on attachment 328503 [details] Patch Attachment 328503 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/5507083 New failing tests: fast/dom/navigator-detached-no-crash.html Created attachment 328511 [details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 328503 [details] Patch Attachment 328503 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/5507054 New failing tests: imported/w3c/web-platform-tests/service-workers/service-worker/ready.https.html Created attachment 328516 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 328503 [details] Patch Attachment 328503 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/5507176 New failing tests: fast/dom/navigator-detached-no-crash.html imported/w3c/web-platform-tests/service-workers/service-worker/ready.https.html http/tests/media/media-stream/disconnected-frame.html Created attachment 328520 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 328503 [details] Patch Attachment 328503 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/5507132 New failing tests: fast/dom/navigator-detached-no-crash.html Created attachment 328522 [details]
Archive of layout-test-results from ews113 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 328547 [details]
Patch
Created attachment 328550 [details]
Patch
Created attachment 328551 [details]
Patch
Comment on attachment 328551 [details] Patch LGTM. Win bot build error might be related to the patch though. View in context: https://bugs.webkit.org/attachment.cgi?id=328551&action=review > Source/WebCore/bindings/js/JSDOMPromiseDeferred.h:269 > JSC::JSPromiseDeferred* promiseDeferred = JSC::JSPromiseDeferred::create(&state, &globalObject); Let's have callerGlobalObject return a JSDOMGlobalObject& so that we can write the promise creation as a oneliner. > Source/WebCore/bindings/js/JSDOMWindowBase.h:104 > +WEBCORE_EXPORT JSC::JSGlobalObject& callerGlobalObject(JSC::ExecState&); Can we move it to JSDOMGlobalObject.h instead? > Source/WebCore/workers/service/ServiceWorkerContainer.cpp:114 > + if (!context || !context->sessionID().isValid() || m_isStopped) { I think m_isStopped is set to true sooner than context is made null. So probably if (m_isStopped || !context->sessionID().isValid()) is sufficient. Ditto below. > LayoutTests/fast/dom/navigator-detached-no-crash-expected.txt:1 > +CONSOLE MESSAGE: Unhandled Promise Rejection: TypeError: Not enough arguments Since we control this test, we might want to catch the promise rejection in the test itself. Either when calling the promise function or just by adding an 'unhandledrejection' event listener. These error messages are sometimes making the tests flaky. Also that would remove the need to rebase expected.txt files. Created attachment 328579 [details]
Patch
Comment on attachment 328579 [details] Patch Clearing flags on attachment: 328579 Committed r225577: <https://trac.webkit.org/changeset/225577> All reviewed patches have been landed. Closing bug. |