More AudioMessageFilter cleanup. Fix crash.

Removes the |lock_| used by AudioMessageFilter for adding and removing
delegates in favor of ensuring those calls happen on the IO thread.

Also, currently message filters are expected to take care of cleaning
up delegates when the IPC closes unexpectedly. I broke this in my last
patch set, http://crrev.com/184478

BUG=178499
TEST=|delegates_| is empty after OnChannelClosing() completes. |stream_id_|
is correctly associated on various audio pathways.

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

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

5 files changed, 43 insertions, 53 deletions

diff --git a/content/renderer/media/audio_message_filter.cc b/content/renderer/media/audio_message_filter.cc
index fd349a2..2cb7eb8 100644
--- a/content/renderer/media/audio_message_filter.cc
+++ b/content/renderer/media/audio_message_filter.cc
@@ -22,7 +22,6 @@ AudioMessageFilter* AudioMessageFilter::Get() {
 AudioMessageFilter::AudioMessageFilter(
     const scoped_refptr<base::MessageLoopProxy>& io_message_loop)
     : channel_(NULL),
-      next_stream_id_(1),
       audio_hardware_config_(NULL),
       io_message_loop_(io_message_loop) {
   DCHECK(!filter_);
@@ -30,15 +29,13 @@ AudioMessageFilter::AudioMessageFilter(
 }
 
 int AudioMessageFilter::AddDelegate(media::AudioOutputIPCDelegate* delegate) {
-  base::AutoLock auto_lock(lock_);
-  const int id = next_stream_id_++;
-  delegates_.insert(std::make_pair(id, delegate));
-  return id;
+  DCHECK(io_message_loop_->BelongsToCurrentThread());
+  return delegates_.Add(delegate);
 }
 
 void AudioMessageFilter::RemoveDelegate(int id) {
-  base::AutoLock auto_lock(lock_);
-  delegates_.erase(id);
+  DCHECK(io_message_loop_->BelongsToCurrentThread());
+  delegates_.Remove(id);
 }
 
 void AudioMessageFilter::CreateStream(int stream_id,
@@ -106,18 +103,19 @@ void AudioMessageFilter::OnChannelClosing() {
   DCHECK(io_message_loop_->BelongsToCurrentThread());
   channel_ = NULL;
 
-  base::AutoLock auto_lock(lock_);
-  DLOG_IF(WARNING, !delegates_.empty())
+  DLOG_IF(WARNING, !delegates_.IsEmpty())
       << "Not all audio devices have been closed.";
 
-  for (DelegateMap::const_iterator it = delegates_.begin();
-       it != delegates_.end(); ++it) {
-    it->second->OnIPCClosed();
+  IDMap<media::AudioOutputIPCDelegate>::iterator it(&delegates_);
+  while (!it.IsAtEnd()) {
+    it.GetCurrentValue()->OnIPCClosed();
+    delegates_.Remove(it.GetCurrentKey());
+    it.Advance();
   }
 }
 
 AudioMessageFilter::~AudioMessageFilter() {
-  CHECK(delegates_.empty());
+  CHECK(delegates_.IsEmpty());
   DCHECK_EQ(filter_, this);
   filter_ = NULL;
 }
@@ -137,31 +135,27 @@ void AudioMessageFilter::OnStreamCreated(
   base::SyncSocket::Handle socket_handle = socket_descriptor.fd;
 #endif
 
-  {
-    base::AutoLock auto_lock(lock_);
-    DelegateMap::const_iterator it = delegates_.find(stream_id);
-    if (it != delegates_.end()) {
-      it->second->OnStreamCreated(handle, socket_handle, length);
-      return;
-    }
+  media::AudioOutputIPCDelegate* delegate = delegates_.Lookup(stream_id);
+  if (!delegate) {
+    DLOG(WARNING) << "Got OnStreamCreated() event for a non-existent or removed"
+                  << " audio renderer. (stream_id=" << stream_id << ").";
+    base::SharedMemory::CloseHandle(handle);
+    base::SyncSocket socket(socket_handle);
+    return;
   }
-
-  DLOG(WARNING) << "Got audio stream event for a non-existent or removed"
-                   " audio renderer. (stream_id=" << stream_id << ").";
-  base::SharedMemory::CloseHandle(handle);
-  base::SyncSocket socket(socket_handle);
+  delegate->OnStreamCreated(handle, socket_handle, length);
 }
 
 void AudioMessageFilter::OnStreamStateChanged(
     int stream_id, media::AudioOutputIPCDelegate::State state) {
   DCHECK(io_message_loop_->BelongsToCurrentThread());
-
-  base::AutoLock auto_lock(lock_);
-  DelegateMap::const_iterator it = delegates_.find(stream_id);
-  DLOG_IF(WARNING, it == delegates_.end())
-      << "No delegate found for state change. " << state;
-  if (it != delegates_.end())
-    it->second->OnStateChanged(state);
+  media::AudioOutputIPCDelegate* delegate = delegates_.Lookup(stream_id);
+  if (!delegate) {
+    DLOG(WARNING) << "Got OnStreamStateChanged() event for a non-existent or"
+                  << " removed audio renderer.  State: " << state;
+    return;
+  }
+  delegate->OnStateChanged(state);
 }
 
 void AudioMessageFilter::OnOutputDeviceChanged(int stream_id,
diff --git a/content/renderer/media/audio_message_filter.h b/content/renderer/media/audio_message_filter.h
index 2d9f4a2..7c352d6 100644
--- a/content/renderer/media/audio_message_filter.h
+++ b/content/renderer/media/audio_message_filter.h
@@ -6,8 +6,7 @@
 #define CONTENT_RENDERER_MEDIA_AUDIO_MESSAGE_FILTER_H_
 
 #include "base/gtest_prod_util.h"
-#include "base/hash_tables.h"
-#include "base/message_loop_proxy.h"
+#include "base/id_map.h"
 #include "base/shared_memory.h"
 #include "base/sync_socket.h"
 #include "base/synchronization/lock.h"
@@ -99,20 +98,16 @@ class CONTENT_EXPORT AudioMessageFilter
   // The singleton instance for this filter.
   static AudioMessageFilter* filter_;
 
-  // IPC channel for Send(), must only be accesed on |io_message_loop_|.
+  // IPC channel for Send(); must only be accesed on |io_message_loop_|.
   IPC::Channel* channel_;
 
-  // Unique ID to use for next added delegate.
-  int next_stream_id_;
-
-  // Guards all variables below which are accessed from multiple threads.
-  base::Lock lock_;
-
-  // A map of stream ids to delegates.
-  typedef base::hash_map<int, media::AudioOutputIPCDelegate*> DelegateMap;
-  DelegateMap delegates_;
+  // A map of stream ids to delegates; must only be accessed on
+  // |io_message_loop_|.
+  IDMap<media::AudioOutputIPCDelegate> delegates_;
 
   // Audio hardware configuration to update when OnOutputDeviceChanged() fires.
+  // Access is guarded by |lock_|.
+  base::Lock lock_;
   media::AudioHardwareConfig* audio_hardware_config_;
 
   // Message loop on which IPC calls are driven.
diff --git a/content/renderer/media/renderer_audio_output_device.cc b/content/renderer/media/renderer_audio_output_device.cc
index 1cbf1e3..678bbec 100644
--- a/content/renderer/media/renderer_audio_output_device.cc
+++ b/content/renderer/media/renderer_audio_output_device.cc
@@ -42,15 +42,18 @@ void RendererAudioOutputDevice::SetSourceRenderView(int render_view_id) {
 }
 
 void RendererAudioOutputDevice::OnStart() {
+  DCHECK(message_loop()->BelongsToCurrentThread());
   is_after_stream_created_ = true;
   OnSourceChange(source_render_view_id_);
 }
 
 void RendererAudioOutputDevice::OnStop() {
+  DCHECK(message_loop()->BelongsToCurrentThread());
   is_after_stream_created_ = false;
 }
 
 void RendererAudioOutputDevice::OnSourceChange(int render_view_id) {
+  DCHECK(message_loop()->BelongsToCurrentThread());
   source_render_view_id_ = render_view_id;
   if (is_after_stream_created_ && source_render_view_id_ != MSG_ROUTING_NONE) {
     AudioMessageFilter* const filter =
diff --git a/media/audio/audio_output_device.cc b/media/audio/audio_output_device.cc
index e95d21b..fd62dc11f 100644
--- a/media/audio/audio_output_device.cc
+++ b/media/audio/audio_output_device.cc
@@ -45,11 +45,11 @@ AudioOutputDevice::AudioOutputDevice(
     : ScopedLoopObserver(io_loop),
       callback_(NULL),
       ipc_(ipc),
+      stream_id_(0),
       state_(IDLE),
       play_on_start_(true),
       stopping_hack_(false) {
   CHECK(ipc_);
-  stream_id_ = ipc_->AddDelegate(this);
 }
 
 void AudioOutputDevice::Initialize(const AudioParameters& params,
@@ -64,9 +64,6 @@ AudioOutputDevice::~AudioOutputDevice() {
   // The current design requires that the user calls Stop() before deleting
   // this class.
   DCHECK(audio_thread_.IsStopped());
-
-  if (ipc_)
-    ipc_->RemoveDelegate(stream_id_);
 }
 
 void AudioOutputDevice::Start() {
@@ -112,6 +109,7 @@ bool AudioOutputDevice::SetVolume(double volume) {
 void AudioOutputDevice::CreateStreamOnIOThread(const AudioParameters& params) {
   DCHECK(message_loop()->BelongsToCurrentThread());
   if (state_ == IDLE) {
+    stream_id_ = ipc_->AddDelegate(this);
     state_ = CREATING_STREAM;
     ipc_->CreateStream(stream_id_, params);
   }
@@ -148,7 +146,9 @@ void AudioOutputDevice::ShutDownOnIOThread() {
   // Make sure we don't call shutdown more than once.
   if (state_ >= CREATING_STREAM) {
     ipc_->CloseStream(stream_id_);
+    ipc_->RemoveDelegate(stream_id_);
     state_ = IDLE;
+    stream_id_ = 0;
   }
 
   // We can run into an issue where ShutDownOnIOThread is called right after
diff --git a/media/audio/audio_output_device_unittest.cc b/media/audio/audio_output_device_unittest.cc
index 07a752d..5628f66 100644
--- a/media/audio/audio_output_device_unittest.cc
+++ b/media/audio/audio_output_device_unittest.cc
@@ -155,9 +155,6 @@ AudioOutputDeviceTest::AudioOutputDeviceTest()
       CHANNEL_LAYOUT_STEREO, input_channels_,
       48000, 16, 1024);
 
-  EXPECT_CALL(audio_output_ipc_, AddDelegate(_))
-      .WillOnce(Return(kStreamId));
-
   audio_device_ = new AudioOutputDevice(
       &audio_output_ipc_, io_loop_.message_loop_proxy());
 
@@ -168,14 +165,13 @@ AudioOutputDeviceTest::AudioOutputDeviceTest()
 }
 
 AudioOutputDeviceTest::~AudioOutputDeviceTest() {
-  EXPECT_CALL(audio_output_ipc_, RemoveDelegate(kStreamId));
-
   audio_device_ = NULL;
 }
 
 void AudioOutputDeviceTest::StartAudioDevice() {
   audio_device_->Start();
 
+  EXPECT_CALL(audio_output_ipc_, AddDelegate(_)).WillOnce(Return(kStreamId));
   EXPECT_CALL(audio_output_ipc_, CreateStream(kStreamId, _));
 
   io_loop_.RunUntilIdle();
@@ -247,6 +243,7 @@ void AudioOutputDeviceTest::StopAudioDevice() {
   audio_device_->Stop();
 
   EXPECT_CALL(audio_output_ipc_, CloseStream(kStreamId));
+  EXPECT_CALL(audio_output_ipc_, RemoveDelegate(kStreamId));
 
   io_loop_.RunUntilIdle();
 }
@@ -282,6 +279,7 @@ TEST_P(AudioOutputDeviceTest, StopBeforeRender) {
   // Expect us to shutdown IPC but not to render anything despite the stream
   // getting created.
   EXPECT_CALL(audio_output_ipc_, CloseStream(kStreamId));
+  EXPECT_CALL(audio_output_ipc_, RemoveDelegate(kStreamId));
   CreateStream();
 }