From bd292465ee6d8219b04f17e2ffc0790313167f01 Mon Sep 17 00:00:00 2001 From: deadbeef Date: Mon, 14 Dec 2015 18:15:29 -0800 Subject: [PATCH] Reland of Free SCTP data channels asynchronously in PeerConnection. (patchset #1 id:1 of https://codereview.webrtc.org/1513143003/ ) Original issue's description: > Revert of Free SCTP data channels asynchronously in PeerConnection. (patchset #3 id:40001 of https://codereview.webrtc.org/1492383002/ ) > > Reason for revert: > Breaks WebrtcTransportTest.DataStream, due to different rtc::Thread implementation on Chromium. > > Original issue's description: > > Free SCTP data channels asynchronously in PeerConnection. > > > > This is needed so that the data channel isn't deleted while one of its > > own methods is on the call stack. > > > > BUG=565048 > > > > Committed: https://crrev.com/386869247f28e72a00307a1b5c92465eea343ad2 > > Cr-Commit-Position: refs/heads/master@{#10923} > > TBR=pthatcher@webrtc.org > NOPRESUBMIT=true > NOTREECHECKS=true > NOTRY=true > BUG=565048 > > Committed: https://crrev.com/a1f567ae9012a8de573b5bde492dd9ca0d17f137 > Cr-Commit-Position: refs/heads/master@{#10977} BUG=565048 Review URL: https://codereview.webrtc.org/1516943002 Cr-Commit-Position: refs/heads/master@{#11015} --- talk/app/webrtc/peerconnection.cc | 10 +++++++ talk/app/webrtc/peerconnection.h | 1 + .../webrtc/peerconnectionendtoend_unittest.cc | 27 +++++++++++++++++++ 3 files changed, 38 insertions(+) diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc index 83e29191c8..83380e17b5 100644 --- a/talk/app/webrtc/peerconnection.cc +++ b/talk/app/webrtc/peerconnection.cc @@ -108,6 +108,7 @@ enum { MSG_SET_SESSIONDESCRIPTION_FAILED, MSG_CREATE_SESSIONDESCRIPTION_FAILED, MSG_GETSTATS, + MSG_FREE_DATACHANNELS, }; struct SetSessionDescriptionMsg : public rtc::MessageData { @@ -1332,6 +1333,10 @@ void PeerConnection::OnMessage(rtc::Message* msg) { delete param; break; } + case MSG_FREE_DATACHANNELS: { + sctp_data_channels_to_free_.clear(); + break; + } default: RTC_DCHECK(false && "Not implemented"); break; @@ -1905,13 +1910,18 @@ void PeerConnection::AllocateSctpSids(rtc::SSLRole role) { } void PeerConnection::OnSctpDataChannelClosed(DataChannel* channel) { + RTC_DCHECK(signaling_thread()->IsCurrent()); for (auto it = sctp_data_channels_.begin(); it != sctp_data_channels_.end(); ++it) { if (it->get() == channel) { if (channel->id() >= 0) { sid_allocator_.ReleaseSid(channel->id()); } + // Since this method is triggered by a signal from the DataChannel, + // we can't free it directly here; we need to free it asynchronously. + sctp_data_channels_to_free_.push_back(*it); sctp_data_channels_.erase(it); + signaling_thread()->Post(this, MSG_FREE_DATACHANNELS, nullptr); return; } } diff --git a/talk/app/webrtc/peerconnection.h b/talk/app/webrtc/peerconnection.h index 12f8d1bd4b..9cb3635690 100644 --- a/talk/app/webrtc/peerconnection.h +++ b/talk/app/webrtc/peerconnection.h @@ -374,6 +374,7 @@ class PeerConnection : public PeerConnectionInterface, // label -> DataChannel std::map> rtp_data_channels_; std::vector> sctp_data_channels_; + std::vector> sctp_data_channels_to_free_; bool remote_peer_supports_msid_ = false; rtc::scoped_ptr remote_stream_factory_; diff --git a/talk/app/webrtc/peerconnectionendtoend_unittest.cc b/talk/app/webrtc/peerconnectionendtoend_unittest.cc index 1d7bb9211e..aff117e1bb 100644 --- a/talk/app/webrtc/peerconnectionendtoend_unittest.cc +++ b/talk/app/webrtc/peerconnectionendtoend_unittest.cc @@ -402,3 +402,30 @@ TEST_F(PeerConnectionEndToEndTest, CloseDataChannels(caller_dc, callee_signaled_data_channels_, 1); } + +// This tests that if a data channel is closed remotely while not referenced +// by the application (meaning only the PeerConnection contributes to its +// reference count), no memory access violation will occur. +// See: https://code.google.com/p/chromium/issues/detail?id=565048 +TEST_F(PeerConnectionEndToEndTest, CloseDataChannelRemotelyWhileNotReferenced) { + MAYBE_SKIP_TEST(rtc::SSLStreamAdapter::HaveDtlsSrtp); + + CreatePcs(); + + webrtc::DataChannelInit init; + rtc::scoped_refptr caller_dc( + caller_->CreateDataChannel("data", init)); + + Negotiate(); + WaitForConnection(); + + WaitForDataChannelsToOpen(caller_dc, callee_signaled_data_channels_, 0); + // This removes the reference to the remote data channel that we hold. + callee_signaled_data_channels_.clear(); + caller_dc->Close(); + EXPECT_EQ_WAIT(DataChannelInterface::kClosed, caller_dc->state(), kMaxWait); + + // Wait for a bit longer so the remote data channel will receive the + // close message and be destroyed. + rtc::Thread::Current()->ProcessMessages(100); +}