From 057ecf01e4c2bc927481204192375dd5afd8d2f5 Mon Sep 17 00:00:00 2001
From: deadbeef <deadbeef@webrtc.org>
Date: Wed, 20 Jan 2016 14:30:43 -0800
Subject: [PATCH] Making WebRtcSession fire a destroyed signal.

This ensures the DtmfSender won't try to access it after it's
destroyed.

BUG=webrtc:5403

Review URL: https://codereview.webrtc.org/1590333004

Cr-Commit-Position: refs/heads/master@{#11327}
---
 talk/app/webrtc/dtmfsender.cc             |  6 ++----
 talk/app/webrtc/webrtcsession.cc          |  3 ++-
 talk/app/webrtc/webrtcsession.h           |  2 ++
 talk/app/webrtc/webrtcsession_unittest.cc | 13 +++++++++++++
 4 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/talk/app/webrtc/dtmfsender.cc b/talk/app/webrtc/dtmfsender.cc
index 3b311df320..30e2ce3873 100644
--- a/talk/app/webrtc/dtmfsender.cc
+++ b/talk/app/webrtc/dtmfsender.cc
@@ -99,6 +99,8 @@ DtmfSender::DtmfSender(AudioTrackInterface* track,
       inter_tone_gap_(kDtmfDefaultGapMs) {
   ASSERT(track_ != NULL);
   ASSERT(signaling_thread_ != NULL);
+  // TODO(deadbeef): Once we can use shared_ptr and weak_ptr,
+  // do that instead of relying on a "destroyed" signal.
   if (provider_) {
     ASSERT(provider_->GetOnDestroyedSignal() != NULL);
     provider_->GetOnDestroyedSignal()->connect(
@@ -107,10 +109,6 @@ DtmfSender::DtmfSender(AudioTrackInterface* track,
 }
 
 DtmfSender::~DtmfSender() {
-  if (provider_) {
-    ASSERT(provider_->GetOnDestroyedSignal() != NULL);
-    provider_->GetOnDestroyedSignal()->disconnect(this);
-  }
   StopSending();
 }
 
diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc
index d8f76379c1..c6d1856f03 100644
--- a/talk/app/webrtc/webrtcsession.cc
+++ b/talk/app/webrtc/webrtcsession.cc
@@ -590,6 +590,7 @@ WebRtcSession::~WebRtcSession() {
     SignalDataChannelDestroyed();
     channel_manager_->DestroyDataChannel(data_channel_.release());
   }
+  SignalDestroyed();
 
   LOG(LS_INFO) << "Session: " << id() << " is destroyed.";
 }
@@ -1428,7 +1429,7 @@ bool WebRtcSession::InsertDtmf(const std::string& track_id,
 }
 
 sigslot::signal0<>* WebRtcSession::GetOnDestroyedSignal() {
-  return &SignalVoiceChannelDestroyed;
+  return &SignalDestroyed;
 }
 
 bool WebRtcSession::SendData(const cricket::SendDataParams& params,
diff --git a/talk/app/webrtc/webrtcsession.h b/talk/app/webrtc/webrtcsession.h
index b79e0ec270..cd3f896726 100644
--- a/talk/app/webrtc/webrtcsession.h
+++ b/talk/app/webrtc/webrtcsession.h
@@ -327,6 +327,8 @@ class WebRtcSession : public AudioProviderInterface,
   sigslot::signal0<> SignalVideoChannelDestroyed;
   sigslot::signal0<> SignalDataChannelCreated;
   sigslot::signal0<> SignalDataChannelDestroyed;
+  // Called when the whole session is destroyed.
+  sigslot::signal0<> SignalDestroyed;
 
   // Called when a valid data channel OPEN message is received.
   // std::string represents the data channel label.
diff --git a/talk/app/webrtc/webrtcsession_unittest.cc b/talk/app/webrtc/webrtcsession_unittest.cc
index e81b8b5b54..0319b21b06 100644
--- a/talk/app/webrtc/webrtcsession_unittest.cc
+++ b/talk/app/webrtc/webrtcsession_unittest.cc
@@ -410,6 +410,8 @@ class WebRtcSessionTest
         allocator_.get(), &observer_));
     session_->SignalDataChannelOpenMessage.connect(
         this, &WebRtcSessionTest::OnDataChannelOpenMessage);
+    session_->GetOnDestroyedSignal()->connect(
+        this, &WebRtcSessionTest::OnSessionDestroyed);
 
     EXPECT_EQ(PeerConnectionInterface::kIceConnectionNew,
         observer_.ice_connection_state_);
@@ -428,6 +430,8 @@ class WebRtcSessionTest
     last_data_channel_config_ = config;
   }
 
+  void OnSessionDestroyed() { session_destroyed_ = true; }
+
   void Init() {
     PeerConnectionInterface::RTCConfiguration configuration;
     Init(nullptr, configuration);
@@ -1473,6 +1477,7 @@ class WebRtcSessionTest
   // Last values received from data channel creation signal.
   std::string last_data_channel_label_;
   InternalDataChannelInit last_data_channel_config_;
+  bool session_destroyed_;
 };
 
 TEST_P(WebRtcSessionTest, TestInitializeWithDtls) {
@@ -4313,6 +4318,14 @@ TEST_F(WebRtcSessionTest, TestPacketOptionsAndOnPacketSent) {
   TestPacketOptions();
 }
 
+// Make sure the signal from "GetOnDestroyedSignal()" fires when the session
+// is destroyed.
+TEST_F(WebRtcSessionTest, TestOnDestroyedSignal) {
+  Init();
+  session_.reset();
+  EXPECT_TRUE(session_destroyed_);
+}
+
 // TODO(bemasc): Add a TestIceStatesBundle with BUNDLE enabled.  That test
 // currently fails because upon disconnection and reconnection OnIceComplete is
 // called more than once without returning to IceGatheringGathering.