From d9ebe015405d8b239c6855e579d4ff2d8616adbb Mon Sep 17 00:00:00 2001
From: Eldar Rello <elrello@microsoft.com>
Date: Wed, 18 Mar 2020 20:41:45 +0200
Subject: [PATCH] Improve rollback for rtp data channel
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Bug: chromium:1057333
Change-Id: I4df21bc183a8df398033ebf29a8407bacf873fac
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/170621
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Eldar Rello <elrello@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#30824}
---
 pc/peer_connection.cc                 |  6 ++++++
 pc/peer_connection.h                  |  3 +++
 pc/peer_connection_integrationtest.cc | 29 +++++++++++++++++++++++++++
 pc/peer_connection_jsep_unittest.cc   | 12 +++++++++++
 4 files changed, 50 insertions(+)

diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index c6af185d47..fd2b81fd09 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -6017,6 +6017,7 @@ RTCError PeerConnection::UpdateSessionState(
     RTC_DCHECK(type == SdpType::kAnswer);
     ChangeSignalingState(PeerConnectionInterface::kStable);
     transceiver_stable_states_by_transceivers_.clear();
+    have_pending_rtp_data_channel_ = false;
   }
 
   // Update internal objects according to the session description's media
@@ -6688,6 +6689,7 @@ bool PeerConnection::CreateDataChannel(const std::string& mid) {
           this, &PeerConnection::OnSentPacket_w);
       data_channel_controller_.rtp_data_channel()->SetRtpTransport(
           rtp_transport);
+      have_pending_rtp_data_channel_ = true;
       return true;
   }
   return false;
@@ -7610,6 +7612,10 @@ RTCError PeerConnection::Rollback(SdpType sdp_type) {
     transceiver->internal()->set_mline_index(state.mline_index());
   }
   transport_controller_->RollbackTransports();
+  if (have_pending_rtp_data_channel_) {
+    DestroyDataChannelTransport();
+    have_pending_rtp_data_channel_ = false;
+  }
   transceiver_stable_states_by_transceivers_.clear();
   pending_local_description_.reset();
   pending_remote_description_.reset();
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index cd5c00116a..4b2384284d 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -1295,6 +1295,9 @@ class PeerConnection : public PeerConnectionInternal,
   std::map<rtc::scoped_refptr<RtpTransceiverProxyWithInternal<RtpTransceiver>>,
            TransceiverStableState>
       transceiver_stable_states_by_transceivers_;
+  // Used when rolling back RTP data channels.
+  bool have_pending_rtp_data_channel_ RTC_GUARDED_BY(signaling_thread()) =
+      false;
   // Holds remote stream ids for transceivers from stable state.
   std::map<rtc::scoped_refptr<RtpTransceiverProxyWithInternal<RtpTransceiver>>,
            std::vector<std::string>>
diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc
index 0a6e5d1faf..054091e0e8 100644
--- a/pc/peer_connection_integrationtest.cc
+++ b/pc/peer_connection_integrationtest.cc
@@ -592,6 +592,10 @@ class PeerConnectionWrapper : public webrtc::PeerConnectionObserver,
     pc()->CreateOffer(observer, offer_answer_options_);
     return WaitForDescriptionFromObserver(observer);
   }
+  bool Rollback() {
+    return SetRemoteDescription(
+        webrtc::CreateSessionDescription(SdpType::kRollback, ""));
+  }
 
  private:
   explicit PeerConnectionWrapper(const std::string& debug_name)
@@ -3246,6 +3250,31 @@ TEST_P(PeerConnectionIntegrationTest, EndToEndCallWithRtpDataChannel) {
                  kDefaultTimeout);
 }
 
+TEST_P(PeerConnectionIntegrationTest, RtpDataChannelWorksAfterRollback) {
+  PeerConnectionInterface::RTCConfiguration rtc_config;
+  rtc_config.enable_rtp_data_channel = true;
+  rtc_config.enable_dtls_srtp = false;
+  ASSERT_TRUE(CreatePeerConnectionWrappersWithConfig(rtc_config, rtc_config));
+  ConnectFakeSignaling();
+  auto data_channel = caller()->pc()->CreateDataChannel("label_1", nullptr);
+  ASSERT_TRUE(data_channel.get() != nullptr);
+  caller()->CreateAndSetAndSignalOffer();
+  ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout);
+
+  caller()->CreateDataChannel("label_2", nullptr);
+  rtc::scoped_refptr<MockSetSessionDescriptionObserver> observer(
+      new rtc::RefCountedObject<MockSetSessionDescriptionObserver>());
+  caller()->pc()->SetLocalDescription(observer,
+                                      caller()->CreateOfferAndWait().release());
+  EXPECT_TRUE_WAIT(observer->called(), kDefaultTimeout);
+  caller()->Rollback();
+
+  std::string data = "hello world";
+  SendRtpDataWithRetries(data_channel, data, 5);
+  EXPECT_EQ_WAIT(data, callee()->data_observer()->last_message(),
+                 kDefaultTimeout);
+}
+
 // Ensure that an RTP data channel is signaled as closed for the caller when
 // the callee rejects it in a subsequent offer.
 TEST_P(PeerConnectionIntegrationTest,
diff --git a/pc/peer_connection_jsep_unittest.cc b/pc/peer_connection_jsep_unittest.cc
index 2a3c4c60cd..0b2f375dde 100644
--- a/pc/peer_connection_jsep_unittest.cc
+++ b/pc/peer_connection_jsep_unittest.cc
@@ -2198,4 +2198,16 @@ TEST_F(PeerConnectionJsepTest,
   EXPECT_TRUE(callee->CreateOfferAndSetAsLocal());
 }
 
+TEST_F(PeerConnectionJsepTest, RollbackRtpDataChannel) {
+  RTCConfiguration config;
+  config.sdp_semantics = SdpSemantics::kUnifiedPlan;
+  config.enable_rtp_data_channel = true;
+  auto pc = CreatePeerConnection(config);
+  pc->CreateDataChannel("dummy");
+  auto offer = pc->CreateOffer();
+  EXPECT_TRUE(pc->CreateOfferAndSetAsLocal());
+  EXPECT_TRUE(pc->SetRemoteDescription(pc->CreateRollback()));
+  EXPECT_TRUE(pc->SetLocalDescription(std::move(offer)));
+}
+
 }  // namespace webrtc