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}
This commit is contained in:
deadbeef
2015-12-14 18:15:29 -08:00
committed by Commit bot
parent 82ccfcf5ca
commit bd292465ee
3 changed files with 38 additions and 0 deletions

View File

@ -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;
}
}

View File

@ -374,6 +374,7 @@ class PeerConnection : public PeerConnectionInterface,
// label -> DataChannel
std::map<std::string, rtc::scoped_refptr<DataChannel>> rtp_data_channels_;
std::vector<rtc::scoped_refptr<DataChannel>> sctp_data_channels_;
std::vector<rtc::scoped_refptr<DataChannel>> sctp_data_channels_to_free_;
bool remote_peer_supports_msid_ = false;
rtc::scoped_ptr<RemoteMediaStreamFactory> remote_stream_factory_;

View File

@ -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<DataChannelInterface> 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);
}