From a1f567ae9012a8de573b5bde492dd9ca0d17f137 Mon Sep 17 00:00:00 2001 From: deadbeef Date: Thu, 10 Dec 2015 11:17:42 -0800 Subject: [PATCH] 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 Review URL: https://codereview.webrtc.org/1513143003 Cr-Commit-Position: refs/heads/master@{#10977} --- talk/app/webrtc/peerconnection.cc | 12 --------- .../webrtc/peerconnectionendtoend_unittest.cc | 27 ------------------- 2 files changed, 39 deletions(-) diff --git a/talk/app/webrtc/peerconnection.cc b/talk/app/webrtc/peerconnection.cc index f1ff4e6d36..933dc83c25 100644 --- a/talk/app/webrtc/peerconnection.cc +++ b/talk/app/webrtc/peerconnection.cc @@ -107,7 +107,6 @@ enum { MSG_SET_SESSIONDESCRIPTION_FAILED, MSG_CREATE_SESSIONDESCRIPTION_FAILED, MSG_GETSTATS, - MSG_DELETE, }; struct SetSessionDescriptionMsg : public rtc::MessageData { @@ -598,8 +597,6 @@ PeerConnection::PeerConnection(PeerConnectionFactory* factory) PeerConnection::~PeerConnection() { TRACE_EVENT0("webrtc", "PeerConnection::~PeerConnection"); RTC_DCHECK(signaling_thread()->IsCurrent()); - // Finish any pending deletions. - signaling_thread()->Clear(this, MSG_DELETE, nullptr); // Need to detach RTP senders/receivers from WebRtcSession, // since it's about to be destroyed. for (const auto& sender : senders_) { @@ -1332,10 +1329,6 @@ void PeerConnection::OnMessage(rtc::Message* msg) { delete param; break; } - case MSG_DELETE: { - delete msg->pdata; - break; - } default: RTC_DCHECK(false && "Not implemented"); break; @@ -1915,11 +1908,6 @@ void PeerConnection::OnSctpDataChannelClosed(DataChannel* 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. - signaling_thread()->Post( - this, MSG_DELETE, - new rtc::TypedMessageData>(channel)); sctp_data_channels_.erase(it); return; } diff --git a/talk/app/webrtc/peerconnectionendtoend_unittest.cc b/talk/app/webrtc/peerconnectionendtoend_unittest.cc index aff117e1bb..1d7bb9211e 100644 --- a/talk/app/webrtc/peerconnectionendtoend_unittest.cc +++ b/talk/app/webrtc/peerconnectionendtoend_unittest.cc @@ -402,30 +402,3 @@ 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); -}