Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1748183006:
Add lock to fix race in AudioRendererMixerInput. (Closed)
|
Created:
4 years, 9 months ago by chcunningham Modified:
4 years, 9 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd lock to fix race in AudioRendererMixerInput.
Clusterfuzz identified a race between the media thread calling SetVolume
and the audio device thread calling ProvideInput. Add a lock to
synchronize access to |volume_| between threads.
Also adds some thread_checkers to just firm up the contract that the
majority of these methods are called only by the media thread.
BUG=588992
Committed: https://crrev.com/57c6958a99f2c62a4408d2a9d262a22129309106
Cr-Commit-Position: refs/heads/master@{#379349}
Committed: https://crrev.com/1675282361cc4dfa9e25beaabeb8c2e104140713
Cr-Commit-Position: refs/heads/master@{#381763}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add threadchecker, restrict lock to protecting |volume_| #Patch Set 3 : Rebase #
Total comments: 4
Patch Set 4 : Remove destructor thread_check #Patch Set 5 : Moving lock down in ProvideInput #
Total comments: 3
Patch Set 6 : Remove thread checker. #Patch Set 7 : Typo. #
Total comments: 2
Patch Set 8 : Better thread safety comments #
Messages
Total messages: 50 (15 generated)
chcunningham@chromium.org changed reviewers: + sandersd@chromium.org
I was not able to repro the race with minimized test case from the bug, but I think this is a pretty straightforward thread safety failure given description in clusterfuzz. Will re-run the clusterfuzz test on their bot once it lands.
dalecurtis@chromium.org changed reviewers: + dalecurtis@chromium.org, olka@chromium.org
I don't think you need all this locking. You only need the lock around the read of SetVolume() and the return to ProvideInput(). +olka who's worked on this recently.
On 2016/03/03 02:01:22, DaleCurtis wrote: > I don't think you need all this locking. You only need the lock around the read > of SetVolume() and the return to ProvideInput(). Makes sense. One thing I notice is that this is a "restartable" sink, which means we can get a second call to Initialize, re-assigning our callback_. The contract is technically that this should only happen after calling Stop(), which would theoretically remove this input from the converter and stop future calls to ProvideInput. But if the contract is violated, I'm not sure there's any real guards in place to prevent us from re-setting the callback_ while we're trying to use it in ProvideInput. Perhaps we need such guards in AudioConverter instead of here though? Right now AudioConverter does not lock at all. It has callers removing/adding inputs on the media thread (via this AudioRendererMixerInput) and I think its iterating over those inputs (calling ProvideInput) on the audio device thread. Adding locking to protect AudioConverters inputs list might be needed and completely address this restartable concern. WDYT?
Agree with Dale: those lock not only not needed, but also pretty dangerous. https://codereview.chromium.org/1748183006/diff/1/media/base/audio_renderer_m... File media/base/audio_renderer_mixer_input.cc (right): https://codereview.chromium.org/1748183006/diff/1/media/base/audio_renderer_m... media/base/audio_renderer_mixer_input.cc:220: base::AutoLock auto_lock(lock_); ProvideInput runs on real-time "render" thread, so we should avoid any locks we'll wait on for any significant amount of time; and Start/Stop/Switch operations are really long ones. https://codereview.chromium.org/1748183006/diff/1/media/base/audio_renderer_m... media/base/audio_renderer_mixer_input.cc:240: base::AutoLock auto_lock(lock_); OnRenderError is a callback which can potentially be run during device start/swtich, i.e. when the lock is already acquired.
On 2016/03/03 02:31:42, chcunningham1 wrote: > On 2016/03/03 02:01:22, DaleCurtis wrote: > > I don't think you need all this locking. You only need the lock around the > read > > of SetVolume() and the return to ProvideInput(). > > Makes sense. > > One thing I notice is that this is a "restartable" sink, which means we can get > a second call to Initialize, re-assigning our callback_. The contract is > technically that this should only happen after calling Stop(), which would > theoretically remove this input from the converter and stop future calls to > ProvideInput. But if the contract is violated, I'm not sure there's any real > guards in place to prevent us from re-setting the callback_ while we're trying > to use it in ProvideInput. Good point. If we assume that all the MixerInput control operations run on the same thread (Dale, it is correct?), I think we can just add [D]CHECK(!started_) in Initialize(). But it also means we are missing thread checker here which should be added. On the other hand, if control operations can be called from different threads, we are missing a bit of thread safety here (which we can probably fix by introducing an atomic state). > Perhaps we need such guards in AudioConverter instead > of here though? Right now AudioConverter does not lock at all. It has callers > removing/adding inputs on the media thread (via this AudioRendererMixerInput) > and I think its iterating over those inputs (calling ProvideInput) on the audio > device thread. Adding locking to protect AudioConverters inputs list might be > needed and completely address this restartable concern. WDYT? AudioRendererMixerInput accesses AudioConverter through AudioRendereMixer, which guards its converter(s) with locks: https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_r... https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_r... https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_r... so we don't modify converter inputs while rendering. In general, I don't think we should not add locks to AudioConverter, because it's a utility class with several different applications (we've got 2 audio converters chained in AudioRendererMixer, for example); introducing locking there will complicate things.
> In general, I don't think we should not add In general, I don't think we should add
> Good point. If we assume that all the MixerInput control operations run on the > same thread (Dale, it is correct?), I think we can just add [D]CHECK(!started_) > in Initialize(). > But it also means we are missing thread checker here which should be added. > Happy to add a thread checker for the control operations. I believe these are all called from the media thread, but thread checker will confirm ;). Just a heads up, IIUC this object is created on the render thread, so I'll need to DetachFromThread in the ctor. > On the other hand, if control operations can be called from different threads, > we are missing a bit of thread safety here (which we can probably fix by > introducing an atomic state). > Expecting this wont be needed, but to be sure I follow you - are you suggesting a second lock specific to the control operations? > > Perhaps we need such guards in AudioConverter instead > > of here though? Right now AudioConverter does not lock at all. It has callers > > removing/adding inputs on the media thread (via this AudioRendererMixerInput) > > and I think its iterating over those inputs (calling ProvideInput) on the > audio > > device thread. Adding locking to protect AudioConverters inputs list might be > > needed and completely address this restartable concern. WDYT? > > AudioRendererMixerInput accesses AudioConverter through AudioRendereMixer, which > guards its converter(s) with locks: > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_r... > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_r... > https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_r... > so we don't modify converter inputs while rendering. > In general, I don't think we should not add locks to AudioConverter, because > it's a utility class with several different applications (we've got 2 audio > converters chained in AudioRendererMixer, for example); introducing locking > there will complicate things. Excellent! You're totally right, mixer has us covered. No need to do it in audio converter.
https://codereview.chromium.org/1748183006/diff/1/media/base/audio_renderer_m... File media/base/audio_renderer_mixer_input.cc (right): https://codereview.chromium.org/1748183006/diff/1/media/base/audio_renderer_m... media/base/audio_renderer_mixer_input.cc:220: base::AutoLock auto_lock(lock_); On 2016/03/03 11:04:02, o1ka wrote: > ProvideInput runs on real-time "render" thread, so we should avoid any locks > we'll wait on for any significant amount of time; and Start/Stop/Switch > operations are really long ones. Ack. I'll remove locks from Start/Stop/Switch and drop in a thread checker. https://codereview.chromium.org/1748183006/diff/1/media/base/audio_renderer_m... media/base/audio_renderer_mixer_input.cc:240: base::AutoLock auto_lock(lock_); On 2016/03/03 11:04:02, o1ka wrote: > OnRenderError is a callback which can potentially be run during device > start/swtich, i.e. when the lock is already acquired. Ack, will remove.
chcunningham@google.com changed reviewers: + chcunningham@google.com
Cast bot is failing in a media browser test. Seems the object is not destructed on the same thread as the Pause/Play calls. Might still be safe... need to take a closer look. [1:1:0303/130413:518433062:FATAL:audio_renderer_mixer_input.cc(39)] Check failed: thread_checker_.CalledOnValidThread(). #0 0x000003ef822e base::debug::StackTrace::StackTrace() #1 0x000003f0ccbb logging::LogMessage::~LogMessage() #2 0x0000016ead6d media::AudioRendererMixerInput::~AudioRendererMixerInput() #3 0x0000016eaf29 media::AudioRendererMixerInput::~AudioRendererMixerInput() #4 0x000003d84ba6 media::WebAudioSourceProviderImpl::~WebAudioSourceProviderImpl() #5 0x000003d84c10 media::WebAudioSourceProviderImpl::~WebAudioSourceProviderImpl() #6 0x000003d8cc33 media::WebMediaPlayerImpl::~WebMediaPlayerImpl() #7 0x000003d8cd49 media::WebMediaPlayerImpl::~WebMediaPlayerImpl() #8 0x000002b477cb blink::HTMLMediaElement::clearMediaPlayer() #9 0x000002b51dcb blink::HTMLMediaElement::stop() #10 0x0000029bad90 blink::ContextLifecycleNotifier::notifyStoppingActiveDOMObjects() #11 0x0000029d5f3c blink::Document::detach() #12 0x000003014542 blink::FrameLoader::prepareForCommit() #13 0x0000030146f1 blink::FrameLoader::commitProvisionalLoad() #14 0x000002ffd99d blink::DocumentLoader::finishedLoading() #15 0x000002fff59e blink::DocumentLoader::maybeLoadEmpty() #16 0x000002fff621 blink::DocumentLoader::startLoadingMainResource() #17 0x000003013dcb blink::FrameLoader::startLoad() #18 0x000003010fde blink::FrameLoader::load() #19 0x0000026148ea blink::WebLocalFrameImpl::load() #20 0x000003c1fe4d content::RenderFrameImpl::NavigateInternal() #21 0x000003c1b409 content::RenderFrameImpl::OnNavigate() #22 0x000003c1b2b9 _ZN3IPC8MessageTI22FrameMsg_Navigate_MetaSt5tupleIJN7content22CommonNavigationParamsENS3_21StartNavigationParamsENS3_23RequestNavigationParamsEEEvE8DispatchINS3_15RenderFrameImplESA_vMSA_FvRKS4_RKS5_RKS6_EEEbPKNS_7MessageEPT_PT0_PT1_T2_ #23 0x000003c1a686 content::RenderFrameImpl::OnMessageReceived() #24 0x0000019fb487 IPC::MessageRouter::RouteMessage() #25 0x0000019fb3c8 IPC::MessageRouter::OnMessageReceived() #26 0x000003b0046c content::ChildThreadImpl::OnMessageReceived() #27 0x0000019efdee IPC::ChannelProxy::Context::OnDispatchMessage() #28 0x000003ef8f8c base::debug::TaskAnnotator::RunTask() #29 0x000003b7b097 scheduler::TaskQueueManager::ProcessTaskFromWorkQueue() #30 0x000003b79d1f scheduler::TaskQueueManager::DoWork() #31 0x000003b7c0b4 _ZN4base8internal7InvokerINS_13IndexSequenceIJLm0ELm1ELm2EEEENS0_9BindStateINS0_15RunnableAdapterIMN9scheduler16TaskQueueManagerEFvNS_9TimeTicksEbEEEFvPS7_S8_bEJNS_7WeakPtrIS7_EERS8_bEEENS0_12InvokeHelperILb1EvSB_EEFvvEE3RunEPNS0_13BindStateBaseE #32 0x000003ef8f8c base::debug::TaskAnnotator::RunTask() #33 0x000003f138bb base::MessageLoop::RunTask() #34 0x000003f13bb8 base::MessageLoop::DeferOrRunPendingTask() #35 0x000003f13d7b base::MessageLoop::DoWork() #36 0x000003f1576f base::MessagePumpDefault::Run() #37 0x000003f13417 base::MessageLoop::RunHandler() #38 0x000003f2dd5c base::RunLoop::Run() #39 0x000003f12730 base::MessageLoop::Run() #40 0x000003c62f20 content::RendererMain() #41 0x000003ae8feb content::RunZygote() #42 0x000003ae9622 content::RunNamedProcessTypeMain() #43 0x000003aea04b content::ContentMainRunnerImpl::Run() #44 0x000003ae8b93 content::ContentMain() #45 0x00000083e92b content::LaunchTests() #46 0x000000815503 main #47 0x7f293efe676d __libc_start_main #48 0x000000481a91 <unknown>
Destruction seems safe. Happens on main thread as part of WMPI teardown. WIMPI waits for all pipeline tasks to complete and kills pipeline prior to this, so no risk of any late control method calls. Also, the
https://codereview.chromium.org/1748183006/diff/40001/media/base/audio_render... File media/base/audio_renderer_mixer_input.cc (right): https://codereview.chromium.org/1748183006/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer_input.cc:201: // No thread checker here. This method is called on a different thread as part Move this down to l.222; it's good practice to not hold unnecessary locks when calling outside of your code (a la |callback_|).
On 2016/03/03 22:08:03, chcunningham wrote: > Destruction seems safe. Happens on main thread as part of WMPI teardown. WIMPI > waits for all pipeline tasks to complete and kills pipeline prior to this, so no > risk of any late control method calls. Also, the (hit enter too soon) Also, the pipeline is stopped and then we wait for the stop to complete, so theirs no chance of the audio device thread continuing to request input after / during destruction. Yay for us! I'm adding a DCHECK(!started_) to the destructor (like was added to Initialize) and removing the thread checking call. Lets see if the bots find anything else...
https://codereview.chromium.org/1748183006/diff/40001/media/base/audio_render... File media/base/audio_renderer_mixer_input.cc (right): https://codereview.chromium.org/1748183006/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer_input.cc:201: // No thread checker here. This method is called on a different thread as part On 2016/03/03 22:14:15, DaleCurtis wrote: > Move this down to l.222; it's good practice to not hold unnecessary locks when > calling outside of your code (a la |callback_|). The method comment is "The return value is the volume level of the provided audio data." If we move the lock we allow volume_ to change between callback->Render and return. Is that ok (why?)?
https://codereview.chromium.org/1748183006/diff/40001/media/base/audio_render... File media/base/audio_renderer_mixer_input.cc (right): https://codereview.chromium.org/1748183006/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer_input.cc:201: // No thread checker here. This method is called on a different thread as part On 2016/03/03 at 22:21:58, chcunningham wrote: > On 2016/03/03 22:14:15, DaleCurtis wrote: > > Move this down to l.222; it's good practice to not hold unnecessary locks when > > calling outside of your code (a la |callback_|). > > The method comment is "The return value is the volume level of the provided audio data." If we move the lock we allow volume_ to change between callback->Render and return. Is that ok (why?)? Why not? Nothing before this point uses the volume information. SetVolume() is generally coming from JavaScript, so the faster we react to it the better.
https://codereview.chromium.org/1748183006/diff/40001/media/base/audio_render... File media/base/audio_renderer_mixer_input.cc (right): https://codereview.chromium.org/1748183006/diff/40001/media/base/audio_render... media/base/audio_renderer_mixer_input.cc:201: // No thread checker here. This method is called on a different thread as part On 2016/03/03 22:23:36, DaleCurtis wrote: > On 2016/03/03 at 22:21:58, chcunningham wrote: > > On 2016/03/03 22:14:15, DaleCurtis wrote: > > > Move this down to l.222; it's good practice to not hold unnecessary locks > when > > > calling outside of your code (a la |callback_|). > > > > The method comment is "The return value is the volume level of the provided > audio data." If we move the lock we allow volume_ to change between > callback->Render and return. Is that ok (why?)? > > Why not? Nothing before this point uses the volume information. SetVolume() is > generally coming from JavaScript, so the faster we react to it the better. Done.
lgtm https://codereview.chromium.org/1748183006/diff/80001/media/base/audio_render... File media/base/audio_renderer_mixer_input.cc (right): https://codereview.chromium.org/1748183006/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_input.cc:221: { {} are unnecessary since you're right above a return.
https://codereview.chromium.org/1748183006/diff/80001/media/base/audio_render... File media/base/audio_renderer_mixer_input.cc (right): https://codereview.chromium.org/1748183006/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_input.cc:221: { On 2016/03/03 23:08:01, DaleCurtis wrote: > {} are unnecessary since you're right above a return. thought maybe you'd ask for it as a best practice... I can remove if you feel strongly. I like that it calls a little extra attention to the synchronization for future readers/changes.
https://codereview.chromium.org/1748183006/diff/80001/media/base/audio_render... File media/base/audio_renderer_mixer_input.cc (right): https://codereview.chromium.org/1748183006/diff/80001/media/base/audio_render... media/base/audio_renderer_mixer_input.cc:221: { On 2016/03/03 at 23:18:53, chcunningham1 wrote: > On 2016/03/03 23:08:01, DaleCurtis wrote: > > {} are unnecessary since you're right above a return. > > thought maybe you'd ask for it as a best practice... I can remove if you feel strongly. I like that it calls a little extra attention to the synchronization for future readers/changes. I don't feel strongly. We typically only use {} around blocks that are mid-function. I wouldn't say it's best practice to do this all the time.
Description was changed from ========== Add lock to fix race in AudioRendererMixerInput. Clusterfuzz identified a race between the media thread calling SetVolume and the audio device thread calling ProvideInput. Add a lock to protect all members from racing between threads. BUG=588992 ========== to ========== Add lock to fix race in AudioRendererMixerInput. Clusterfuzz identified a race between the media thread calling SetVolume and the audio device thread calling ProvideInput. Add a lock to synchronize access to |volume_| between threads. Also adds some thread_checkers to just firm up the contract that the majority of these methods are called only by the media thread. BUG=588992 ==========
chcunningham@chromium.org changed reviewers: - chcunningham@google.com, sandersd@chromium.org
The CQ bit was checked by chcunningham@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1748183006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1748183006/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> > On the other hand, if control operations can be called from different threads, > > we are missing a bit of thread safety here (which we can probably fix by > > introducing an atomic state). > > > > Expecting this wont be needed, but to be sure I follow you - are you suggesting > a second lock specific to the control operations? I'm glad that it appeared to be not needed, because it was some vague idea I had which does not look livable to me now :) Yes, to serialize operations we would either need a dedicated lock (from my hard-earned experience one should never use the same lock to protect data and to serialize operations), or posting operations to a dedicated thread.
Thanks a lot for fixing that!
On 2016/03/04 09:55:04, o1ka wrote: > Thanks a lot for fixing that! Happy to fix it :) Thanks for reviewing!
The CQ bit was checked by chcunningham@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1748183006/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1748183006/80001
Message was sent while issue was closed.
Description was changed from ========== Add lock to fix race in AudioRendererMixerInput. Clusterfuzz identified a race between the media thread calling SetVolume and the audio device thread calling ProvideInput. Add a lock to synchronize access to |volume_| between threads. Also adds some thread_checkers to just firm up the contract that the majority of these methods are called only by the media thread. BUG=588992 ========== to ========== Add lock to fix race in AudioRendererMixerInput. Clusterfuzz identified a race between the media thread calling SetVolume and the audio device thread calling ProvideInput. Add a lock to synchronize access to |volume_| between threads. Also adds some thread_checkers to just firm up the contract that the majority of these methods are called only by the media thread. BUG=588992 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add lock to fix race in AudioRendererMixerInput. Clusterfuzz identified a race between the media thread calling SetVolume and the audio device thread calling ProvideInput. Add a lock to synchronize access to |volume_| between threads. Also adds some thread_checkers to just firm up the contract that the majority of these methods are called only by the media thread. BUG=588992 ========== to ========== Add lock to fix race in AudioRendererMixerInput. Clusterfuzz identified a race between the media thread calling SetVolume and the audio device thread calling ProvideInput. Add a lock to synchronize access to |volume_| between threads. Also adds some thread_checkers to just firm up the contract that the majority of these methods are called only by the media thread. BUG=588992 Committed: https://crrev.com/57c6958a99f2c62a4408d2a9d262a22129309106 Cr-Commit-Position: refs/heads/master@{#379349} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/57c6958a99f2c62a4408d2a9d262a22129309106 Cr-Commit-Position: refs/heads/master@{#379349}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1771463002/ by chcunningham@chromium.org. The reason for reverting is: This seems to cause failures in LayoutTests/http/tests/security/media. Not sure why this wasn't caught by CQ... Tracking CQ weirdness here: https://bugs.chromium.org/p/chromium/issues/detail?id=592079 .
Message was sent while issue was closed.
Description was changed from ========== Add lock to fix race in AudioRendererMixerInput. Clusterfuzz identified a race between the media thread calling SetVolume and the audio device thread calling ProvideInput. Add a lock to synchronize access to |volume_| between threads. Also adds some thread_checkers to just firm up the contract that the majority of these methods are called only by the media thread. BUG=588992 Committed: https://crrev.com/57c6958a99f2c62a4408d2a9d262a22129309106 Cr-Commit-Position: refs/heads/master@{#379349} ========== to ========== Add lock to fix race in AudioRendererMixerInput. Clusterfuzz identified a race between the media thread calling SetVolume and the audio device thread calling ProvideInput. Add a lock to synchronize access to |volume_| between threads. Also adds some thread_checkers to just firm up the contract that the majority of these methods are called only by the media thread. BUG=588992 Committed: https://crrev.com/57c6958a99f2c62a4408d2a9d262a22129309106 Cr-Commit-Position: refs/heads/master@{#379349} ==========
Turns out we can't use a thread checker because these methods are accessed by both the main and media thread. Main thread access occurs via WebAudio API, while media thread access comes via HTMLMediaElement calls. In both cases, access to AudioRendererMixerInput is funneled through WebAudioSourceProviderImpl which guards these calls with a lock.
linux_blink_rel_ng bot is happy this time. Reviewers, please take one last look.
lgtm
Thanks for dealing with this! lgtm after comment cleanup. https://codereview.chromium.org/1748183006/diff/120001/media/base/audio_rende... File media/base/audio_renderer_mixer_input.cc (right): https://codereview.chromium.org/1748183006/diff/120001/media/base/audio_rende... media/base/audio_renderer_mixer_input.cc:199: // should be enforced by the caller. See the lock in AudioRendererMixer. It's not quite clear to me how the lock in AudioRendererMixer is related here? And can we add a header comment about caller responsibility for thread safety?
https://codereview.chromium.org/1748183006/diff/120001/media/base/audio_rende... File media/base/audio_renderer_mixer_input.cc (right): https://codereview.chromium.org/1748183006/diff/120001/media/base/audio_rende... media/base/audio_renderer_mixer_input.cc:199: // should be enforced by the caller. See the lock in AudioRendererMixer. On 2016/03/16 14:31:18, o1ka wrote: > It's not quite clear to me how the lock in AudioRendererMixer is related here? > And can we add a header comment about caller responsibility for thread safety? Woops! AudioRendererMixer should have been WebAudioSourceProviderImpl. I've simplified this comment and added a more detailed comment in the header.
The CQ bit was checked by chcunningham@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dalecurtis@chromium.org, olka@chromium.org Link to the patchset: https://codereview.chromium.org/1748183006/#ps140001 (title: "Better thread safety comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1748183006/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1748183006/140001
Message was sent while issue was closed.
Description was changed from ========== Add lock to fix race in AudioRendererMixerInput. Clusterfuzz identified a race between the media thread calling SetVolume and the audio device thread calling ProvideInput. Add a lock to synchronize access to |volume_| between threads. Also adds some thread_checkers to just firm up the contract that the majority of these methods are called only by the media thread. BUG=588992 Committed: https://crrev.com/57c6958a99f2c62a4408d2a9d262a22129309106 Cr-Commit-Position: refs/heads/master@{#379349} ========== to ========== Add lock to fix race in AudioRendererMixerInput. Clusterfuzz identified a race between the media thread calling SetVolume and the audio device thread calling ProvideInput. Add a lock to synchronize access to |volume_| between threads. Also adds some thread_checkers to just firm up the contract that the majority of these methods are called only by the media thread. BUG=588992 Committed: https://crrev.com/57c6958a99f2c62a4408d2a9d262a22129309106 Cr-Commit-Position: refs/heads/master@{#379349} ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add lock to fix race in AudioRendererMixerInput. Clusterfuzz identified a race between the media thread calling SetVolume and the audio device thread calling ProvideInput. Add a lock to synchronize access to |volume_| between threads. Also adds some thread_checkers to just firm up the contract that the majority of these methods are called only by the media thread. BUG=588992 Committed: https://crrev.com/57c6958a99f2c62a4408d2a9d262a22129309106 Cr-Commit-Position: refs/heads/master@{#379349} ========== to ========== Add lock to fix race in AudioRendererMixerInput. Clusterfuzz identified a race between the media thread calling SetVolume and the audio device thread calling ProvideInput. Add a lock to synchronize access to |volume_| between threads. Also adds some thread_checkers to just firm up the contract that the majority of these methods are called only by the media thread. BUG=588992 Committed: https://crrev.com/57c6958a99f2c62a4408d2a9d262a22129309106 Cr-Commit-Position: refs/heads/master@{#379349} Committed: https://crrev.com/1675282361cc4dfa9e25beaabeb8c2e104140713 Cr-Commit-Position: refs/heads/master@{#381763} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/1675282361cc4dfa9e25beaabeb8c2e104140713 Cr-Commit-Position: refs/heads/master@{#381763} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
