Bug 233600 – [WebGPU] Hook up StreamServerConnection to Remote*** classes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGPU

[Myles C. Maxfield]

[WebGPU] Hook up StreamServerConnection to Remote*** classes

[Myles C. Maxfield]

Created attachment 445354 [details]
Patch

[Myles C. Maxfield]

Comment on attachment 445354 [details]
Patch

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

> Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteGPUProxy.cpp:51
> +    m_gpuProcessConnection->messageReceiverMap().addMessageReceiver(Messages::RemoteGPUProxy::messageReceiverName(), identifier.toUInt64(), *this);

The connection().send(Messages::GPUConnectionToWebProcess::CreateRemoteGPU()) call is coming in the next patch.

[Myles C. Maxfield]

Comment on attachment 445354 [details]
Patch

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

>> Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteGPUProxy.cpp:51
>> +    m_gpuProcessConnection->messageReceiverMap().addMessageReceiver(Messages::RemoteGPUProxy::messageReceiverName(), identifier.toUInt64(), *this);
> 
> The connection().send(Messages::GPUConnectionToWebProcess::CreateRemoteGPU()) call is coming in the next patch.

(The next patch is https://bugs.webkit.org/show_bug.cgi?id=233619 which depends on this one.)

[Kimmo Kinnunen]

Comment on attachment 445354 [details]
Patch

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

Looks good with the comments addressed

> Source/WebKit/GPUProcess/graphics/WebGPU/RemoteGPU.cpp:56
> +    initialize();

here the virtual function table is not initialised yet, so move this to RemoteGPU::create()..

> Source/WebKit/GPUProcess/graphics/WebGPU/RemoteGPU.cpp:67
> +    m_streamConnection->startReceivingMessages(*this, Messages::RemoteGPU::messageReceiverName(), m_identifier.toUInt64());

This will induce message dispatches from IPC dispatch thread to the work queue thread. The work queue thread will call virtual functions through `this`.. If the virtual function table is not constructed, those end up in wrong places.

> Source/WebKit/GPUProcess/graphics/WebGPU/RemoteGPU.cpp:76
> +    });

it's not obvious that it's guaranteed that the protectedThis is the only ref to RemoteGPU.
It would be simpler to just not require that, so maybe empty the object heap in workQueueUninitialize()

> Source/WebKit/GPUProcess/graphics/WebGPU/RemoteGPU.cpp:98
> +    m_streamConnection = nullptr;

the object heap should probably be emptied from this?
It appears that RemoteGPU can live in main thread and work queue thread, but the subobjects seem to be designed to live in the work thread only. 
Alternatively you'd need to assert in the destructor that it runs in the work queue thread. (which is a bit confusing as it will actually destroy the work queue itself, but it does work, at least on Cocoa/Darwin)

> Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteGPUProxy.cpp:98
> +    if (!sendResult || !response) {

probably useful to mark it lost here too, to establish a pattern for marking lost for all the future messages.

[Myles C. Maxfield]

Comment on attachment 445354 [details]
Patch

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

>> Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteGPUProxy.cpp:98
>> +    if (!sendResult || !response) {
> 
> probably useful to mark it lost here too, to establish a pattern for marking lost for all the future messages.

requestAdapter() can be called with different arguments, though, and those different arguments might cause it to succeed.

[Kimmo Kinnunen]

Comment on attachment 445354 [details]
Patch

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

>>> Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteGPUProxy.cpp:98
>>> +    if (!sendResult || !response) {
>> 
>> probably useful to mark it lost here too, to establish a pattern for marking lost for all the future messages.
> 
> requestAdapter() can be called with different arguments, though, and those different arguments might cause it to succeed.

Sure, but at least !sendResult part means that the GPU Process did not respond. After this, the process is in undefined state.

[Myles C. Maxfield]

Comment on attachment 445354 [details]
Patch

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

>>>> Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteGPUProxy.cpp:98
>>>> +    if (!sendResult || !response) {
>>> 
>>> probably useful to mark it lost here too, to establish a pattern for marking lost for all the future messages.
>> 
>> requestAdapter() can be called with different arguments, though, and those different arguments might cause it to succeed.
> 
> Sure, but at least !sendResult part means that the GPU Process did not respond. After this, the process is in undefined state.

Right, good. 👍

[Myles C. Maxfield]

Committed r286320 (244679@main): <https://commits.webkit.org/244679@main>

[Radar WebKit Bug Importer]

<rdar://problem/85884713>