WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
221894
RemoteAudioSourceProviderManager is accessed in non-thread-safe manner
https://bugs.webkit.org/show_bug.cgi?id=221894
Summary
RemoteAudioSourceProviderManager is accessed in non-thread-safe manner
Kimmo Kinnunen
Reported
2021-02-15 02:43:49 PST
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.
Attachments
Patch
(8.48 KB, patch)
2021-02-15 02:56 PST
,
Kimmo Kinnunen
ews-feeder
: commit-queue-
Details
Formatted Diff
Diff
Patch
(8.53 KB, patch)
2021-02-15 04:40 PST
,
Kimmo Kinnunen
no flags
Details
Formatted Diff
Diff
update changelog
(8.04 KB, patch)
2021-02-15 12:17 PST
,
Kimmo Kinnunen
no flags
Details
Formatted Diff
Diff
update name
(8.03 KB, patch)
2021-02-16 00:33 PST
,
Kimmo Kinnunen
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Kimmo Kinnunen
Comment 1
2021-02-15 02:56:00 PST
Created
attachment 420296
[details]
Patch
Kimmo Kinnunen
Comment 2
2021-02-15 04:40:01 PST
Created
attachment 420306
[details]
Patch
Eric Carlson
Comment 3
2021-02-15 08:37:02 PST
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 4
2021-02-15 10:51:02 PST
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 5
2021-02-15 10:56:38 PST
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 6
2021-02-15 12:17:02 PST
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
Comment 7
2021-02-15 12:17:17 PST
Created
attachment 420346
[details]
update changelog
Chris Dumez
Comment 8
2021-02-15 12:19:37 PST
(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
Comment 9
2021-02-16 00:33:34 PST
Created
attachment 420432
[details]
update name
EWS
Comment 10
2021-02-16 10:08:14 PST
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
Comment 11
2021-02-16 10:09:13 PST
<
rdar://problem/74396052
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug