diff options
author | fischman@chromium.org <fischman@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-03-23 15:25:28 +0000 |
---|---|---|
committer | fischman@chromium.org <fischman@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-03-23 15:25:28 +0000 |
commit | 36330d1074e6428e4bede815a52bb280d58787c2 (patch) | |
tree | 195e31c4bf5456e692db91edb7819151e2b83184 /media/audio | |
parent | 57e17cd42c77255be13e2b8804a526413d1d12d8 (diff) | |
download | chromium_src-36330d1074e6428e4bede815a52bb280d58787c2.zip chromium_src-36330d1074e6428e4bede815a52bb280d58787c2.tar.gz chromium_src-36330d1074e6428e4bede815a52bb280d58787c2.tar.bz2 |
Fix race in AudioInputController::Close().
Before this change, a reference to AudioInputController was bound into the
Callback invoking DoClose(), which executes on the audio thread. If the
audio thread is slow to delete that callback (after dispatching |closed_task|)
then the main thread's reference to the AudioInputController might be released
first, leading to AudioInputController being destructed on the audio thread,
triggering a DCHECK in Timer::~Timer() (owned by the AudioInputController via
DelayTimer).
This race was exposed while working on http://codereview.chromium.org/9717021/
but isn't so much related to the content of that CL as to the fact that that CL
twiddles callback lifetime/timing a bit.
Review URL: http://codereview.chromium.org/9839051
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@128495 0039d316-1c4b-4281-b951-d872f2087c98
Diffstat (limited to 'media/audio')
-rw-r--r-- | media/audio/audio_input_controller.cc | 8 | ||||
-rw-r--r-- | media/audio/audio_input_controller.h | 7 | ||||
-rw-r--r-- | media/audio/audio_input_controller_unittest.cc | 19 |
3 files changed, 13 insertions, 21 deletions
diff --git a/media/audio/audio_input_controller.cc b/media/audio/audio_input_controller.cc index 27903fa..67623da 100644 --- a/media/audio/audio_input_controller.cc +++ b/media/audio/audio_input_controller.cc @@ -103,8 +103,8 @@ void AudioInputController::Record() { void AudioInputController::Close(const base::Closure& closed_task) { DCHECK(!closed_task.is_null()); - message_loop_->PostTask(FROM_HERE, base::Bind( - &AudioInputController::DoClose, this, closed_task)); + message_loop_->PostTaskAndReply( + FROM_HERE, base::Bind(&AudioInputController::DoClose, this), closed_task); } void AudioInputController::DoCreate(AudioManager* audio_manager, @@ -150,7 +150,7 @@ void AudioInputController::DoRecord() { handler_->OnRecording(this); } -void AudioInputController::DoClose(const base::Closure& closed_task) { +void AudioInputController::DoClose() { DCHECK(message_loop_->BelongsToCurrentThread()); if (state_ != kClosed) { @@ -162,8 +162,6 @@ void AudioInputController::DoClose(const base::Closure& closed_task) { state_ = kClosed; } - - closed_task.Run(); } void AudioInputController::DoReportError(int code) { diff --git a/media/audio/audio_input_controller.h b/media/audio/audio_input_controller.h index 13815ee..59f8d16 100644 --- a/media/audio/audio_input_controller.h +++ b/media/audio/audio_input_controller.h @@ -148,12 +148,13 @@ class MEDIA_EXPORT AudioInputController virtual void Record(); // Closes the audio input stream. The state is changed and the resources - // are freed on the audio thread. |closed_task| is executed after that. + // are freed on the audio thread. |closed_task| is then executed on the thread + // that called Close(). // Callbacks (EventHandler and SyncWriter) must exist until |closed_task| // is called. // It is safe to call this method more than once. Calls after the first one // will have no effect. - // This method is called on the audio thread. + // This method trampolines to the audio thread. virtual void Close(const base::Closure& closed_task); // AudioInputCallback implementation. Threading details depends on the @@ -184,7 +185,7 @@ class MEDIA_EXPORT AudioInputController void DoCreate(AudioManager* audio_manager, const AudioParameters& params, const std::string& device_id); void DoRecord(); - void DoClose(const base::Closure& closed_task); + void DoClose(); void DoReportError(int code); // Methods which ensures that OnError() is triggered when data recording diff --git a/media/audio/audio_input_controller_unittest.cc b/media/audio/audio_input_controller_unittest.cc index 250f7c8..869610e 100644 --- a/media/audio/audio_input_controller_unittest.cc +++ b/media/audio/audio_input_controller_unittest.cc @@ -37,10 +37,8 @@ ACTION_P3(CheckCountAndPostQuitTask, count, limit, loop_or_proxy) { // Closes AudioOutputController synchronously. static void CloseAudioController(AudioInputController* controller) { - base::WaitableEvent closed_event(true, false); - controller->Close(base::Bind(&base::WaitableEvent::Signal, - base::Unretained(&closed_event))); - closed_event.Wait(); + controller->Close(MessageLoop::QuitClosure()); + MessageLoop::current()->Run(); } class MockAudioInputControllerEventHandler @@ -216,16 +214,11 @@ TEST_F(AudioInputControllerTest, CloseTwice) { controller->Record(); - base::WaitableEvent closed_event_1(true, false); - controller->Close(base::Bind(&base::WaitableEvent::Signal, - base::Unretained(&closed_event_1))); + controller->Close(MessageLoop::QuitClosure()); + MessageLoop::current()->Run(); - base::WaitableEvent closed_event_2(true, false); - controller->Close(base::Bind(&base::WaitableEvent::Signal, - base::Unretained(&closed_event_2))); - - closed_event_1.Wait(); - closed_event_2.Wait(); + controller->Close(MessageLoop::QuitClosure()); + MessageLoop::current()->Run(); } } // namespace media |