Make AudioOutputController.Close() truly asynchronous.

Added closed_task parameter in this method. This parameter is used to
notify the caller when the stream is actually closed. Callbacks may
be called until closed_task is executed.

BUG=55755
TEST=Unittests, audio still works, doesn't crash

Review URL: http://codereview.chromium.org/3415007

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@59869 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 73 insertions, 35 deletions

diff --git a/chrome/browser/renderer_host/audio_renderer_host.cc b/chrome/browser/renderer_host/audio_renderer_host.cc
index 3219333..b4ba6ba 100644
--- a/chrome/browser/renderer_host/audio_renderer_host.cc
+++ b/chrome/browser/renderer_host/audio_renderer_host.cc
@@ -335,7 +335,14 @@ void AudioRendererHost::OnCreateStream(
   }
 
   scoped_ptr<AudioEntry> entry(new AudioEntry());
-  scoped_refptr<media::AudioOutputController> controller = NULL;
+  // Create the shared memory and share with the renderer process.
+  if (!entry->shared_memory.Create(L"", false, false, hardware_packet_size) ||
+      !entry->shared_memory.Map(entry->shared_memory.max_size())) {
+    // If creation of shared memory failed then send an error message.
+    SendErrorMessage(msg.routing_id(), stream_id);
+    return;
+  }
+
   if (low_latency) {
     // If this is the low latency mode, we need to construct a SyncReader first.
     scoped_ptr<AudioSyncReader> reader(
@@ -350,44 +357,32 @@ void AudioRendererHost::OnCreateStream(
     // If we have successfully created the SyncReader then assign it to the
     // entry and construct an AudioOutputController.
     entry->reader.reset(reader.release());
-    controller =
+    entry->controller =
         media::AudioOutputController::CreateLowLatency(
             this, params.params,
             hardware_packet_size,
             entry->reader.get());
   } else {
     // The choice of buffer capacity is based on experiment.
-    controller =
+    entry->controller =
         media::AudioOutputController::Create(this, params.params,
                                              hardware_packet_size,
                                              3 * hardware_packet_size);
   }
 
-  if (!controller) {
+  if (!entry->controller) {
     SendErrorMessage(msg.routing_id(), stream_id);
     return;
   }
 
   // If we have created the controller successfully create a entry and add it
   // to the map.
-  entry->controller = controller;
   entry->render_view_id = msg.routing_id();
   entry->stream_id = stream_id;
 
-  // Create the shared memory and share with the renderer process.
-  if (!entry->shared_memory.Create(L"", false, false, hardware_packet_size) ||
-      !entry->shared_memory.Map(entry->shared_memory.max_size())) {
-    // If creation of shared memory failed then close the controller and
-    // sends an error message.
-    controller->Close();
-    SendErrorMessage(msg.routing_id(), stream_id);
-    return;
-  }
-
-  // If everything is successful then add it to the map.
-  audio_entries_.insert(std::make_pair(
-      AudioEntryId(msg.routing_id(), stream_id),
-      entry.release()));
+ audio_entries_.insert(std::make_pair(
+     AudioEntryId(msg.routing_id(), stream_id),
+     entry.release()));
 }
 
 void AudioRendererHost::OnPlayStream(const IPC::Message& msg, int stream_id) {
@@ -431,10 +426,8 @@ void AudioRendererHost::OnCloseStream(const IPC::Message& msg, int stream_id) {
 
   AudioEntry* entry = LookupById(msg.routing_id(), stream_id);
 
-  // Note that closing an audio stream is a blocking operation. This call may
-  // block the IO thread for up to 100ms.
   if (entry)
-    DeleteEntry(entry);
+    CloseAndDeleteStream(entry);
 }
 
 void AudioRendererHost::OnSetVolume(const IPC::Message& msg, int stream_id,
@@ -519,10 +512,25 @@ void AudioRendererHost::SendErrorMessage(int32 render_view_id,
 void AudioRendererHost::DeleteEntries() {
   DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
 
-  while (!audio_entries_.empty()) {
-    DeleteEntry(audio_entries_.begin()->second);
+  for (AudioEntryMap::iterator i = audio_entries_.begin();
+       i != audio_entries_.end(); ++i) {
+    CloseAndDeleteStream(i->second);
   }
-  DCHECK(audio_entries_.empty());
+}
+
+void AudioRendererHost::CloseAndDeleteStream(AudioEntry* entry) {
+  if (!entry->pending_close) {
+    entry->controller->Close(
+        NewRunnableMethod(this, &AudioRendererHost::OnStreamClosed, entry));
+    entry->pending_close = true;
+  }
+}
+
+void AudioRendererHost::OnStreamClosed(AudioEntry* entry) {
+  // Delete the entry after we've closed the stream.
+  ChromeThread::PostTask(
+      ChromeThread::IO, FROM_HERE,
+      NewRunnableMethod(this, &AudioRendererHost::DeleteEntry, entry));
 }
 
 void AudioRendererHost::DeleteEntry(AudioEntry* entry) {
@@ -531,10 +539,7 @@ void AudioRendererHost::DeleteEntry(AudioEntry* entry) {
   // Delete the entry when this method goes out of scope.
   scoped_ptr<AudioEntry> entry_deleter(entry);
 
-  // Close the audio stream then remove the entry.
-  entry->controller->Close();
-
-  // Entry the entry from the map.
+  // Erase the entry from the map.
   audio_entries_.erase(
       AudioEntryId(entry->render_view_id, entry->stream_id));
 }
@@ -545,7 +550,7 @@ void AudioRendererHost::DeleteEntryOnError(AudioEntry* entry) {
   // Sends the error message first before we close the stream because
   // |entry| is destroyed in DeleteEntry().
   SendErrorMessage(entry->render_view_id, entry->stream_id);
-  DeleteEntry(entry);
+  CloseAndDeleteStream(entry);
 }
 
 AudioRendererHost::AudioEntry* AudioRendererHost::LookupById(
diff --git a/chrome/browser/renderer_host/audio_renderer_host.h b/chrome/browser/renderer_host/audio_renderer_host.h
index 074ec66..c67e0e7 100644
--- a/chrome/browser/renderer_host/audio_renderer_host.h
+++ b/chrome/browser/renderer_host/audio_renderer_host.h
@@ -86,7 +86,8 @@ class AudioRendererHost : public base::RefCountedThreadSafe<
     AudioEntry()
         : render_view_id(0),
           stream_id(0),
-          pending_buffer_request(false) {
+          pending_buffer_request(false),
+          pending_close(false) {
     }
 
     // The AudioOutputController that manages the audio stream.
@@ -106,6 +107,9 @@ class AudioRendererHost : public base::RefCountedThreadSafe<
     scoped_ptr<media::AudioOutputController::SyncReader> reader;
 
     bool pending_buffer_request;
+
+    // Set to true after we called Close() for the controller.
+    bool pending_close;
   };
 
   typedef std::map<AudioEntryId, AudioEntry*> AudioEntryMap;
@@ -219,6 +223,13 @@ class AudioRendererHost : public base::RefCountedThreadSafe<
   // Delete all audio entry and all audio streams
   void DeleteEntries();
 
+  // Closes the stream. The stream is then deleted in DeleteEntry() after it
+  // is closed.
+  void CloseAndDeleteStream(AudioEntry* entry);
+
+  // Called on the audio thread after the audio stream is closed.
+  void OnStreamClosed(AudioEntry* entry);
+
   // Delete an audio entry and close the related audio stream.
   void DeleteEntry(AudioEntry* entry);
 
diff --git a/chrome/browser/renderer_host/audio_renderer_host_unittest.cc b/chrome/browser/renderer_host/audio_renderer_host_unittest.cc
index ad2fa7e..08734ee 100644
--- a/chrome/browser/renderer_host/audio_renderer_host_unittest.cc
+++ b/chrome/browser/renderer_host/audio_renderer_host_unittest.cc
@@ -12,6 +12,7 @@
 #include "chrome/common/render_messages.h"
 #include "chrome/common/render_messages_params.h"
 #include "ipc/ipc_message_utils.h"
+#include "media/audio/audio_manager.h"
 #include "media/audio/fake_audio_output_stream.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -192,7 +193,7 @@ class AudioRendererHostTest : public testing::Test {
     host_ = NULL;
 
     // We need to continue running message_loop_ to complete all destructions.
-    message_loop_->RunAllPending();
+    SyncWithAudioThread();
 
     io_thread_.reset();
   }
@@ -309,17 +310,38 @@ class AudioRendererHostTest : public testing::Test {
     CHECK(controller) << "AudioOutputController not found";
 
     // Expect an error signal sent through IPC.
-    EXPECT_CALL(*host_, OnStreamError(kRouteId, kStreamId))
-        .WillOnce(QuitMessageLoop(message_loop_.get()));
+    EXPECT_CALL(*host_, OnStreamError(kRouteId, kStreamId));
 
     // Simulate an error sent from the audio device.
     host_->OnError(controller, 0);
-    message_loop_->Run();
+    SyncWithAudioThread();
 
     // Expect the audio stream record is removed.
     EXPECT_EQ(0u, host_->audio_entries_.size());
   }
 
+  // Called on the audio thread.
+  static void PostQuitMessageLoop(MessageLoop* message_loop) {
+    message_loop->PostTask(FROM_HERE, new MessageLoop::QuitTask());
+  }
+
+  // Called on the main thread.
+  static void PostQuitOnAudioThread(MessageLoop* message_loop) {
+    AudioManager::GetAudioManager()->GetMessageLoop()->PostTask(
+        FROM_HERE, NewRunnableFunction(&PostQuitMessageLoop, message_loop));
+  }
+
+  // SyncWithAudioThread() waits until all pending tasks on the audio thread
+  // are executed while also processing pending task in message_loop_ on the
+  // current thread. It is used to synchronize with the audio thread when we are
+  // closing an audio stream.
+  void SyncWithAudioThread() {
+    message_loop_->PostTask(
+        FROM_HERE, NewRunnableFunction(&PostQuitOnAudioThread,
+                                       message_loop_.get()));
+    message_loop_->Run();
+  }
+
   MessageLoop* message_loop() { return message_loop_.get(); }
   MockAudioRendererHost* host() { return host_; }
   void EnableRealDevice() { mock_stream_ = false; }