Bug 221894 – RemoteAudioSourceProviderManager is accessed in non-thread-safe manner

• Component: WebKit2
• Status: RESOLVED FIXED

[Kimmo Kinnunen]

RemoteAudioSourceProviderManager is accessed in non-thread-safe manner

The class calls Connection::addWorkQueueMessageReceiver() during its construction, passing 'this' as the receiver.
The virtual function pointer is not fully constructed during the call time.
Connection may start dispatching calls to the work queue.
The work queue may dispatch the task function through that pointer immediately after the call.
If a message comes during the constructor call, it may be dispatched during the 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 class calls Connection::removeWorkQueueMessageReceiver() during its destruction.

Connection may still dispatch calls to the work queue and the work queue 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 WorkQueueMessageReceiver.  Since the instance might already be in destructor, this is a ref count increase on a destroyed object.

[Kimmo Kinnunen]

Created attachment 420296 [details]
Patch

[Kimmo Kinnunen]

Created attachment 420306 [details]
Patch

[Eric Carlson]

Comment on attachment 420306 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420306&action=review

> Source/WebKit/ChangeLog:8
> +        Remove the call to Connection::addWorkQueueMessageReceiver() from the constructor.

I don't see this change. Did you forgot to include it in the patch?

[Chris Dumez]

Comment on attachment 420306 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420306&action=review

>> Source/WebKit/ChangeLog:8
>> +        Remove the call to Connection::addWorkQueueMessageReceiver() from the constructor.
> 
> I don't see this change. Did you forgot to include it in the patch?

I think this was done in bug 221891. This changelog probably needs updating.

[Chris Dumez]

Comment on attachment 420306 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420306&action=review

> Source/WebKit/WebProcess/GPU/GPUProcessConnection.cpp:88
> +        m_audioSourceProviderManager->disconnectGPUProcess();

This function does not exist on my checkout. Not sure why the bots are green, likely EWS does not build this code?

[Kimmo Kinnunen]

Comment on attachment 420306 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=420306&action=review

Thanks for reviewing!

>>> Source/WebKit/ChangeLog:8
>>> +        Remove the call to Connection::addWorkQueueMessageReceiver() from the constructor.
>> 
>> I don't see this change. Did you forgot to include it in the patch?
> 
> I think this was done in bug 221891. This changelog probably needs updating.

Ah, thanks! I've made this change in so many ways now to pass the review that I got the patches and changes done mixed up.
Removed the comment, the change changes only the destructor.

>> Source/WebKit/WebProcess/GPU/GPUProcessConnection.cpp:88
>> +        m_audioSourceProviderManager->disconnectGPUProcess();
> 
> This function does not exist on my checkout. Not sure why the bots are green, likely EWS does not build this code?

You mean the RemoteAudioSourceProviderManager::disconnectGPUProcess() added in the next hunk below?
Or GPUProcessConnection::~GPUProcessConnection()?

[Kimmo Kinnunen]

Created attachment 420346 [details]
update changelog

[Chris Dumez]

(In reply to Kimmo Kinnunen from comment #6)
> Comment on attachment 420306 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=420306&action=review
> 
> Thanks for reviewing!
> 
> >>> Source/WebKit/ChangeLog:8
> >>> +        Remove the call to Connection::addWorkQueueMessageReceiver() from the constructor.
> >> 
> >> I don't see this change. Did you forgot to include it in the patch?
> > 
> > I think this was done in bug 221891. This changelog probably needs updating.
> 
> Ah, thanks! I've made this change in so many ways now to pass the review
> that I got the patches and changes done mixed up.
> Removed the comment, the change changes only the destructor.
> 
> >> Source/WebKit/WebProcess/GPU/GPUProcessConnection.cpp:88
> >> +        m_audioSourceProviderManager->disconnectGPUProcess();
> > 
> > This function does not exist on my checkout. Not sure why the bots are green, likely EWS does not build this code?
> 
> You mean the RemoteAudioSourceProviderManager::disconnectGPUProcess() added
> in the next hunk below?
> Or GPUProcessConnection::~GPUProcessConnection()?

Yes, I had missed it.

[Kimmo Kinnunen]

Created attachment 420432 [details]
update name

[EWS]

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

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

[Radar WebKit Bug Importer]

<rdar://problem/74396052>