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

This reverts commit 298313534d.

Changes from the original commit:
* Call OnTransportClosed() from TeardownDataChannelTransport_n()
  (same as before the original commit)
* Not call OnTransportClosed() from OnTransportChanged() when its
  called with nullptr (also preserving the behaviour from before
  the original commit).

Original change's description:
> Revert "Add param to DCC::SetupDataChannelTransport_n, simplify DCC* setup code."
>
> This reverts commit 2ec6a6c578.
>
> 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}

Bug: webrtc:11547
Change-Id: I8ebbc3d3a12786dff2096350a77e03e98466ff00
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/301702
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39884}

pc/data_channel_controller.cc

@@ -159,13 +159,11 @@ void DataChannelController::OnTransportClosed(RTCError error) {
   }
 }
 
-void DataChannelController::SetupDataChannelTransport_n() {
+void DataChannelController::SetupDataChannelTransport_n(
+    DataChannelTransportInterface* transport) {
   RTC_DCHECK_RUN_ON(network_thread());
-
+  RTC_DCHECK(transport);
-  // There's a new data channel transport.  This needs to be signaled to the
+  set_data_channel_transport(transport);
-  // `sctp_data_channels_n_` so that they can reopen and reconnect.  This is
-  // necessary when bundling is applied.
-  NotifyDataChannelsOfTransportCreated();
 }
 
 void DataChannelController::PrepareForShutdown() {
@@ -175,14 +173,8 @@ void DataChannelController::PrepareForShutdown() {
 
 void DataChannelController::TeardownDataChannelTransport_n(RTCError error) {
   RTC_DCHECK_RUN_ON(network_thread());
-
   OnTransportClosed(error);
-
+  set_data_channel_transport(nullptr);
-  if (data_channel_transport_) {
-    data_channel_transport_->SetDataSink(nullptr);
-    set_data_channel_transport(nullptr);
-  }
-
   RTC_DCHECK(sctp_data_channels_n_.empty());
   weak_factory_.InvalidateWeakPtrs();
 }
@@ -194,16 +186,7 @@ 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();
-    }
   }
 }
 
@@ -416,7 +399,19 @@ 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);
+  }
 }
 
 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();
+  void SetupDataChannelTransport_n(DataChannelTransportInterface* transport);
   // Called from PeerConnection::TeardownDataChannelTransport_n
   void TeardownDataChannelTransport_n(RTCError error);
 
