Rollback transport created by data channel
No-Try: True
Bug: chromium:1032987
Change-Id: I2c0dbd6a19e71a391dc2e0d30676d4efa26a9525
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/168306
Commit-Queue: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30561}
diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc
index 8d4eee0..0687a06 100644
--- a/pc/jsep_transport_controller.cc
+++ b/pc/jsep_transport_controller.cc
@@ -443,19 +443,19 @@
use_datagram_transport_for_data_channels_receive_only;
}
-void JsepTransportController::RollbackTransportForMids(
- const std::vector<std::string>& mids) {
+void JsepTransportController::RollbackTransports() {
if (!network_thread_->IsCurrent()) {
- network_thread_->Invoke<void>(RTC_FROM_HERE,
- [=] { RollbackTransportForMids(mids); });
+ network_thread_->Invoke<void>(RTC_FROM_HERE, [=] { RollbackTransports(); });
return;
}
- for (auto&& mid : mids) {
+ RTC_DCHECK_RUN_ON(network_thread_);
+ for (auto&& mid : pending_mids_) {
RemoveTransportForMid(mid);
}
- for (auto&& mid : mids) {
+ for (auto&& mid : pending_mids_) {
MaybeDestroyJsepTransport(mid);
}
+ pending_mids_.clear();
}
rtc::scoped_refptr<webrtc::IceTransportInterface>
@@ -605,7 +605,7 @@
bool local,
SdpType type,
const cricket::SessionDescription* description) {
- RTC_DCHECK(network_thread_->IsCurrent());
+ RTC_DCHECK_RUN_ON(network_thread_);
RTC_DCHECK(description);
if (local) {
@@ -718,6 +718,9 @@
content_info.name + ": " + error.message());
}
}
+ if (type == SdpType::kAnswer) {
+ pending_mids_.clear();
+ }
return RTCError::OK();
}
@@ -874,7 +877,8 @@
if (mid_to_transport_[mid] == jsep_transport) {
return true;
}
-
+ RTC_DCHECK_RUN_ON(network_thread_);
+ pending_mids_.push_back(mid);
mid_to_transport_[mid] = jsep_transport;
return config_.transport_observer->OnTransportChanged(
mid, jsep_transport->rtp_transport(), jsep_transport->RtpDtlsTransport(),
diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h
index 9c3f691..c966e74 100644
--- a/pc/jsep_transport_controller.h
+++ b/pc/jsep_transport_controller.h
@@ -224,9 +224,9 @@
bool use_datagram_transport_for_data_channels,
bool use_datagram_transport_for_data_channels_receive_only);
- // TODO(elrello): For now the rollback only removes mid to transport mappings
+ // For now the rollback only removes mid to transport mappings
// and deletes unused transports, but doesn't consider anything more complex.
- void RollbackTransportForMids(const std::vector<std::string>& mids);
+ void RollbackTransports();
// Gets the transport parameters for the transport identified by |mid|.
// If |mid| is bundled, returns the parameters for the bundled transport.
@@ -430,7 +430,8 @@
// This keeps track of the mapping between media section
// (BaseChannel/SctpTransport) and the JsepTransport underneath.
std::map<std::string, cricket::JsepTransport*> mid_to_transport_;
-
+ // Keep track of mids that have been mapped to transports. Used for rollback.
+ std::vector<std::string> pending_mids_ RTC_GUARDED_BY(network_thread_);
// Aggregate states for Transports.
// standardized_ice_connection_state_ is intended to replace
// ice_connection_state, see bugs.webrtc.org/9308
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index b9e9d29..6678552a 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -7536,7 +7536,6 @@
}
RTC_DCHECK_RUN_ON(signaling_thread());
RTC_DCHECK(IsUnifiedPlan());
- std::vector<std::string> mids;
std::vector<rtc::scoped_refptr<MediaStreamInterface>> all_added_streams;
std::vector<rtc::scoped_refptr<MediaStreamInterface>> all_removed_streams;
std::vector<rtc::scoped_refptr<RtpReceiverInterface>> removed_receivers;
@@ -7563,8 +7562,6 @@
}
RTC_DCHECK(transceiver->internal()->mid().has_value());
- std::string mid = transceiver->internal()->mid().value();
- mids.push_back(mid);
DestroyTransceiverChannel(transceiver);
if (signaling_state() == PeerConnectionInterface::kHaveRemoteOffer &&
@@ -7589,7 +7586,7 @@
transceiver->internal()->set_mid(state.mid());
transceiver->internal()->set_mline_index(state.mline_index());
}
- transport_controller_->RollbackTransportForMids(mids);
+ transport_controller_->RollbackTransports();
transceiver_stable_states_by_transceivers_.clear();
pending_local_description_.reset();
pending_remote_description_.reset();
diff --git a/pc/peer_connection_jsep_unittest.cc b/pc/peer_connection_jsep_unittest.cc
index 3186e8f..2a3c4c60 100644
--- a/pc/peer_connection_jsep_unittest.cc
+++ b/pc/peer_connection_jsep_unittest.cc
@@ -2129,4 +2129,73 @@
"id_1");
}
+TEST_F(PeerConnectionJsepTest, DataChannelImplicitRollback) {
+ RTCConfiguration config;
+ config.sdp_semantics = SdpSemantics::kUnifiedPlan;
+ config.enable_implicit_rollback = true;
+ auto caller = CreatePeerConnection(config);
+ caller->AddTransceiver(cricket::MEDIA_TYPE_VIDEO);
+ auto callee = CreatePeerConnection(config);
+ callee->CreateDataChannel("dummy");
+ EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
+ EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
+ EXPECT_TRUE(callee->CreateAnswerAndSetAsLocal());
+ EXPECT_TRUE(callee->observer()->negotiation_needed());
+ EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
+}
+
+TEST_F(PeerConnectionJsepTest, RollbackRemoteDataChannelThenAddTransceiver) {
+ auto caller = CreatePeerConnection();
+ auto callee = CreatePeerConnection();
+ caller->CreateDataChannel("dummy");
+ EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
+ EXPECT_TRUE(callee->SetRemoteDescription(callee->CreateRollback()));
+ callee->AddTransceiver(cricket::MEDIA_TYPE_VIDEO);
+ EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
+}
+
+TEST_F(PeerConnectionJsepTest,
+ RollbackRemoteDataChannelThenAddTransceiverAndDataChannel) {
+ auto caller = CreatePeerConnection();
+ auto callee = CreatePeerConnection();
+ caller->CreateDataChannel("dummy");
+ EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
+ EXPECT_TRUE(callee->SetRemoteDescription(callee->CreateRollback()));
+ callee->AddTransceiver(cricket::MEDIA_TYPE_VIDEO);
+ callee->CreateDataChannel("dummy");
+ EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
+}
+
+TEST_F(PeerConnectionJsepTest, RollbackRemoteDataChannelThenAddDataChannel) {
+ auto caller = CreatePeerConnection();
+ auto callee = CreatePeerConnection();
+ caller->CreateDataChannel("dummy");
+ EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
+ EXPECT_TRUE(callee->SetRemoteDescription(callee->CreateRollback()));
+ callee->CreateDataChannel("dummy");
+ EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
+}
+
+TEST_F(PeerConnectionJsepTest, RollbackRemoteTransceiverThenAddDataChannel) {
+ auto caller = CreatePeerConnection();
+ auto callee = CreatePeerConnection();
+ caller->AddTransceiver(cricket::MEDIA_TYPE_VIDEO);
+ EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
+ EXPECT_TRUE(callee->SetRemoteDescription(callee->CreateRollback()));
+ callee->CreateDataChannel("dummy");
+ EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
+}
+
+TEST_F(PeerConnectionJsepTest,
+ RollbackRemoteTransceiverThenAddDataChannelAndTransceiver) {
+ auto caller = CreatePeerConnection();
+ auto callee = CreatePeerConnection();
+ caller->AddTransceiver(cricket::MEDIA_TYPE_VIDEO);
+ EXPECT_TRUE(callee->SetRemoteDescription(caller->CreateOffer()));
+ EXPECT_TRUE(callee->SetRemoteDescription(callee->CreateRollback()));
+ callee->CreateDataChannel("dummy");
+ callee->AddTransceiver(cricket::MEDIA_TYPE_VIDEO);
+ EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
+}
+
} // namespace webrtc
