Implement proper SCTP data channel closing procedure.
The proper closing procedure is:
1. Alice resets outgoing stream.
2. Bob receives incoming stream reset, resets his outgoing stream.
3. Alice receives incoming stream reset; channel closed!
4. Bob receives acknowledgement of reset; channel closed!
https://tools.ietf.org/html/draft-ietf-rtcweb-data-channel-13#section-6.7
However, up until now we've been sending both an incoming and outgoing reset
from the side initiating the closing procedure, and doing nothing on the remote
side.
This means that if you call "Close" and the remote endpoint is using an old
version of WebRTC, the channel's state will be stuck at "closing" since the
remote endpoint won't send a reset. Which is already what happens when Firefox
is talking to Chrome.
This CL also fixes an issue where the DataChannel's state prematurely went to
"closed" before the closing procedure was complete. Which could result in a
new DataChannel attempting to re-use the ID and failing.
TBR=magjed@webrtc.org
Bug: chromium:449934, webrtc:4453
Change-Id: Ic1ba813e46538c6c65868961aae6a9780d68a5e2
Reviewed-on: https://webrtc-review.googlesource.com/79061
Reviewed-by: Taylor Brandstetter <deadbeef@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Commit-Queue: Taylor Brandstetter <deadbeef@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#23478}diff --git a/pc/datachannel.cc b/pc/datachannel.cc
index 525b6f9..b7a717c 100644
--- a/pc/datachannel.cc
+++ b/pc/datachannel.cc
@@ -229,6 +229,7 @@
send_ssrc_ = 0;
send_ssrc_set_ = false;
SetState(kClosing);
+ // Will send queued data before beginning the underlying closing procedure.
UpdateState();
}
@@ -251,7 +252,9 @@
// blocked.
RTC_DCHECK(data_channel_type_ == cricket::DCT_SCTP);
if (!QueueSendDataMessage(buffer)) {
- Close();
+ RTC_LOG(LS_ERROR) << "Closing the DataChannel due to a failure to queue "
+ "additional data.";
+ CloseAbruptly();
}
return true;
}
@@ -276,11 +279,6 @@
UpdateState();
}
-// The remote peer request that this channel shall be closed.
-void DataChannel::RemotePeerRequestClose() {
- DoClose();
-}
-
void DataChannel::SetSctpSid(int sid) {
RTC_DCHECK_LT(config_.id, 0);
RTC_DCHECK_GE(sid, 0);
@@ -293,6 +291,33 @@
provider_->AddSctpDataStream(sid);
}
+void DataChannel::OnClosingProcedureStartedRemotely(int sid) {
+ if (data_channel_type_ == cricket::DCT_SCTP && sid == config_.id &&
+ state_ != kClosing && state_ != kClosed) {
+ // Don't bother sending queued data since the side that initiated the
+ // closure wouldn't receive it anyway. See crbug.com/559394 for a lengthy
+ // discussion about this.
+ queued_send_data_.Clear();
+ queued_control_data_.Clear();
+ // Just need to change state to kClosing, SctpTransport will handle the
+ // rest of the closing procedure and OnClosingProcedureComplete will be
+ // called later.
+ started_closing_procedure_ = true;
+ SetState(kClosing);
+ }
+}
+
+void DataChannel::OnClosingProcedureComplete(int sid) {
+ if (data_channel_type_ == cricket::DCT_SCTP && sid == config_.id) {
+ // If the closing procedure is complete, we should have finished sending
+ // all pending data and transitioned to kClosing already.
+ RTC_DCHECK_EQ(state_, kClosing);
+ RTC_DCHECK(queued_send_data_.Empty());
+ DisconnectFromProvider();
+ SetState(kClosed);
+ }
+}
+
void DataChannel::OnTransportChannelCreated() {
RTC_DCHECK(data_channel_type_ == cricket::DCT_SCTP);
if (!connected_to_provider_) {
@@ -306,11 +331,15 @@
}
void DataChannel::OnTransportChannelDestroyed() {
- // This method needs to synchronously close the data channel, which means any
- // queued data needs to be discarded.
- queued_send_data_.Clear();
- queued_control_data_.Clear();
- DoClose();
+ // The SctpTransport is going away (for example, because the SCTP m= section
+ // was rejected), so we need to close abruptly.
+ CloseAbruptly();
+}
+
+// The remote peer request that this channel shall be closed.
+void DataChannel::RemotePeerRequestClose() {
+ RTC_DCHECK(data_channel_type_ == cricket::DCT_RTP);
+ CloseAbruptly();
}
void DataChannel::SetSendSsrc(uint32_t send_ssrc) {
@@ -387,7 +416,7 @@
queued_received_data_.Clear();
if (data_channel_type_ != cricket::DCT_RTP) {
- Close();
+ CloseAbruptly();
}
return;
@@ -396,12 +425,6 @@
}
}
-void DataChannel::OnStreamClosedRemotely(int sid) {
- if (data_channel_type_ == cricket::DCT_SCTP && sid == config_.id) {
- Close();
- }
-}
-
void DataChannel::OnChannelReady(bool writable) {
writable_ = writable;
if (!writable) {
@@ -413,14 +436,23 @@
UpdateState();
}
-void DataChannel::DoClose() {
- if (state_ == kClosed)
+void DataChannel::CloseAbruptly() {
+ if (state_ == kClosed) {
return;
+ }
- receive_ssrc_set_ = false;
- send_ssrc_set_ = false;
+ if (connected_to_provider_) {
+ DisconnectFromProvider();
+ }
+
+ // Closing abruptly means any queued data gets thrown away.
+ queued_send_data_.Clear();
+ queued_control_data_.Clear();
+
+ // Still go to "kClosing" before "kClosed", since observers may be expecting
+ // that.
SetState(kClosing);
- UpdateState();
+ SetState(kClosed);
}
void DataChannel::UpdateState() {
@@ -460,13 +492,28 @@
break;
}
case kClosing: {
+ // Wait for all queued data to be sent before beginning the closing
+ // procedure.
if (queued_send_data_.Empty() && queued_control_data_.Empty()) {
- if (connected_to_provider_) {
- DisconnectFromProvider();
- }
-
- if (!connected_to_provider_ && !send_ssrc_set_ && !receive_ssrc_set_) {
- SetState(kClosed);
+ if (data_channel_type_ == cricket::DCT_RTP) {
+ // For RTP data channels, we can go to "closed" after we finish
+ // sending data and the send/recv SSRCs are unset.
+ if (connected_to_provider_) {
+ DisconnectFromProvider();
+ }
+ if (!send_ssrc_set_ && !receive_ssrc_set_) {
+ SetState(kClosed);
+ }
+ } else {
+ // For SCTP data channels, we need to wait for the closing procedure
+ // to complete; after calling RemoveSctpDataStream,
+ // OnClosingProcedureComplete will end up called asynchronously
+ // afterwards.
+ if (connected_to_provider_ && !started_closing_procedure_ &&
+ config_.id >= 0) {
+ started_closing_procedure_ = true;
+ provider_->RemoveSctpDataStream(config_.id);
+ }
}
}
break;
@@ -498,10 +545,6 @@
provider_->DisconnectDataChannel(this);
connected_to_provider_ = false;
-
- if (data_channel_type_ == cricket::DCT_SCTP && config_.id >= 0) {
- provider_->RemoveSctpDataStream(config_.id);
- }
}
void DataChannel::DeliverQueuedReceivedData() {
@@ -586,7 +629,7 @@
RTC_LOG(LS_ERROR) << "Closing the DataChannel due to a failure to send data, "
"send_result = "
<< send_result;
- Close();
+ CloseAbruptly();
return false;
}
@@ -653,7 +696,7 @@
RTC_LOG(LS_ERROR) << "Closing the DataChannel due to a failure to send"
" the CONTROL message, send_result = "
<< send_result;
- Close();
+ CloseAbruptly();
}
return retval;
}