Bug 205451 – Add a few more crossThreadCopy when hopping threads

• Status: NEW
• Component: Service Workers

[youenn fablet]

Add a few more crossThreadCopy when hopping threads

[youenn fablet]

Created attachment 386103 [details]
Patch

[Chris Dumez]

Comment on attachment 386103 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        This should not change behavior since these structures come straight from IPC.

It is not clear to me why we want to do this then. Do we see crashes or not?

[youenn fablet]

(In reply to Chris Dumez from comment #2)
> Comment on attachment 386103 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386103&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        This should not change behavior since these structures come straight from IPC.
> 
> It is not clear to me why we want to do this then. Do we see crashes or not?

We see crashes but I do not think these will fix them.
That said, this seems safer and future proof to have these cross thread copies that we could optimise if we want to.

[Fujii Hironori]

Comment on attachment 386103 [details]
Patch

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

> Source/WebCore/workers/service/context/SWContextManager.cpp:83
> +static inline ServiceWorkerOrClientData crossThreadCopy(ServiceWorkerOrClientData&& data)

It might be better to have CrossThreadCopierBase specialization for Variant.

> Source/WebCore/workers/service/context/SWContextManager.cpp:99
> +    serviceWorker->thread().runLoop().postTask([serviceWorker = makeRef(*serviceWorker), message = WTFMove(message), sourceData = crossThreadCopy(WTFMove(sourceData))](auto&) mutable {

crossThreadCopy(WTFMove(something)) is a good pattern to transfer data cross-threads safely, which introduced by my change Bug 198957.
It would be better to use crossThreadCopy for 'message'.

[Fujii Hironori]

I will check the WinCairo EWS failure.

> C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/Variant.h(1048): error C2338: For conversion construction of variants, exactly one type must be constructible

[Fujii Hironori]

UnifiedSource-f74e0903-5.cpp unified both WorkerSWClientConnection.cpp and SWContextManager.cpp.
Then, crossThreadCopy of SWContextManager.cpp is used in WorkerSWClientConnection.cpp.

[Fujii Hironori]

Created attachment 386375 [details]
Patch

[youenn fablet]

(In reply to Fujii Hironori from comment #4)
> Comment on attachment 386103 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=386103&action=review
> 
> > Source/WebCore/workers/service/context/SWContextManager.cpp:83
> > +static inline ServiceWorkerOrClientData crossThreadCopy(ServiceWorkerOrClientData&& data)
> 
> It might be better to have CrossThreadCopierBase specialization for Variant.

Your change looks good. Not sure I can r+ it though!

Moving a Variant does not let the Variant in any good state, similarly to std::optional. I am not sure we want a generic crossThreadCopy(Variant&&), or at least we want to be very cautious when using it.

> 
> > Source/WebCore/workers/service/context/SWContextManager.cpp:99
> > +    serviceWorker->thread().runLoop().postTask([serviceWorker = makeRef(*serviceWorker), message = WTFMove(message), sourceData = crossThreadCopy(WTFMove(sourceData))](auto&) mutable {
> 
> crossThreadCopy(WTFMove(something)) is a good pattern to transfer data
> cross-threads safely, which introduced by my change Bug 198957.

Sure, that is a bit more code but would be more efficient.

> It would be better to use crossThreadCopy for 'message'.

I believe message does not need that since it is isolated by design.

[Fujii Hironori]

Comment on attachment 386103 [details]
Patch

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

>>> Source/WebCore/workers/service/context/SWContextManager.cpp:99
>>> +    serviceWorker->thread().runLoop().postTask([serviceWorker = makeRef(*serviceWorker), message = WTFMove(message), sourceData = crossThreadCopy(WTFMove(sourceData))](auto&) mutable {
>> 
>> crossThreadCopy(WTFMove(something)) is a good pattern to transfer data cross-threads safely, which introduced by my change Bug 198957.
>> It would be better to use crossThreadCopy for 'message'.
> 
> Sure, that is a bit more code but would be more efficient.

It'd better to add &&-qualified isolatedCopy() methods for ServiceWorkerData and ServiceWorkerClientData.

[Fujii Hironori]

(In reply to youenn fablet from comment #8)
> > It would be better to use crossThreadCopy for 'message'.
> 
> I believe message does not need that since it is isolated by design.

Oh, you are right.