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.
Created attachment 420296 [details] Patch
Created attachment 420306 [details] Patch
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?
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.
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?
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()?
Created attachment 420346 [details] update changelog
(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.
Created attachment 420432 [details] update name
Committed r272915: <https://commits.webkit.org/r272915> All reviewed patches have been landed. Closing bug and clearing flags on attachment 420432 [details].
<rdar://problem/74396052>