[webkit-reviews] review granted: [Bug 180444] ServiceWorkers API should reject promises when calling objects inside detached frames : [Attachment 328551] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 180444: ServiceWorkers API should reject promises when calling objects
inside detached frames
https://bugs.webkit.org/show_bug.cgi?id=180444

Attachment 328551: Patch

https://bugs.webkit.org/attachment.cgi?id=328551&action=review




--- Comment #13 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 328551
  --> https://bugs.webkit.org/attachment.cgi?id=328551
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.