Commit 16752823 authored by chcunningham's avatar chcunningham Committed by Commit bot

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}

Review URL: https://codereview.chromium.org/1748183006

Cr-Commit-Position: refs/heads/master@{#381763}
parent 214f5aa8
...@@ -32,12 +32,14 @@ AudioRendererMixerInput::AudioRendererMixerInput( ...@@ -32,12 +32,14 @@ AudioRendererMixerInput::AudioRendererMixerInput(
base::Unretained(this))) {} base::Unretained(this))) {}
AudioRendererMixerInput::~AudioRendererMixerInput() { AudioRendererMixerInput::~AudioRendererMixerInput() {
DCHECK(!started_);
DCHECK(!mixer_); DCHECK(!mixer_);
} }
void AudioRendererMixerInput::Initialize( void AudioRendererMixerInput::Initialize(
const AudioParameters& params, const AudioParameters& params,
AudioRendererSink::RenderCallback* callback) { AudioRendererSink::RenderCallback* callback) {
DCHECK(!started_);
DCHECK(!mixer_); DCHECK(!mixer_);
DCHECK(callback); DCHECK(callback);
...@@ -106,6 +108,7 @@ void AudioRendererMixerInput::Pause() { ...@@ -106,6 +108,7 @@ void AudioRendererMixerInput::Pause() {
} }
bool AudioRendererMixerInput::SetVolume(double volume) { bool AudioRendererMixerInput::SetVolume(double volume) {
base::AutoLock auto_lock(volume_lock_);
volume_ = volume; volume_ = volume;
return true; return true;
} }
...@@ -190,7 +193,13 @@ double AudioRendererMixerInput::ProvideInput(AudioBus* audio_bus, ...@@ -190,7 +193,13 @@ double AudioRendererMixerInput::ProvideInput(AudioBus* audio_bus,
frames_filled, audio_bus->frames() - frames_filled); frames_filled, audio_bus->frames() - frames_filled);
} }
return frames_filled > 0 ? volume_ : 0; // We're reading |volume_| from the audio device thread and must avoid racing
// with the main/media thread calls to SetVolume(). See thread safety comment
// in the header file.
{
base::AutoLock auto_lock(volume_lock_);
return frames_filled > 0 ? volume_ : 0;
}
} }
void AudioRendererMixerInput::OnRenderError() { void AudioRendererMixerInput::OnRenderError() {
......
// Copyright (c) 2012 The Chromium Authors. All rights reserved. // Copyright (c) 2012 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be // Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file. // found in the LICENSE file.
//
// THREAD SAFETY
//
// This class is generally not thread safe. Callers should ensure thread safety.
// For instance, the |sink_lock_| in WebAudioSourceProvider synchronizes access
// to this object across the main thread (for WebAudio APIs) and the
// media thread (for HTMLMediaElement APIs).
//
// The one exception is protection for |volume_| via |volume_lock_|. This lock
// prevents races between SetVolume() (called on any thread) and ProvideInput
// (called on audio device thread). See http://crbug.com/588992.
#ifndef MEDIA_BASE_AUDIO_RENDERER_MIXER_INPUT_H_ #ifndef MEDIA_BASE_AUDIO_RENDERER_MIXER_INPUT_H_
#define MEDIA_BASE_AUDIO_RENDERER_MIXER_INPUT_H_ #define MEDIA_BASE_AUDIO_RENDERER_MIXER_INPUT_H_
...@@ -9,6 +20,7 @@ ...@@ -9,6 +20,7 @@
#include "base/callback.h" #include "base/callback.h"
#include "base/macros.h" #include "base/macros.h"
#include "base/synchronization/lock.h"
#include "media/base/audio_converter.h" #include "media/base/audio_converter.h"
#include "media/base/audio_renderer_sink.h" #include "media/base/audio_renderer_sink.h"
#include "media/base/output_device.h" #include "media/base/output_device.h"
...@@ -69,6 +81,10 @@ class MEDIA_EXPORT AudioRendererMixerInput ...@@ -69,6 +81,10 @@ class MEDIA_EXPORT AudioRendererMixerInput
private: private:
friend class AudioRendererMixerInputTest; friend class AudioRendererMixerInputTest;
// Protect |volume_|, accessed by separate threads in ProvideInput() and
// SetVolume().
base::Lock volume_lock_;
bool started_; bool started_;
bool playing_; bool playing_;
double volume_; double volume_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment