[webkit-reviews] review granted: [Bug 233600] [WebGPU] Hook up StreamServerConnection to Remote*** classes : [Attachment 445354] Patch

[bugzilla-daemon at webkit.org]

Kimmo Kinnunen <kkinnunen at apple.com> has granted Myles C. Maxfield
<mmaxfield at apple.com>'s request for review:
Bug 233600: [WebGPU] Hook up StreamServerConnection to Remote*** classes
https://bugs.webkit.org/show_bug.cgi?id=233600

Attachment 445354: Patch

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




--- Comment #4 from Kimmo Kinnunen <kkinnunen at apple.com> ---
Comment on attachment 445354
  --> https://bugs.webkit.org/attachment.cgi?id=445354
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.