Makes the sid of a closed DataChannel available to reuse per the spec.

BUG=2646
R=pthatcher@webrtc.org

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

git-svn-id: http://webrtc.googlecode.com/svn/trunk@6468 4adac7df-926f-26a2-2b94-8c16560cd09d

talk/app/webrtc/mediastreamsignaling.cc

@@ -274,6 +274,23 @@ bool MediaStreamSignaling::AddDataChannelFromOpenMessage(
   return true;
 }
 
+void MediaStreamSignaling::RemoveSctpDataChannel(int sid) {
+  for (SctpDataChannels::iterator iter = sctp_data_channels_.begin();
+       iter != sctp_data_channels_.end();
+       ++iter) {
+    if ((*iter)->id() == sid) {
+      sctp_data_channels_.erase(iter);
+
+      if (talk_base::IsEven(sid) && sid <= last_allocated_sctp_even_sid_) {
+        last_allocated_sctp_even_sid_ = sid - 2;
+      } else if (talk_base::IsOdd(sid) && sid <= last_allocated_sctp_odd_sid_) {
+        last_allocated_sctp_odd_sid_ = sid - 2;
+      }
+      return;
+    }
+  }
+}
+
 bool MediaStreamSignaling::AddLocalStream(MediaStreamInterface* local_stream) {
   if (local_streams_->find(local_stream->label()) != NULL) {
     LOG(LS_WARNING) << "MediaStream with label " << local_stream->label()
@@ -481,12 +498,19 @@ void MediaStreamSignaling::OnVideoChannelClose() {
 }
 
 void MediaStreamSignaling::OnDataChannelClose() {
-  RtpDataChannels::iterator it1 = rtp_data_channels_.begin();
+  // Use a temporary copy of the RTP/SCTP DataChannel list because the
-  for (; it1 != rtp_data_channels_.end(); ++it1) {
+  // DataChannel may callback to us and try to modify the list.
+  RtpDataChannels temp_rtp_dcs;
+  temp_rtp_dcs.swap(rtp_data_channels_);
+  RtpDataChannels::iterator it1 = temp_rtp_dcs.begin();
+  for (; it1 != temp_rtp_dcs.end(); ++it1) {
     it1->second->OnDataEngineClose();
   }
-  SctpDataChannels::iterator it2 = sctp_data_channels_.begin();
+
-  for (; it2 != sctp_data_channels_.end(); ++it2) {
+  SctpDataChannels temp_sctp_dcs;
+  temp_sctp_dcs.swap(sctp_data_channels_);
+  SctpDataChannels::iterator it2 = temp_sctp_dcs.begin();
+  for (; it2 != temp_sctp_dcs.end(); ++it2) {
     (*it2)->OnDataEngineClose();
   }
 }

talk/app/webrtc/mediastreamsignaling.h

@@ -198,6 +198,7 @@ class MediaStreamSignaling : public sigslot::has_slots<> {
   // After we receive an OPEN message, create a data channel and add it.
   bool AddDataChannelFromOpenMessage(const cricket::ReceiveDataParams& params,
                                      const talk_base::Buffer& payload);
+  void RemoveSctpDataChannel(int sid);
 
   // Returns a MediaSessionOptions struct with options decided by |constraints|,
   // the local MediaStreams and DataChannels.

talk/app/webrtc/mediastreamsignaling_unittest.cc

@@ -1141,6 +1141,47 @@ TEST_F(MediaStreamSignalingTest, SctpIdAllocationNoReuse) {
   EXPECT_NE(old_id, new_id);
 }
 
+// Verifies that SCTP ids of removed DataChannels can be reused.
+TEST_F(MediaStreamSignalingTest, SctpIdReusedForRemovedDataChannel) {
+  int odd_id = 1;
+  int even_id = 0;
+  AddDataChannel(cricket::DCT_SCTP, "a", odd_id);
+  AddDataChannel(cricket::DCT_SCTP, "a", even_id);
+
+  int allocated_id = -1;
+  ASSERT_TRUE(signaling_->AllocateSctpSid(talk_base::SSL_SERVER,
+                                          &allocated_id));
+  EXPECT_EQ(odd_id + 2, allocated_id);
+  AddDataChannel(cricket::DCT_SCTP, "a", allocated_id);
+
+  ASSERT_TRUE(signaling_->AllocateSctpSid(talk_base::SSL_CLIENT,
+                                          &allocated_id));
+  EXPECT_EQ(even_id + 2, allocated_id);
+  AddDataChannel(cricket::DCT_SCTP, "a", allocated_id);
+
+  signaling_->RemoveSctpDataChannel(odd_id);
+  signaling_->RemoveSctpDataChannel(even_id);
+
+  // Verifies that removed DataChannel ids are reused.
+  ASSERT_TRUE(signaling_->AllocateSctpSid(talk_base::SSL_SERVER,
+                                          &allocated_id));
+  EXPECT_EQ(odd_id, allocated_id);
+
+  ASSERT_TRUE(signaling_->AllocateSctpSid(talk_base::SSL_CLIENT,
+                                          &allocated_id));
+  EXPECT_EQ(even_id, allocated_id);
+
+  // Verifies that used higher DataChannel ids are not reused.
+  ASSERT_TRUE(signaling_->AllocateSctpSid(talk_base::SSL_SERVER,
+                                          &allocated_id));
+  EXPECT_NE(odd_id + 2, allocated_id);
+
+  ASSERT_TRUE(signaling_->AllocateSctpSid(talk_base::SSL_CLIENT,
+                                          &allocated_id));
+  EXPECT_NE(even_id + 2, allocated_id);
+
+}
+
 // Verifies that duplicated label is not allowed for RTP data channel.
 TEST_F(MediaStreamSignalingTest, RtpDuplicatedLabelNotAllowed) {
   AddDataChannel(cricket::DCT_RTP, "a", -1);

talk/app/webrtc/webrtcsession.cc

@@ -1144,6 +1144,8 @@ void WebRtcSession::AddSctpDataStream(uint32 sid) {
 }
 
 void WebRtcSession::RemoveSctpDataStream(uint32 sid) {
+  mediastream_signaling_->RemoveSctpDataChannel(static_cast<int>(sid));
+
   if (!data_channel_.get()) {
     LOG(LS_ERROR) << "RemoveDataChannelStreams called when data_channel_ is "
                   << "NULL.";

talk/base/common.cc

@@ -78,4 +78,12 @@ void LogAssert(const char* function, const char* file, int line,
   }
 }
 
+bool IsOdd(int n) {
+  return (n & 0x1);
+}
+
+bool IsEven(int n) {
+  return !IsOdd(n);
+}
+
 } // namespace talk_base

talk/base/common.h

@@ -116,6 +116,10 @@ typedef void (*AssertLogger)(const char* function,
 // only by one component.
 void SetCustomAssertLogger(AssertLogger logger);
 
+bool IsOdd(int n);
+
+bool IsEven(int n);
+
 }  // namespace talk_base
 
 #if ENABLE_DEBUG