ThreadMessageReceiverRefCounted subclasses are accessed in non-thread-safe way The subclasses call Connection::addThreadMessageReceiver() during their construction, passing 'this' as the receiver. The virtual function pointer is not fully constructed during the call time. Connection may start dispatching calls through that pointer immediately after the call. If a message comes during the call, it may be dispatched during the subclass constructor still running, before the virtual function pointer being correctly set up. Accessing virtual function pointer is not thread safe until it is fully constructed. The subclasses call Connection::removeThreadMessageReceiver() during their destruction. Connection may still dispatch calls through the passed in virtual function pointer. The virtual pointer is not as expected anymore at that point, is not thread-safe to read. Upon dispatch, the connection will ref the ThreadMessageReceiver. Since the instance might already be in destructor, this is a ref count increase on a destroyed object.
Created attachment 420290 [details] Patch
Created attachment 420291 [details] Patch
Comment on attachment 420291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420291&action=review r=me with nits. > Source/WebKit/GPUProcess/webrtc/RemoteAudioMediaStreamTrackRendererManager.cpp:46 > +void RemoteAudioMediaStreamTrackRendererManager::connectGPUProcess() I don't understand why this is named connectGPUProcess(). This code runs in the GPUProcess and if anything, it starts listening for IPC from the WebProcess. I think we should find a better name. Maybe startListeningForIPC()? > Source/WebKit/GPUProcess/webrtc/RemoteAudioMediaStreamTrackRendererManager.h:66 > + void disconnectGPUProcess(); What's this? Can't find the definition. > Source/WebKit/GPUProcess/webrtc/RemoteSampleBufferDisplayLayerManager.cpp:48 > +void RemoteSampleBufferDisplayLayerManager::connectGPUProcess() Same comment about naming. > Source/WebKit/NetworkProcess/webrtc/NetworkRTCProvider.cpp:98 > +void NetworkRTCProvider::connectWebProcess() Same comment about naming. > Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:171 > +void LibWebRTCCodecs::connectGPUProcess() Same comment about naming. > Source/WebKit/WebProcess/cocoa/RemoteCaptureSampleManager.cpp:52 > +void RemoteCaptureSampleManager::disconnectGPUProcess() Same comment about naming.
Created attachment 420429 [details] update names
Created attachment 420430 [details] update names
commit-queue failed to commit attachment 420430 [details] to WebKit repository. To retry, please set cq+ flag again.
Committed r272905: <https://commits.webkit.org/r272905> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420430 [details].
<rdar://problem/74391824>
Reverted r272905 for reason: caused TestWebKitAPI.WebKit2.CrashGPUProcessWhileCapturingAndCalling to timeout Committed r273029 (234230@main): <https://commits.webkit.org/234230@main>
Created attachment 420813 [details] Patch
Comment on attachment 420813 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=420813&action=review > Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:-179 > - m_connection->removeThreadMessageReceiver(Messages::LibWebRTCCodecsProxy::messageReceiverName()); This line was originally buggy.. > Source/WebKit/WebProcess/GPU/webrtc/LibWebRTCCodecs.cpp:471 > + m_connection->removeThreadMessageReceiver(Messages::LibWebRTCCodecs::messageReceiverName()); ..and I managed to put the above line here. Here in this patch this line is now correct and the test passes again
Comment on attachment 420813 [details] Patch Taking the liberty to carry the r+ over from the last patch, as the only thing that changed was the ReceiverName change from Proxy message to plain message.
Committed r273074: <https://commits.webkit.org/r273074> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420813 [details].