Bug 233600 - [WebGPU] Hook up StreamServerConnection to Remote*** classes
Summary: [WebGPU] Hook up StreamServerConnection to Remote*** classes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGPU (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Myles C. Maxfield
URL:
Keywords: InRadar
Depends on:
Blocks: 233619
  Show dependency treegraph
 
Reported: 2021-11-29 14:59 PST by Myles C. Maxfield
Modified: 2021-11-30 13:15 PST (History)
11 users (show)

See Also:


Attachments
Patch (150.87 KB, patch)
2021-11-29 15:23 PST, Myles C. Maxfield
kkinnunen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2021-11-29 14:59:06 PST
[WebGPU] Hook up StreamServerConnection to Remote*** classes
Comment 1 Myles C. Maxfield 2021-11-29 15:23:18 PST
Created attachment 445354 [details]
Patch
Comment 2 Myles C. Maxfield 2021-11-29 20:41:57 PST
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.
Comment 3 Myles C. Maxfield 2021-11-29 23:13:53 PST
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.)
Comment 4 Kimmo Kinnunen 2021-11-30 00:28:38 PST
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.
Comment 5 Myles C. Maxfield 2021-11-30 10:20:21 PST
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.
Comment 6 Kimmo Kinnunen 2021-11-30 11:31:28 PST
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.
Comment 7 Myles C. Maxfield 2021-11-30 12:47:27 PST
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. 👍
Comment 8 Myles C. Maxfield 2021-11-30 13:14:16 PST
Committed r286320 (244679@main): <https://commits.webkit.org/244679@main>
Comment 9 Radar WebKit Bug Importer 2021-11-30 13:15:27 PST
<rdar://problem/85884713>