Bug 221891 – ThreadMessageReceiverRefCounted subclasses are accessed in non-thread-safe way

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Kimmo Kinnunen]

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.

[Kimmo Kinnunen]

Created attachment 420290 [details]
Patch

[Kimmo Kinnunen]

Created attachment 420291 [details]
Patch

[Chris Dumez]

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.

[Kimmo Kinnunen]

Created attachment 420429 [details]
update names

[Kimmo Kinnunen]

Created attachment 420430 [details]
update names

[EWS]

commit-queue failed to commit attachment 420430 [details] to WebKit repository. To retry, please set cq+ flag again.

[EWS]

Committed r272905: <https://commits.webkit.org/r272905>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420430 [details].

[Radar WebKit Bug Importer]

<rdar://problem/74391824>

[Truitt Savell]

Reverted r272905 for reason:

caused TestWebKitAPI.WebKit2.CrashGPUProcessWhileCapturingAndCalling to timeout

Committed r273029 (234230@main): <https://commits.webkit.org/234230@main>

[Kimmo Kinnunen]

Created attachment 420813 [details]
Patch

[Kimmo Kinnunen]

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

[Kimmo Kinnunen]

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.

[EWS]

Committed r273074: <https://commits.webkit.org/r273074>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 420813 [details].