Attempt to recycle a stopped data m-line before creating a new one

which avoids an infinitely growing SDP if the remote end rejects the datachannel section. This will reactivate the m-line even if all datachannels are closed. BUG=chromium:1442604 Change-Id: If60f93b406271163df692d96102baab701923602 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/304241 Reviewed-by: Harald Alvestrand <[email protected]> Commit-Queue: Philipp Hancke <[email protected]> Reviewed-by: Henrik Boström <[email protected]> Cr-Commit-Position: refs/heads/main@{#40029}
basharalbashier · May 9, 2023 · 522380f · 522380f
1 parent 8095d02
commit 522380f
Show file tree

Hide file tree

Showing 6 changed files with 126 additions and 14 deletions.
diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc
@@ -28,7 +28,7 @@ DataChannelController::~DataChannelController() {
       << "Missing call to PrepareForShutdown?";
 }
 
-bool DataChannelController::HasDataChannelsForTest() const {
+bool DataChannelController::HasDataChannels() const {
   auto has_channels = [&] {
     RTC_DCHECK_RUN_ON(network_thread());
     return !sctp_data_channels_n_.empty();

diff --git a/pc/data_channel_controller.h b/pc/data_channel_controller.h
@@ -87,8 +87,9 @@ class DataChannelController : public SctpDataChannelControllerInterface,
                                      const InternalDataChannelInit& config);
   void AllocateSctpSids(rtc::SSLRole role);
 
-  // Used by tests to check if data channels are currently tracked.
-  bool HasDataChannelsForTest() const;
+  // Check if data channels are currently tracked. Used to decide whether a
+  // rejected m=application section should be reoffered.
+  bool HasDataChannels() const;
 
   // At some point in time, a data channel has existed.
   bool HasUsedDataChannels() const;

diff --git a/pc/data_channel_controller_unittest.cc b/pc/data_channel_controller_unittest.cc
@@ -105,17 +105,17 @@ TEST_F(DataChannelControllerTest, CreateDataChannelEarlyRelease) {
 
 TEST_F(DataChannelControllerTest, CreateDataChannelEarlyClose) {
   DataChannelControllerForTest dcc(pc_.get());
-  EXPECT_FALSE(dcc.HasDataChannelsForTest());
+  EXPECT_FALSE(dcc.HasDataChannels());
   EXPECT_FALSE(dcc.HasUsedDataChannels());
   auto ret = dcc.InternalCreateDataChannelWithProxy(
       "label", InternalDataChannelInit(DataChannelInit()));
   ASSERT_TRUE(ret.ok());
   auto channel = ret.MoveValue();
-  EXPECT_TRUE(dcc.HasDataChannelsForTest());
+  EXPECT_TRUE(dcc.HasDataChannels());
   EXPECT_TRUE(dcc.HasUsedDataChannels());
   channel->Close();
   run_loop_.Flush();
-  EXPECT_FALSE(dcc.HasDataChannelsForTest());
+  EXPECT_FALSE(dcc.HasDataChannels());
   EXPECT_TRUE(dcc.HasUsedDataChannels());
 }
 

diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
@@ -1420,9 +1420,10 @@ PeerConnection::CreateDataChannelOrError(const std::string& label,
 
   rtc::scoped_refptr<DataChannelInterface> channel = ret.MoveValue();
 
-  // Trigger the onRenegotiationNeeded event for
-  // the first SCTP DataChannel.
-  if (first_datachannel) {
+  // Check the onRenegotiationNeeded event (with plan-b backward compat)
+  if (configuration_.sdp_semantics == SdpSemantics::kUnifiedPlan ||
+      (configuration_.sdp_semantics == SdpSemantics::kPlanB_DEPRECATED &&
+       first_datachannel)) {
     sdp_handler_->UpdateNegotiationNeeded();
   }
   NoteUsageEvent(UsageEvent::DATA_ADDED);

diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc
@@ -3342,8 +3342,20 @@ bool SdpOfferAnswerHandler::CheckIfNegotiationIsNeeded() {
   // 4. If connection has created any RTCDataChannels, and no m= section in
   // description has been negotiated yet for data, return true.
   if (data_channel_controller()->HasUsedDataChannels()) {
-    if (!cricket::GetFirstDataContent(description->description()->contents()))
+    const cricket::ContentInfo* data_content =
+        cricket::GetFirstDataContent(description->description()->contents());
+    if (!data_content) {
       return true;
+    }
+    // The remote end might have rejected the data content.
+    const cricket::ContentInfo* remote_data_content =
+        current_remote_description()
+            ? current_remote_description()->description()->GetContentByName(
+                  data_content->name)
+            : nullptr;
+    if (remote_data_content && remote_data_content->rejected) {
+      return true;
+    }
   }
   if (!ConfiguredForMedia()) {
     return false;
@@ -4263,10 +4275,28 @@ void SdpOfferAnswerHandler::GetOptionsForUnifiedPlanOffer(
   }
   // Lastly, add a m-section if we have requested local data channels and an
   // m section does not already exist.
-  if (!pc_->GetDataMid() && data_channel_controller()->HasUsedDataChannels()) {
-    session_options->media_description_options.push_back(
-        GetMediaDescriptionOptionsForActiveData(
-            mid_generator_.GenerateString()));
+  if (!pc_->GetDataMid() && data_channel_controller()->HasUsedDataChannels() &&
+      data_channel_controller()->HasDataChannels()) {
+    // Attempt to recycle a stopped m-line.
+    // TODO(crbug.com/1442604): GetDataMid() should return the mid if one was
+    // ever created but rejected.
+    bool recycled = false;
+    for (size_t i = 0; i < session_options->media_description_options.size();
+         i++) {
+      auto media_description = session_options->media_description_options[i];
+      if (media_description.type == cricket::MEDIA_TYPE_DATA &&
+          media_description.stopped) {
+        session_options->media_description_options[i] =
+            GetMediaDescriptionOptionsForActiveData(media_description.mid);
+        recycled = true;
+        break;
+      }
+    }
+    if (!recycled) {
+      session_options->media_description_options.push_back(
+          GetMediaDescriptionOptionsForActiveData(
+              mid_generator_.GenerateString()));
+    }
   }
 }
 

diff --git a/pc/sdp_offer_answer_unittest.cc b/pc/sdp_offer_answer_unittest.cc
@@ -34,6 +34,7 @@
 #include "modules/audio_processing/include/audio_processing.h"
 #include "p2p/base/port_allocator.h"
 #include "pc/peer_connection_wrapper.h"
+#include "pc/session_description.h"
 #include "pc/test/fake_audio_capture_module.h"
 #include "pc/test/mock_peer_connection_observers.h"
 #include "rtc_base/rtc_certificate_generator.h"
@@ -467,4 +468,83 @@ TEST_F(SdpOfferAnswerTest, RollbackPreservesAddTrackMid) {
   EXPECT_EQ(saved_mid, first_transceiver->mid());
 }
 
+#ifdef WEBRTC_HAVE_SCTP
+
+TEST_F(SdpOfferAnswerTest, RejectedDataChannelsDoNotGetReoffered) {
+  auto pc = CreatePeerConnection();
+  EXPECT_TRUE(pc->pc()->CreateDataChannelOrError("dc", nullptr).ok());
+  EXPECT_TRUE(pc->CreateOfferAndSetAsLocal());
+  auto mid = pc->pc()->local_description()->description()->contents()[0].mid();
+
+  // An answer that rejects the datachannel content.
+  std::string sdp =
+      "v=0\r\n"
+      "o=- 4131505339648218884 3 IN IP4 **-----**\r\n"
+      "s=-\r\n"
+      "t=0 0\r\n"
+      "a=ice-ufrag:zGWFZ+fVXDeN6UoI/136\r\n"
+      "a=ice-pwd:9AUNgUqRNI5LSIrC1qFD2iTR\r\n"
+      "a=fingerprint:sha-256 "
+      "AD:52:52:E0:B1:37:34:21:0E:15:8E:B7:56:56:7B:B4:39:0E:6D:1C:F5:84:A7:EE:"
+      "B5:27:3E:30:B1:7D:69:42\r\n"
+      "a=setup:passive\r\n"
+      "m=application 0 UDP/DTLS/SCTP webrtc-datachannel\r\n"
+      "c=IN IP4 0.0.0.0\r\n"
+      "a=sctp-port:5000\r\n"
+      "a=max-message-size:262144\r\n"
+      "a=mid:" +
+      mid + "\r\n";
+  auto answer = CreateSessionDescription(SdpType::kAnswer, sdp);
+  ASSERT_TRUE(pc->SetRemoteDescription(std::move(answer)));
+  // The subsequent offer should not recycle the m-line since the existing data
+  // channel is closed.
+  auto offer = pc->CreateOffer();
+  const auto& offer_contents = offer->description()->contents();
+  ASSERT_EQ(offer_contents.size(), 1u);
+  EXPECT_EQ(offer_contents[0].mid(), mid);
+  EXPECT_EQ(offer_contents[0].rejected, true);
+}
+
+TEST_F(SdpOfferAnswerTest, RejectedDataChannelsDoGetReofferedWhenActive) {
+  auto pc = CreatePeerConnection();
+  EXPECT_TRUE(pc->pc()->CreateDataChannelOrError("dc", nullptr).ok());
+  EXPECT_TRUE(pc->CreateOfferAndSetAsLocal());
+  auto mid = pc->pc()->local_description()->description()->contents()[0].mid();
+
+  // An answer that rejects the datachannel content.
+  std::string sdp =
+      "v=0\r\n"
+      "o=- 4131505339648218884 3 IN IP4 **-----**\r\n"
+      "s=-\r\n"
+      "t=0 0\r\n"
+      "a=ice-ufrag:zGWFZ+fVXDeN6UoI/136\r\n"
+      "a=ice-pwd:9AUNgUqRNI5LSIrC1qFD2iTR\r\n"
+      "a=fingerprint:sha-256 "
+      "AD:52:52:E0:B1:37:34:21:0E:15:8E:B7:56:56:7B:B4:39:0E:6D:1C:F5:84:A7:EE:"
+      "B5:27:3E:30:B1:7D:69:42\r\n"
+      "a=setup:passive\r\n"
+      "m=application 0 UDP/DTLS/SCTP webrtc-datachannel\r\n"
+      "c=IN IP4 0.0.0.0\r\n"
+      "a=sctp-port:5000\r\n"
+      "a=max-message-size:262144\r\n"
+      "a=mid:" +
+      mid + "\r\n";
+  auto answer = CreateSessionDescription(SdpType::kAnswer, sdp);
+  ASSERT_TRUE(pc->SetRemoteDescription(std::move(answer)));
+
+  // The subsequent offer should recycle the m-line when there is a new data
+  // channel.
+  EXPECT_TRUE(pc->pc()->CreateDataChannelOrError("dc2", nullptr).ok());
+  EXPECT_TRUE(pc->pc()->ShouldFireNegotiationNeededEvent(
+      pc->observer()->latest_negotiation_needed_event()));
+
+  auto offer = pc->CreateOffer();
+  const auto& offer_contents = offer->description()->contents();
+  ASSERT_EQ(offer_contents.size(), 1u);
+  EXPECT_EQ(offer_contents[0].mid(), mid);
+  EXPECT_EQ(offer_contents[0].rejected, false);
+}
+
+#endif  // WEBRTC_HAVE_SCTP
+
 }  // namespace webrtc