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}

pc/jsep_transport_controller.cc

@@ -443,19 +443,19 @@ void JsepTransportController::SetMediaTransportSettings(
       use_datagram_transport_for_data_channels_receive_only;
 }
 
-void JsepTransportController::RollbackTransportForMids(
+void JsepTransportController::RollbackTransports() {
-    const std::vector<std::string>& mids) {
   if (!network_thread_->IsCurrent()) {
-    network_thread_->Invoke<void>(RTC_FROM_HERE,
+    network_thread_->Invoke<void>(RTC_FROM_HERE, [=] { RollbackTransports(); });
-                                  [=] { RollbackTransportForMids(mids); });
     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 @@ RTCError JsepTransportController::ApplyDescription_n(
     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 @@ RTCError JsepTransportController::ApplyDescription_n(
                                content_info.name + ": " + error.message());
     }
   }
+  if (type == SdpType::kAnswer) {
+    pending_mids_.clear();
+  }
   return RTCError::OK();
 }
 
@@ -874,7 +877,8 @@ bool JsepTransportController::SetTransportForMid(
   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(),

pc/jsep_transport_controller.h

@@ -224,9 +224,9 @@ class JsepTransportController : public sigslot::has_slots<> {
       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 @@ class JsepTransportController : public sigslot::has_slots<> {
   // 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

pc/peer_connection.cc

@@ -7536,7 +7536,6 @@ RTCError PeerConnection::Rollback(SdpType sdp_type) {
   }
   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 @@ RTCError PeerConnection::Rollback(SdpType sdp_type) {
     }
 
     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 @@ RTCError PeerConnection::Rollback(SdpType sdp_type) {
     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();

pc/peer_connection_jsep_unittest.cc

@@ -2129,4 +2129,73 @@ TEST_F(PeerConnectionJsepTest, RollbackMultipleStreamChanges) {
             "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