Bug 239122 – Shared workers should match service worker registrations

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

Shared workers should match service worker registrations

[youenn fablet]

Created attachment 457327 [details]
Patch

[youenn fablet]

Created attachment 457605 [details]
Patch

[youenn fablet]

Created attachment 457619 [details]
Patch

[youenn fablet]

Created attachment 457631 [details]
Patch

[youenn fablet]

Created attachment 457680 [details]
Patch

[Chris Dumez]

Comment on attachment 457680 [details]
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.

[youenn fablet]

> > 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.

It is the id of the shared worker context, exposed as client.id.

[Chris Dumez]

(In reply to youenn fablet from comment #7)
> > > 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.
> 
> It is the id of the shared worker context, exposed as client.id.

I see but the naming is confusing in the context of a shared worker when we say "client". I thought you meant client of the shared worker. When you meant that the shared worker is the client of the service worker. Not sure how to clarify though.

[Chris Dumez]

(In reply to Chris Dumez from comment #8)
> (In reply to youenn fablet from comment #7)
> > > > 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.
> > 
> > It is the id of the shared worker context, exposed as client.id.
> 
> I see but the naming is confusing in the context of a shared worker when we
> say "client". I thought you meant client of the shared worker. When you
> meant that the shared worker is the client of the service worker. Not sure
> how to clarify though.

Maybe it should be m_contextIdentifier?

[Radar WebKit Bug Importer]

<rdar://problem/91954369>

[youenn fablet]

> Maybe it should be m_contextIdentifier?

Sounds good.

[youenn fablet]

Created attachment 458056 [details]
Patch for landing

[youenn fablet]

Created attachment 458130 [details]
Fix remote worker process change

[youenn fablet]

Created attachment 458139 [details]
Fix remote worker process change

[youenn fablet]

Created attachment 458144 [details]
Rebasing

[youenn fablet]

Created attachment 458347 [details]
Patch

[youenn fablet]

Created attachment 458351 [details]
Rebasing

[youenn fablet]

Created attachment 458353 [details]
Fix build

[youenn fablet]

Created attachment 458420 [details]
Fix WinCairo

[youenn fablet]

Created attachment 458423 [details]
Fix WinCairo

[EWS]

Committed r293501 (250032@main): <https://commits.webkit.org/250032@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 458423 [details].