Revert "Add param to DCC::SetupDataChannelTransport_n, simplify DCC* setup code."

This reverts commit 2ec6a6c57830e06f601607c1b9473ad821b57e07.

Reason for revert: It breaks WPT tests (e.g. https://ci.chromium.org/ui/p/chromium/builders/try/linux-rel/1361972/overview) blocking the roll into Chromium.

Original change's description:
> Add param to DCC::SetupDataChannelTransport_n, simplify DCC* setup code.
>
> * DCC = DataChannelController.
>
> * Consolidate steps to set the mid and transport name. They're now
>   set at the same time and without a separate PostTask.
> * Transport sink is now consistently set in DCC
> * Order of notifications for setting up the transport is now the same
>   regardless of the first time the transport is being set or if it's
>   being replaced.
> * Made set_data_channel_transport() private.
>
> Bug: webrtc:11547
> Change-Id: I39e89c6e269e6f06d55981d7944678bf23c8817a
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300562
> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
> Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
> Cr-Commit-Position: refs/heads/main@{#39859}

Bug: webrtc:11547
Change-Id: I0d8d7453b71be80fbf1b7eba7d161336e29de091
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/301360
Auto-Submit: Mirko Bonadei <mbonadei@webrtc.org>
Owners-Override: Mirko Bonadei <mbonadei@webrtc.org>
Commit-Queue: Mirko Bonadei <mbonadei@webrtc.org>
Bot-Commit: rubber-stamper@appspot.gserviceaccount.com <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/heads/main@{#39864}

pc/data_channel_controller.cc

@@ -159,11 +159,13 @@ void DataChannelController::OnTransportClosed(RTCError error) {
   }
 }
 
-void DataChannelController::SetupDataChannelTransport_n(
+void DataChannelController::SetupDataChannelTransport_n() {
-    DataChannelTransportInterface* transport) {
   RTC_DCHECK_RUN_ON(network_thread());
-  RTC_DCHECK(transport);
+
-  set_data_channel_transport(transport);
+  // There's a new data channel transport.  This needs to be signaled to the
+  // `sctp_data_channels_n_` so that they can reopen and reconnect.  This is
+  // necessary when bundling is applied.
+  NotifyDataChannelsOfTransportCreated();
 }
 
 void DataChannelController::PrepareForShutdown() {
@@ -173,7 +175,14 @@ void DataChannelController::PrepareForShutdown() {
 
 void DataChannelController::TeardownDataChannelTransport_n(RTCError error) {
   RTC_DCHECK_RUN_ON(network_thread());
+
+  OnTransportClosed(error);
+
+  if (data_channel_transport_) {
+    data_channel_transport_->SetDataSink(nullptr);
     set_data_channel_transport(nullptr);
+  }
+
   RTC_DCHECK(sctp_data_channels_n_.empty());
   weak_factory_.InvalidateWeakPtrs();
 }
@@ -185,7 +194,16 @@ void DataChannelController::OnTransportChanged(
       data_channel_transport_ != new_data_channel_transport) {
     // Changed which data channel transport is used for `sctp_mid_` (eg. now
     // it's bundled).
+    data_channel_transport_->SetDataSink(nullptr);
     set_data_channel_transport(new_data_channel_transport);
+    if (new_data_channel_transport) {
+      new_data_channel_transport->SetDataSink(this);
+
+      // There's a new data channel transport.  This needs to be signaled to the
+      // `sctp_data_channels_n_` so that they can reopen and reconnect.  This is
+      // necessary when bundling is applied.
+      NotifyDataChannelsOfTransportCreated();
+    }
   }
 }
 
@@ -398,21 +416,7 @@ void DataChannelController::OnSctpDataChannelClosed(SctpDataChannel* channel) {
 void DataChannelController::set_data_channel_transport(
     DataChannelTransportInterface* transport) {
   RTC_DCHECK_RUN_ON(network_thread());
-
-  if (data_channel_transport_)
-    data_channel_transport_->SetDataSink(nullptr);
-
   data_channel_transport_ = transport;
-
-  if (data_channel_transport_) {
-    // There's a new data channel transport.  This needs to be signaled to the
-    // `sctp_data_channels_n_` so that they can reopen and reconnect.  This is
-    // necessary when bundling is applied.
-    NotifyDataChannelsOfTransportCreated();
-    data_channel_transport_->SetDataSink(this);
-  } else {
-    OnTransportClosed(RTCError::OK());
-  }
 }
 
 void DataChannelController::NotifyDataChannelsOfTransportCreated() {

pc/data_channel_controller.h

@@ -68,7 +68,7 @@ class DataChannelController : public SctpDataChannelControllerInterface,
   void PrepareForShutdown();
 
   // Called from PeerConnection::SetupDataChannelTransport_n
-  void SetupDataChannelTransport_n(DataChannelTransportInterface* transport);
+  void SetupDataChannelTransport_n();
   // Called from PeerConnection::TeardownDataChannelTransport_n
   void TeardownDataChannelTransport_n(RTCError error);
 
@@ -93,6 +93,9 @@ class DataChannelController : public SctpDataChannelControllerInterface,
   // At some point in time, a data channel has existed.
   bool HasUsedDataChannels() const;
 
+  // Accessors
+  void set_data_channel_transport(DataChannelTransportInterface* transport);
+
   void OnSctpDataChannelClosed(SctpDataChannel* channel);
 
  protected:
@@ -132,8 +135,6 @@ class DataChannelController : public SctpDataChannelControllerInterface,
   // (calls OnTransportChannelCreated on the signaling thread).
   void NotifyDataChannelsOfTransportCreated();
 
-  void set_data_channel_transport(DataChannelTransportInterface* transport);
-
   // Plugin transport used for data channels.  Pointer may be accessed and
   // checked from any thread, but the object may only be touched on the
   // network thread.

pc/data_channel_controller_unittest.cc

@@ -51,15 +51,8 @@ class MockDataChannelTransport : public webrtc::DataChannelTransportInterface {
 // behavior by calling those methods from within its destructor.
 class DataChannelControllerForTest : public DataChannelController {
  public:
-  explicit DataChannelControllerForTest(
+  explicit DataChannelControllerForTest(PeerConnectionInternal* pc)
-      PeerConnectionInternal* pc,
+      : DataChannelController(pc) {}
-      DataChannelTransportInterface* transport = nullptr)
-      : DataChannelController(pc) {
-    if (transport) {
-      network_thread()->BlockingCall(
-          [&] { SetupDataChannelTransport_n(transport); });
-    }
-  }
 
   ~DataChannelControllerForTest() override {
     network_thread()->BlockingCall(
@@ -150,7 +143,9 @@ TEST_F(DataChannelControllerTest, MaxChannels) {
                                                          : rtc::SSL_CLIENT);
   });
 
-  DataChannelControllerForTest dcc(pc_.get(), &transport);
+  DataChannelControllerForTest dcc(pc_.get());
+  pc_->network_thread()->BlockingCall(
+      [&] { dcc.set_data_channel_transport(&transport); });
 
   // Allocate the maximum number of channels + 1. Inside the loop, the creation
   // process will allocate a stream id for each channel.
@@ -176,7 +171,9 @@ TEST_F(DataChannelControllerTest, NoStreamIdReuseWhileClosing) {
   });
 
   NiceMock<MockDataChannelTransport> transport;  // Wider scope than `dcc`.
-  DataChannelControllerForTest dcc(pc_.get(), &transport);
+  DataChannelControllerForTest dcc(pc_.get());
+  pc_->network_thread()->BlockingCall(
+      [&] { dcc.set_data_channel_transport(&transport); });
 
   // Create the first channel and check that we got the expected, first sid.
   auto channel1 = dcc.InternalCreateDataChannelWithProxy(

pc/peer_connection.cc

@@ -2113,14 +2113,12 @@ absl::optional<std::string> PeerConnection::GetDataMid() const {
   return sctp_mid_s_;
 }
 
-void PeerConnection::SetSctpDataInfo(absl::string_view mid,
+void PeerConnection::SetSctpDataMid(const std::string& mid) {
-                                     absl::string_view transport_name) {
   RTC_DCHECK_RUN_ON(signaling_thread());
-  sctp_mid_s_ = std::string(mid);
+  sctp_mid_s_ = mid;
-  SetSctpTransportName(std::string(transport_name));
 }
 
-void PeerConnection::ResetSctpDataInfo() {
+void PeerConnection::ResetSctpDataMid() {
   RTC_DCHECK_RUN_ON(signaling_thread());
   sctp_mid_s_.reset();
   SetSctpTransportName("");
@@ -2513,32 +2511,37 @@ absl::optional<AudioDeviceModule::Stats> PeerConnection::GetAudioDeviceStats() {
   return absl::nullopt;
 }
 
-absl::optional<std::string> PeerConnection::SetupDataChannelTransport_n(
+bool PeerConnection::SetupDataChannelTransport_n(const std::string& mid) {
-    absl::string_view mid) {
-  sctp_mid_n_ = std::string(mid);
   DataChannelTransportInterface* transport =
-      transport_controller_->GetDataChannelTransport(*sctp_mid_n_);
+      transport_controller_->GetDataChannelTransport(mid);
   if (!transport) {
     RTC_LOG(LS_ERROR)
         << "Data channel transport is not available for data channels, mid="
         << mid;
-    sctp_mid_n_ = absl::nullopt;
+    return false;
-    return absl::nullopt;
   }
+  RTC_LOG(LS_INFO) << "Setting up data channel transport for mid=" << mid;
 
-  absl::optional<std::string> transport_name;
+  data_channel_controller_.set_data_channel_transport(transport);
+  data_channel_controller_.SetupDataChannelTransport_n();
+  sctp_mid_n_ = mid;
   cricket::DtlsTransportInternal* dtls_transport =
-      transport_controller_->GetDtlsTransport(*sctp_mid_n_);
+      transport_controller_->GetDtlsTransport(mid);
   if (dtls_transport) {
-    transport_name = dtls_transport->transport_name();
+    signaling_thread()->PostTask(
-  } else {
+        SafeTask(signaling_thread_safety_.flag(),
-    // Make sure we still set a valid string.
+                 [this, name = dtls_transport->transport_name()] {
-    transport_name = std::string("");
+                   RTC_DCHECK_RUN_ON(signaling_thread());
+                   SetSctpTransportName(std::move(name));
+                 }));
   }
 
-  data_channel_controller_.SetupDataChannelTransport_n(transport);
+  // Note: setting the data sink and checking initial state must be done last,
-
+  // after setting up the data channel.  Setting the data sink may trigger
-  return transport_name;
+  // callbacks to PeerConnection which require the transport to be completely
+  // set up (eg. OnReadyToSend()).
+  transport->SetDataSink(&data_channel_controller_);
+  return true;
 }
 
 void PeerConnection::TeardownDataChannelTransport_n(RTCError error) {

pc/peer_connection.h

@@ -402,10 +402,9 @@ class PeerConnection : public PeerConnectionInternal,
   // channels are configured this will return nullopt.
   absl::optional<std::string> GetDataMid() const override;
 
-  void SetSctpDataInfo(absl::string_view mid,
+  void SetSctpDataMid(const std::string& mid) override;
-                       absl::string_view transport_name) override;
 
-  void ResetSctpDataInfo() override;
+  void ResetSctpDataMid() override;
 
   // Asynchronously calls SctpTransport::Start() on the network thread for
   // `sctp_mid()` if set. Called as part of setting the local description.
@@ -433,8 +432,8 @@ class PeerConnection : public PeerConnectionInternal,
   // this session.
   bool SrtpRequired() const override;
 
-  absl::optional<std::string> SetupDataChannelTransport_n(
+  bool SetupDataChannelTransport_n(const std::string& mid) override
-      absl::string_view mid) override RTC_RUN_ON(network_thread());
+      RTC_RUN_ON(network_thread());
   void TeardownDataChannelTransport_n(RTCError error) override
       RTC_RUN_ON(network_thread());
 

pc/peer_connection_internal.h

@@ -117,16 +117,10 @@ class PeerConnectionSdpMethods {
   // Returns true if SRTP (either using DTLS-SRTP or SDES) is required by
   // this session.
   virtual bool SrtpRequired() const = 0;
-  // Configures the data channel transport on the network thread.
+  virtual bool SetupDataChannelTransport_n(const std::string& mid) = 0;
-  // The return value will be unset if an error occurs. If the setup succeeded
-  // the return value will be set and contain the name of the transport
-  // (empty string if a name isn't available).
-  virtual absl::optional<std::string> SetupDataChannelTransport_n(
-      absl::string_view mid) = 0;
   virtual void TeardownDataChannelTransport_n(RTCError error) = 0;
-  virtual void SetSctpDataInfo(absl::string_view mid,
+  virtual void SetSctpDataMid(const std::string& mid) = 0;
-                               absl::string_view transport_name) = 0;
+  virtual void ResetSctpDataMid() = 0;
-  virtual void ResetSctpDataInfo() = 0;
 
   virtual const FieldTrialsView& trials() const = 0;
 

pc/sctp_data_channel.cc

@@ -725,6 +725,15 @@ void SctpDataChannel::OnDataReceived(DataMessageType type,
 
 void SctpDataChannel::OnTransportReady() {
   RTC_DCHECK_RUN_ON(network_thread_);
+
+  // TODO(bugs.webrtc.org/11547): The transport is configured inside
+  // `PeerConnection::SetupDataChannelTransport_n`, which results in
+  // `SctpDataChannel` getting the OnTransportChannelCreated callback, and then
+  // that's immediately followed by calling `transport->SetDataSink` which is
+  // what triggers the callback to `OnTransportReady()`.
+  // These steps are currently accomplished via two separate PostTask calls to
+  // the signaling thread, but could simply be done in single method call on
+  // the network thread.
   RTC_DCHECK(connected_to_transport());
   RTC_DCHECK(id_n_.HasValue());
 

pc/sctp_data_channel.h

@@ -182,6 +182,11 @@ class SctpDataChannel : public DataChannelInterface {
   // Specializations of CloseAbruptlyWithError
   void CloseAbruptlyWithDataChannelFailure(const std::string& message);
 
+  // Slots for controller to connect signals to.
+  //
+  // TODO(deadbeef): Make these private once we're hooking up signals ourselves,
+  // instead of relying on SctpDataChannelControllerInterface.
+
   // Called when the SctpTransport's ready to use. That can happen when we've
   // finished negotiation, or if the channel was created after negotiation has
   // already finished.

pc/sdp_offer_answer.cc

@@ -5093,15 +5093,18 @@ bool SdpOfferAnswerHandler::CreateDataChannel(const std::string& mid) {
 
   RTC_LOG(LS_INFO) << "Creating data channel, mid=" << mid;
 
-  absl::optional<std::string> transport_name =
+  if (!context_->network_thread()->BlockingCall([this, &mid] {
-      context_->network_thread()->BlockingCall([&] {
         RTC_DCHECK_RUN_ON(context_->network_thread());
         return pc_->SetupDataChannelTransport_n(mid);
-      });
+      })) {
-  if (!transport_name)
     return false;
-
+  }
-  pc_->SetSctpDataInfo(mid, *transport_name);
+  // TODO(tommi): Is this necessary? SetupDataChannelTransport_n() above
+  // will have queued up updating the transport name on the signaling thread
+  // and could update the mid at the same time. This here is synchronous
+  // though, but it changes the state of PeerConnection and makes it be
+  // out of sync (transport name not set while the mid is set).
+  pc_->SetSctpDataMid(mid);
   return true;
 }
 
@@ -5112,7 +5115,7 @@ void SdpOfferAnswerHandler::DestroyDataChannelTransport(RTCError error) {
         RTC_DCHECK_RUN_ON(context_->network_thread());
         pc_->TeardownDataChannelTransport_n(error);
       });
-  pc_->ResetSctpDataInfo();
+  pc_->ResetSctpDataMid();
 }
 
 void SdpOfferAnswerHandler::DestroyAllChannels() {

pc/test/fake_peer_connection_base.h

@@ -358,14 +358,12 @@ class FakePeerConnectionBase : public PeerConnectionInternal {
 
   Call* call_ptr() override { return nullptr; }
   bool SrtpRequired() const override { return false; }
-  absl::optional<std::string> SetupDataChannelTransport_n(
+  bool SetupDataChannelTransport_n(const std::string& mid) override {
-      absl::string_view mid) override {
+    return false;
-    return absl::nullopt;
   }
   void TeardownDataChannelTransport_n(RTCError error) override {}
-  void SetSctpDataInfo(absl::string_view mid,
+  void SetSctpDataMid(const std::string& mid) override {}
-                       absl::string_view transport_name) override {}
+  void ResetSctpDataMid() override {}
-  void ResetSctpDataInfo() override {}
 
   const FieldTrialsView& trials() const override { return field_trials_; }
 

pc/test/mock_peer_connection_internal.h

@@ -263,16 +263,13 @@ class MockPeerConnectionInternal : public PeerConnectionInternal {
               (override));
   MOCK_METHOD(Call*, call_ptr, (), (override));
   MOCK_METHOD(bool, SrtpRequired, (), (const, override));
-  MOCK_METHOD(absl::optional<std::string>,
+  MOCK_METHOD(bool,
               SetupDataChannelTransport_n,
-              (absl::string_view mid),
+              (const std::string&),
               (override));
   MOCK_METHOD(void, TeardownDataChannelTransport_n, (RTCError), (override));
-  MOCK_METHOD(void,
+  MOCK_METHOD(void, SetSctpDataMid, (const std::string&), (override));
-              SetSctpDataInfo,
+  MOCK_METHOD(void, ResetSctpDataMid, (), (override));
-              (absl::string_view, absl::string_view),
-              (override));
-  MOCK_METHOD(void, ResetSctpDataInfo, (), (override));
   MOCK_METHOD(const FieldTrialsView&, trials, (), (const, override));
 
   // PeerConnectionInternal