From 95e51f509c73eb4d62d25cacb052c97b88fefd7d Mon Sep 17 00:00:00 2001
From: "pbos@webrtc.org" <pbos@webrtc.org@4adac7df-926f-26a2-2b94-8c16560cd09d>
Date: Thu, 5 Sep 2013 12:38:54 +0000
Subject: [PATCH] Remove send and receive streams when destroyed.

Fixes crash where packets were sent to a receive stream that had been
destroyed but not removed from the ssrc mapping from call to receiver.
Added a repro case that reliably crashed before the fix.

BUG=
R=stefan@webrtc.org

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

git-svn-id: http://webrtc.googlecode.com/svn/trunk@4681 4adac7df-926f-26a2-2b94-8c16560cd09d
---
 webrtc/video_engine/internal/video_call.cc | 44 ++++++++++++---
 webrtc/video_engine/test/engine_tests.cc   | 66 ++++++++++++++++++++--
 2 files changed, 98 insertions(+), 12 deletions(-)

diff --git a/webrtc/video_engine/internal/video_call.cc b/webrtc/video_engine/internal/video_call.cc
index 97f606a17..6e50395c0 100644
--- a/webrtc/video_engine/internal/video_call.cc
+++ b/webrtc/video_engine/internal/video_call.cc
@@ -95,11 +95,25 @@ VideoSendStream* VideoCall::CreateSendStream(
 
 SendStreamState* VideoCall::DestroySendStream(
     webrtc::VideoSendStream* send_stream) {
-  if (send_stream == NULL) {
-    return NULL;
+  assert(send_stream != NULL);
+
+  VideoSendStream* send_stream_impl = NULL;
+  {
+    WriteLockScoped write_lock(*send_lock_);
+    for (std::map<uint32_t, VideoSendStream*>::iterator it =
+             send_ssrcs_.begin();
+         it != send_ssrcs_.end();
+         ++it) {
+      if (it->second == static_cast<VideoSendStream*>(send_stream)) {
+        send_stream_impl = it->second;
+        send_ssrcs_.erase(it);
+        break;
+      }
+    }
   }
-  // TODO(pbos): Remove it properly! Free the SSRCs!
-  delete static_cast<VideoSendStream*>(send_stream);
+
+  assert(send_stream_impl != NULL);
+  delete send_stream_impl;
 
   // TODO(pbos): Return its previous state
   return NULL;
@@ -122,11 +136,25 @@ VideoReceiveStream* VideoCall::CreateReceiveStream(
 
 void VideoCall::DestroyReceiveStream(
     webrtc::VideoReceiveStream* receive_stream) {
-  if (receive_stream == NULL) {
-    return;
+  assert(receive_stream != NULL);
+
+  VideoReceiveStream* receive_stream_impl = NULL;
+  {
+    WriteLockScoped write_lock(*receive_lock_);
+    for (std::map<uint32_t, VideoReceiveStream*>::iterator it =
+             receive_ssrcs_.begin();
+         it != receive_ssrcs_.end();
+         ++it) {
+      if (it->second == static_cast<VideoReceiveStream*>(receive_stream)) {
+        receive_stream_impl = it->second;
+        receive_ssrcs_.erase(it);
+        break;
+      }
+    }
   }
-  // TODO(pbos): Remove its SSRCs!
-  delete static_cast<VideoReceiveStream*>(receive_stream);
+
+  assert(receive_stream_impl != NULL);
+  delete receive_stream_impl;
 }
 
 uint32_t VideoCall::SendBitrateEstimate() {
diff --git a/webrtc/video_engine/test/engine_tests.cc b/webrtc/video_engine/test/engine_tests.cc
index 61a0f5c2f..7a449affe 100644
--- a/webrtc/video_engine/test/engine_tests.cc
+++ b/webrtc/video_engine/test/engine_tests.cc
@@ -256,13 +256,17 @@ class EngineTest : public ::testing::TestWithParam<EngineTestParams> {
 
   void StopSending() {
     frame_generator_capturer_->Stop();
-    send_stream_->StopSend();
-    receive_stream_->StopReceive();
+    if (send_stream_ != NULL)
+      send_stream_->StopSend();
+    if (receive_stream_ != NULL)
+      receive_stream_->StopReceive();
   }
 
   void DestroyStreams() {
-    sender_call_->DestroySendStream(send_stream_);
-    receiver_call_->DestroyReceiveStream(receive_stream_);
+    if (send_stream_ != NULL)
+      sender_call_->DestroySendStream(send_stream_);
+    if (receive_stream_ != NULL)
+      receiver_call_->DestroyReceiveStream(receive_stream_);
     send_stream_= NULL;
     receive_stream_ = NULL;
   }
@@ -553,5 +557,59 @@ TEST_P(EngineTest, DISABLED_ReceivesPliAndRecoversWithoutNack) {
   ReceivesPliAndRecovers(0);
 }
 
+TEST_P(EngineTest, SurvivesIncomingRtpPacketsToDestroyedReceiveStream) {
+  class PacketInputObserver : public PacketReceiver {
+   public:
+    explicit PacketInputObserver(PacketReceiver* receiver)
+        : receiver_(receiver), delivered_packet_(EventWrapper::Create()) {}
+
+    EventTypeWrapper Wait() {
+      return delivered_packet_->Wait(30 * 1000);
+    }
+
+   private:
+    virtual bool DeliverPacket(const uint8_t* packet, size_t length) {
+      if (RtpHeaderParser::IsRtcp(packet, static_cast<int>(length))) {
+        return receiver_->DeliverPacket(packet, length);
+      } else {
+        EXPECT_FALSE(receiver_->DeliverPacket(packet, length));
+        delivered_packet_->Set();
+        return false;
+      }
+    }
+
+    PacketReceiver* receiver_;
+    scoped_ptr<EventWrapper> delivered_packet_;
+  };
+
+  test::DirectTransport send_transport, receive_transport;
+
+  CreateCalls(&send_transport, &receive_transport);
+  PacketInputObserver input_observer(receiver_call_->Receiver());
+
+  send_transport.SetReceiver(&input_observer);
+  receive_transport.SetReceiver(sender_call_->Receiver());
+
+  CreateTestConfigs();
+
+  CreateStreams();
+  CreateFrameGenerator();
+
+  StartSending();
+
+  receiver_call_->DestroyReceiveStream(receive_stream_);
+  receive_stream_ = NULL;
+
+  // Wait() waits for a received packet.
+  EXPECT_EQ(kEventSignaled, input_observer.Wait());
+
+  StopSending();
+
+  DestroyStreams();
+
+  send_transport.StopSending();
+  receive_transport.StopSending();
+}
+
 INSTANTIATE_TEST_CASE_P(EngineTest, EngineTest, ::testing::Values(video_vga));
 }  // namespace webrtc