Created attachment 457327 [details]
Patch

Created attachment 457605 [details]
Patch

Created attachment 457619 [details]
Patch

Created attachment 457631 [details]
Patch

Created attachment 457680 [details]
Patch

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.

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

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

(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?

<rdar://problem/91954369>

> Maybe it should be m_contextIdentifier?

Sounds good.

Created attachment 458056 [details]
Patch for landing

Created attachment 458130 [details]
Fix remote worker process change

Created attachment 458139 [details]
Fix remote worker process change

Created attachment 458144 [details]
Rebasing

Created attachment 458347 [details]
Patch

Created attachment 458351 [details]
Rebasing

Created attachment 458353 [details]
Fix build

Created attachment 458420 [details]
Fix WinCairo

Created attachment 458423 [details]
Fix WinCairo

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