From 58d76cb6353dd2590c9d1f918735a3ea13dd4017 Mon Sep 17 00:00:00 2001
From: "pbos@webrtc.org" <pbos@webrtc.org@4adac7df-926f-26a2-2b94-8c16560cd09d>
Date: Thu, 8 Aug 2013 17:32:21 +0000
Subject: [PATCH] Delete Channels without ChannelManager lock.

Triggered Helgrind error, as deleting a Channel will also unregister a
module which has called GetChannel(), resulting in a cyclic lock graph.
This change will also allow other threads to access the ChannelManager
instance while Channels are deleted.

R=xians@webrtc.org

Review URL: https://webrtc-codereview.appspot.com/1946005

git-svn-id: http://webrtc.googlecode.com/svn/trunk@4505 4adac7df-926f-26a2-2b94-8c16560cd09d
---
 webrtc/voice_engine/channel_manager.cc | 30 ++++++++++++++++++--------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/webrtc/voice_engine/channel_manager.cc b/webrtc/voice_engine/channel_manager.cc
index 6cf935d16..8110bb6a2 100644
--- a/webrtc/voice_engine/channel_manager.cc
+++ b/webrtc/voice_engine/channel_manager.cc
@@ -78,22 +78,34 @@ void ChannelManager::GetAllChannels(std::vector<ChannelOwner>* channels) {
 }
 
 void ChannelManager::DestroyChannel(int32_t channel_id) {
-  CriticalSectionScoped crit(lock_.get());
   assert(channel_id >= 0);
+  // Holds a reference to a channel, this is used so that we never delete
+  // Channels while holding a lock, but rather when the method returns.
+  ChannelOwner reference(NULL);
+  {
+    CriticalSectionScoped crit(lock_.get());
 
-  for (std::vector<ChannelOwner>::iterator it = channels_.begin();
-       it != channels_.end();
-       ++it) {
-    if (it->channel()->ChannelId() == channel_id) {
-      channels_.erase(it);
-      break;
+    for (std::vector<ChannelOwner>::iterator it = channels_.begin();
+         it != channels_.end();
+         ++it) {
+      if (it->channel()->ChannelId() == channel_id) {
+        reference = *it;
+        channels_.erase(it);
+        break;
+      }
     }
   }
 }
 
 void ChannelManager::DestroyAllChannels() {
-  CriticalSectionScoped crit(lock_.get());
-  channels_.clear();
+  // Holds references so that Channels are not destroyed while holding this
+  // lock, but rather when the method returns.
+  std::vector<ChannelOwner> references;
+  {
+    CriticalSectionScoped crit(lock_.get());
+    references = channels_;
+    channels_.clear();
+  }
 }
 
 size_t ChannelManager::NumOfChannels() const {