From dfbcf8161ef44bb952561a9a742c3d4e4487e405 Mon Sep 17 00:00:00 2001
From: "jiayl@webrtc.org" <jiayl@webrtc.org>
Date: Fri, 5 Sep 2014 00:01:12 +0000
Subject: [PATCH] Fix an issue in MediaStreamSignaling that a remotely create
 DataChannel is added to the list twice.

BUG=3778
R=pthatcher@webrtc.org

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

git-svn-id: http://webrtc.googlecode.com/svn/trunk@7073 4adac7df-926f-26a2-2b94-8c16560cd09d
---
 talk/app/webrtc/mediastreamsignaling.cc       |  3 +-
 .../webrtc/mediastreamsignaling_unittest.cc   | 40 ++++++++++++++++---
 2 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/talk/app/webrtc/mediastreamsignaling.cc b/talk/app/webrtc/mediastreamsignaling.cc
index 9a22ad4ca..df51ba160 100644
--- a/talk/app/webrtc/mediastreamsignaling.cc
+++ b/talk/app/webrtc/mediastreamsignaling.cc
@@ -326,12 +326,13 @@ bool MediaStreamSignaling::AddDataChannelFromOpenMessage(
     LOG(LS_ERROR) << "Failed to create DataChannel from the OPEN message.";
     return false;
   }
-  sctp_data_channels_.push_back(channel);
+
   stream_observer_->OnAddDataChannel(channel);
   return true;
 }
 
 void MediaStreamSignaling::RemoveSctpDataChannel(int sid) {
+  ASSERT(sid >= 0);
   for (SctpDataChannels::iterator iter = sctp_data_channels_.begin();
        iter != sctp_data_channels_.end();
        ++iter) {
diff --git a/talk/app/webrtc/mediastreamsignaling_unittest.cc b/talk/app/webrtc/mediastreamsignaling_unittest.cc
index 2ccfd8d8e..84f67b959 100644
--- a/talk/app/webrtc/mediastreamsignaling_unittest.cc
+++ b/talk/app/webrtc/mediastreamsignaling_unittest.cc
@@ -261,14 +261,20 @@ static bool CompareStreamCollections(StreamCollectionInterface* s1,
 class FakeDataChannelFactory : public webrtc::DataChannelFactory {
  public:
   FakeDataChannelFactory(FakeDataChannelProvider* provider,
-                         cricket::DataChannelType dct)
-      : provider_(provider), type_(dct) {}
+                         cricket::DataChannelType dct,
+                         webrtc::MediaStreamSignaling* media_stream_signaling)
+      : provider_(provider),
+        type_(dct),
+        media_stream_signaling_(media_stream_signaling) {}
 
   virtual rtc::scoped_refptr<webrtc::DataChannel> CreateDataChannel(
       const std::string& label,
       const webrtc::InternalDataChannelInit* config) {
     last_init_ = *config;
-    return webrtc::DataChannel::Create(provider_, type_, label, *config);
+    rtc::scoped_refptr<webrtc::DataChannel> data_channel =
+        webrtc::DataChannel::Create(provider_, type_, label, *config);
+    media_stream_signaling_->AddDataChannel(data_channel);
+    return data_channel;
   }
 
   const webrtc::InternalDataChannelInit& last_init() const {
@@ -278,6 +284,7 @@ class FakeDataChannelFactory : public webrtc::DataChannelFactory {
  private:
   FakeDataChannelProvider* provider_;
   cricket::DataChannelType type_;
+  webrtc::MediaStreamSignaling* media_stream_signaling_;
   webrtc::InternalDataChannelInit last_init_;
 };
 
@@ -1265,7 +1272,8 @@ TEST_F(MediaStreamSignalingTest, SctpDuplicatedLabelAllowed) {
 // message.
 TEST_F(MediaStreamSignalingTest, CreateDataChannelFromOpenMessage) {
   FakeDataChannelFactory fake_factory(data_channel_provider_.get(),
-                                      cricket::DCT_SCTP);
+                                      cricket::DCT_SCTP,
+                                      signaling_.get());
   signaling_->SetDataChannelFactory(&fake_factory);
   webrtc::DataChannelInit config;
   config.id = 1;
@@ -1285,7 +1293,8 @@ TEST_F(MediaStreamSignalingTest, DuplicatedLabelFromOpenMessageAllowed) {
   AddDataChannel(cricket::DCT_SCTP, "a", -1);
 
   FakeDataChannelFactory fake_factory(data_channel_provider_.get(),
-                                      cricket::DCT_SCTP);
+                                      cricket::DCT_SCTP,
+                                      signaling_.get());
   signaling_->SetDataChannelFactory(&fake_factory);
   webrtc::DataChannelInit config;
   config.id = 0;
@@ -1314,3 +1323,24 @@ TEST_F(MediaStreamSignalingTest,
   signaling_->OnRemoteSctpDataChannelClosed(config.id);
   EXPECT_EQ(webrtc::DataChannelInterface::kClosed, data_channel->state());
 }
+
+// Verifies that DataChannel added from OPEN message is added to
+// MediaStreamSignaling only once (webrtc issue 3778).
+TEST_F(MediaStreamSignalingTest, DataChannelFromOpenMessageAddedOnce) {
+  FakeDataChannelFactory fake_factory(data_channel_provider_.get(),
+                                      cricket::DCT_SCTP,
+                                      signaling_.get());
+  signaling_->SetDataChannelFactory(&fake_factory);
+  webrtc::DataChannelInit config;
+  config.id = 1;
+  rtc::Buffer payload;
+  webrtc::WriteDataChannelOpenMessage("a", config, &payload);
+  cricket::ReceiveDataParams params;
+  params.ssrc = config.id;
+  EXPECT_TRUE(signaling_->AddDataChannelFromOpenMessage(params, payload));
+  EXPECT_TRUE(signaling_->HasDataChannels());
+
+  // Removes the DataChannel and verifies that no DataChannel is left.
+  signaling_->RemoveSctpDataChannel(config.id);
+  EXPECT_FALSE(signaling_->HasDataChannels());
+}