@@ -93,9 +93,6 @@ 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:
@@ -135,6 +132,8 @@ 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,8 +51,15 @@ class MockDataChannelTransport : public webrtc::DataChannelTransportInterface {
 // behavior by calling those methods from within its destructor.
 class DataChannelControllerForTest : public DataChannelController {
  public:
-  explicit DataChannelControllerForTest(PeerConnectionInternal* pc)
+  explicit DataChannelControllerForTest(
-      : DataChannelController(pc) {}
+      PeerConnectionInternal* pc,
+      DataChannelTransportInterface* transport = nullptr)
+      : DataChannelController(pc) {
+    if (transport) {
+      network_thread()->BlockingCall(
+          [&] { SetupDataChannelTransport_n(transport); });
+    }
+  }
 
   ~DataChannelControllerForTest() override {
     network_thread()->BlockingCall(
@@ -143,9 +150,7 @@ TEST_F(DataChannelControllerTest, MaxChannels) {
                                                          : rtc::SSL_CLIENT);
   });
 
-  DataChannelControllerForTest dcc(pc_.get());
+  DataChannelControllerForTest dcc(pc_.get(), &transport);
-  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.
@@ -171,9 +176,7 @@ TEST_F(DataChannelControllerTest, NoStreamIdReuseWhileClosing) {
   });
 
   NiceMock<MockDataChannelTransport> transport;  // Wider scope than `dcc`.
-  DataChannelControllerForTest dcc(pc_.get());
+  DataChannelControllerForTest dcc(pc_.get(), &transport);
-  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,12 +2113,14 @@ absl::optional<std::string> PeerConnection::GetDataMid() const {
   return sctp_mid_s_;
 }
 
-void PeerConnection::SetSctpDataMid(const std::string& mid) {
+void PeerConnection::SetSctpDataInfo(absl::string_view mid,
+                                     absl::string_view transport_name) {
   RTC_DCHECK_RUN_ON(signaling_thread());
-  sctp_mid_s_ = mid;
+  sctp_mid_s_ = std::string(mid);
+  SetSctpTransportName(std::string(transport_name));
 }
 
-void PeerConnection::ResetSctpDataMid() {
+void PeerConnection::ResetSctpDataInfo() {
   RTC_DCHECK_RUN_ON(signaling_thread());
   sctp_mid_s_.reset();
   SetSctpTransportName("");
@@ -2511,37 +2513,32 @@ absl::optional<AudioDeviceModule::Stats> PeerConnection::GetAudioDeviceStats() {
   return absl::nullopt;
 }
 
-bool PeerConnection::SetupDataChannelTransport_n(const std::string& mid) {
+absl::optional<std::string> PeerConnection::SetupDataChannelTransport_n(
+    absl::string_view mid) {
+  sctp_mid_n_ = std::string(mid);
   DataChannelTransportInterface* transport =
-      transport_controller_->GetDataChannelTransport(mid);
+      transport_controller_->GetDataChannelTransport(*sctp_mid_n_);
   if (!transport) {
     RTC_LOG(LS_ERROR)
         << "Data channel transport is not available for data channels, mid="
         << mid;
-    return false;
+    sctp_mid_n_ = absl::nullopt;
+    return absl::nullopt;
   }
-  RTC_LOG(LS_INFO) << "Setting up data channel transport for mid=" << mid;
 
-  data_channel_controller_.set_data_channel_transport(transport);
+  absl::optional<std::string> transport_name;
-  data_channel_controller_.SetupDataChannelTransport_n();
-  sctp_mid_n_ = mid;
   cricket::DtlsTransportInternal* dtls_transport =
-      transport_controller_->GetDtlsTransport(mid);
+      transport_controller_->GetDtlsTransport(*sctp_mid_n_);
   if (dtls_transport) {
-    signaling_thread()->PostTask(
+    transport_name = dtls_transport->transport_name();
-        SafeTask(signaling_thread_safety_.flag(),
+  } else {
-                 [this, name = dtls_transport->transport_name()] {
+    // Make sure we still set a valid string.
-                   RTC_DCHECK_RUN_ON(signaling_thread());
+    transport_name = std::string("");
-                   SetSctpTransportName(std::move(name));
-                 }));
   }
 
-  // Note: setting the data sink and checking initial state must be done last,
+  data_channel_controller_.SetupDataChannelTransport_n(transport);
-  // after setting up the data channel.  Setting the data sink may trigger
+
-  // callbacks to PeerConnection which require the transport to be completely
+  return transport_name;
-  // set up (eg. OnReadyToSend()).
-  transport->SetDataSink(&data_channel_controller_);
-  return true;
 }
 
 void PeerConnection::TeardownDataChannelTransport_n(RTCError error) {

pc/peer_connection.h

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

pc/peer_connection_internal.h

@@ -117,10 +117,16 @@ class PeerConnectionSdpMethods {
   // Returns true if SRTP (either using DTLS-SRTP or SDES) is required by
   // this session.
   virtual bool SrtpRequired() const = 0;
-  virtual bool SetupDataChannelTransport_n(const std::string& mid) = 0;
+  // Configures the data channel transport on the network thread.
+  // 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 SetSctpDataMid(const std::string& mid) = 0;
+  virtual void SetSctpDataInfo(absl::string_view mid,
-  virtual void ResetSctpDataMid() = 0;
+                               absl::string_view transport_name) = 0;
+  virtual void ResetSctpDataInfo() = 0;
 
   virtual const FieldTrialsView& trials() const = 0;
 

pc/sctp_data_channel.cc

@@ -725,15 +725,6 @@ 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,11 +182,6 @@ 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,18 +5093,15 @@ bool SdpOfferAnswerHandler::CreateDataChannel(const std::string& mid) {
 
   RTC_LOG(LS_INFO) << "Creating data channel, mid=" << mid;
 
-  if (!context_->network_thread()->BlockingCall([this, &mid] {
+  absl::optional<std::string> transport_name =
+      context_->network_thread()->BlockingCall([&] {
         RTC_DCHECK_RUN_ON(context_->network_thread());
         return pc_->SetupDataChannelTransport_n(mid);
-      })) {
+      });
+  if (!transport_name)
     return false;
-  }
+
-  // TODO(tommi): Is this necessary? SetupDataChannelTransport_n() above
+  pc_->SetSctpDataInfo(mid, *transport_name);
-  // 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;
 }
 
@@ -5115,7 +5112,7 @@ void SdpOfferAnswerHandler::DestroyDataChannelTransport(RTCError error) {
         RTC_DCHECK_RUN_ON(context_->network_thread());
         pc_->TeardownDataChannelTransport_n(error);
       });
-  pc_->ResetSctpDataMid();
+  pc_->ResetSctpDataInfo();
 }
 
 void SdpOfferAnswerHandler::DestroyAllChannels() {

pc/test/fake_peer_connection_base.h

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

pc/test/mock_peer_connection_internal.h

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