diff --git a/talk/app/webrtc/mediastreamsignaling.cc b/talk/app/webrtc/mediastreamsignaling.cc index c7fa67323..99f627a1d 100644 --- a/talk/app/webrtc/mediastreamsignaling.cc +++ b/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(); - for (; it1 != rtp_data_channels_.end(); ++it1) { + // Use a temporary copy of the RTP/SCTP DataChannel list because the + // 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(); } } diff --git a/talk/app/webrtc/mediastreamsignaling.h b/talk/app/webrtc/mediastreamsignaling.h index 8051289ab..737816616 100644 --- a/talk/app/webrtc/mediastreamsignaling.h +++ b/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. diff --git a/talk/app/webrtc/mediastreamsignaling_unittest.cc b/talk/app/webrtc/mediastreamsignaling_unittest.cc index 14b68e95c..150058eea 100644 --- a/talk/app/webrtc/mediastreamsignaling_unittest.cc +++ b/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); diff --git a/talk/app/webrtc/webrtcsession.cc b/talk/app/webrtc/webrtcsession.cc index 957557947..3ba1943de 100644 --- a/talk/app/webrtc/webrtcsession.cc +++ b/talk/app/webrtc/webrtcsession.cc @@ -1144,6 +1144,8 @@ void WebRtcSession::AddSctpDataStream(uint32 sid) { } void WebRtcSession::RemoveSctpDataStream(uint32 sid) { + mediastream_signaling_->RemoveSctpDataChannel(static_cast(sid)); + if (!data_channel_.get()) { LOG(LS_ERROR) << "RemoveDataChannelStreams called when data_channel_ is " << "NULL."; diff --git a/talk/base/common.cc b/talk/base/common.cc index 40755ddbd..9f63aa4da 100644 --- a/talk/base/common.cc +++ b/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 diff --git a/talk/base/common.h b/talk/base/common.h index 381b6b5ac..ed7d59ed6 100644 --- a/talk/base/common.h +++ b/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