[webkit-reviews] review granted: [Bug 239122] Shared workers should match service worker registrations : [Attachment 457680] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has granted youenn fablet <youennf at gmail.com>'s
request for review:
Bug 239122: Shared workers should match service worker registrations
https://bugs.webkit.org/show_bug.cgi?id=239122

Attachment 457680: Patch

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




--- Comment #6 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 457680
  --> https://bugs.webkit.org/attachment.cgi?id=457680
Patch

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

r=me

> Source/WebCore/workers/service/SWClientConnection.cpp:134
> +	   worker->postTaskToWorkerGlobalScope([message = WTFMove(message),
sourceData = WTFMove(sourceData), sourceOrigin =
WTFMove(sourceOrigin).isolatedCopy()] (auto& context) mutable {

WTFMove(sourceData).isolatedCopy()

Let the optimization in `ServiceWorkerData isolatedCopy() &&;` determine if
moving is safe.

> Source/WebCore/workers/service/SWClientConnection.cpp:141
> +	   sharedWorker->thread().runLoop().postTask([message =
WTFMove(message), sourceData = WTFMove(sourceData), sourceOrigin =
WTFMove(sourceOrigin).isolatedCopy()] (auto& context) mutable {

WTFMove(sourceData).isolatedCopy()

> Source/WebCore/workers/shared/context/SharedWorkerThreadProxy.cpp:54
> +    static NeverDestroyed<HashMap<ScriptExecutionContextIdentifier,
SharedWorkerThreadProxy*>> map;

Should probably use MainThreadNeverDestroyed.

> Source/WebCore/workers/shared/context/SharedWorkerThreadProxy.cpp:93
> +    , m_clientIdentifier(*initializationData.clientIdentifier)

use-after-move of initializationData. While this may work in practice, it looks
unsafe and should be avoided.

> Source/WebCore/workers/shared/context/SharedWorkerThreadProxy.h:88
> +    ScriptExecutionContextIdentifier m_clientIdentifier;

I am a bit unclear about what this means. A sharedWorker doesn't necessarily
have a single client.