Summary: | [WebGPU] Hook up StreamServerConnection to Remote*** classes | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Myles C. Maxfield <mmaxfield> | ||||
Component: | WebGPU | Assignee: | Myles C. Maxfield <mmaxfield> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | annulen, dino, ews-watchlist, gyuyoung.kim, hi, kkinnunen, mkwst, rmorisset, ryuan.choi, sergio, webkit-bug-importer | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 233619 | ||||||
Attachments: |
|
Description
Myles C. Maxfield
2021-11-29 14:59:06 PST
Created attachment 445354 [details]
Patch
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 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 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 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 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 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. 👍 Committed r286320 (244679@main): <https://commits.webkit.org/244679@main> |