Bug 180444 - ServiceWorkers API should reject promises when calling objects inside detached frames
Summary: ServiceWorkers API should reject promises when calling objects inside detache...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-12-05 13:54 PST by Chris Dumez
Modified: 2017-12-06 16:25 PST (History)
7 users (show)

See Also:


Attachments
Patch (16.32 KB, patch)
2017-12-05 14:46 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (2.29 MB, application/zip)
2017-12-05 15:48 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (2.24 MB, application/zip)
2017-12-05 16:04 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews107 for mac-elcapitan-wk2 (2.53 MB, application/zip)
2017-12-05 16:11 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews113 for mac-elcapitan (3.45 MB, application/zip)
2017-12-05 16:26 PST, EWS Watchlist
no flags Details
Patch (22.05 KB, patch)
2017-12-05 20:02 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (22.39 KB, patch)
2017-12-05 20:36 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (22.75 KB, patch)
2017-12-05 20:46 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (27.88 KB, patch)
2017-12-06 09:59 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-12-05 13:54:22 PST
ServiceWorkers API should reject promises when called on objects inside detached frames. This is covered by:
- imported/w3c/web-platform-tests/service-workers/service-worker/detached-context.https.html
Comment 1 Chris Dumez 2017-12-05 14:46:04 PST
Created attachment 328503 [details]
Patch
Comment 2 EWS Watchlist 2017-12-05 15:48:17 PST
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
Comment 3 EWS Watchlist 2017-12-05 15:48:18 PST
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 4 EWS Watchlist 2017-12-05 16:04:08 PST
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
Comment 5 EWS Watchlist 2017-12-05 16:04:09 PST
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 6 EWS Watchlist 2017-12-05 16:11:28 PST
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
Comment 7 EWS Watchlist 2017-12-05 16:11:30 PST
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 8 EWS Watchlist 2017-12-05 16:26:07 PST
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
Comment 9 EWS Watchlist 2017-12-05 16:26:08 PST
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
Comment 10 Chris Dumez 2017-12-05 20:02:10 PST
Created attachment 328547 [details]
Patch
Comment 11 Chris Dumez 2017-12-05 20:36:06 PST
Created attachment 328550 [details]
Patch
Comment 12 Chris Dumez 2017-12-05 20:46:27 PST
Created attachment 328551 [details]
Patch
Comment 13 youenn fablet 2017-12-06 09:21:42 PST
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.
Comment 14 Chris Dumez 2017-12-06 09:59:47 PST
Created attachment 328579 [details]
Patch
Comment 15 WebKit Commit Bot 2017-12-06 10:40:09 PST
Comment on attachment 328579 [details]
Patch

Clearing flags on attachment: 328579

Committed r225577: <https://trac.webkit.org/changeset/225577>
Comment 16 WebKit Commit Bot 2017-12-06 10:40:11 PST
All reviewed patches have been landed.  Closing bug.
Comment 17 Radar WebKit Bug Importer 2017-12-06 16:25:09 PST
<rdar://problem/35895925>
Comment 18 Radar WebKit Bug Importer 2017-12-06 16:25:10 PST
<rdar://problem/35895926>