Remove calls to data_channel_transport() from the wrong thread.

Applying thread guards and removing the accessor that was being
called from the wrong context.

Bug: webrtc:11547, webrtc:9987
Change-Id: I80953aab48e5d155fc9d101526a3fa1f2704c39f
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/300544
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#39832}

pc/data_channel_controller.cc

@@ -49,24 +49,27 @@ RTCError DataChannelController::SendData(
     StreamId sid,
     const SendDataParams& params,
     const rtc::CopyOnWriteBuffer& payload) {
-  if (data_channel_transport())
+  RTC_DCHECK_RUN_ON(network_thread());
-    return DataChannelSendData(sid, params, payload);
+  if (!data_channel_transport_) {
     RTC_LOG(LS_ERROR) << "SendData called before transport is ready";
     return RTCError(RTCErrorType::INVALID_STATE);
   }
+  return data_channel_transport_->SendData(sid.stream_id_int(), params,
+                                           payload);
+}
 
 void DataChannelController::AddSctpDataStream(StreamId sid) {
   RTC_DCHECK_RUN_ON(network_thread());
   RTC_DCHECK(sid.HasValue());
-  if (data_channel_transport()) {
+  if (data_channel_transport_) {
-    data_channel_transport()->OpenChannel(sid.stream_id_int());
+    data_channel_transport_->OpenChannel(sid.stream_id_int());
   }
 }
 
 void DataChannelController::RemoveSctpDataStream(StreamId sid) {
   RTC_DCHECK_RUN_ON(network_thread());
-  if (data_channel_transport()) {
+  if (data_channel_transport_) {
-    data_channel_transport()->CloseChannel(sid.stream_id_int());
+    data_channel_transport_->CloseChannel(sid.stream_id_int());
   }
 }
 
@@ -178,11 +181,11 @@ void DataChannelController::TeardownDataChannelTransport_n() {
 void DataChannelController::OnTransportChanged(
     DataChannelTransportInterface* new_data_channel_transport) {
   RTC_DCHECK_RUN_ON(network_thread());
-  if (data_channel_transport() &&
+  if (data_channel_transport_ &&
-      data_channel_transport() != new_data_channel_transport) {
+      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);
+    data_channel_transport_->SetDataSink(nullptr);
     set_data_channel_transport(new_data_channel_transport);
     if (new_data_channel_transport) {
       new_data_channel_transport->SetDataSink(this);
@@ -418,33 +421,15 @@ void DataChannelController::OnTransportChannelClosed(RTCError error) {
   }
 }
 
-DataChannelTransportInterface* DataChannelController::data_channel_transport()
-    const {
-  // TODO(bugs.webrtc.org/11547): Only allow this accessor to be called on the
-  // network thread.
-  // RTC_DCHECK_RUN_ON(network_thread());
-  return data_channel_transport_;
-}
-
 void DataChannelController::set_data_channel_transport(
     DataChannelTransportInterface* transport) {
   RTC_DCHECK_RUN_ON(network_thread());
   data_channel_transport_ = transport;
 }
 
-RTCError DataChannelController::DataChannelSendData(
-    StreamId sid,
-    const SendDataParams& params,
-    const rtc::CopyOnWriteBuffer& payload) {
-  RTC_DCHECK_RUN_ON(network_thread());
-  RTC_DCHECK(data_channel_transport());
-  return data_channel_transport()->SendData(sid.stream_id_int(), params,
-                                            payload);
-}
-
 void DataChannelController::NotifyDataChannelsOfTransportCreated() {
   RTC_DCHECK_RUN_ON(network_thread());
-  RTC_DCHECK(data_channel_transport());
+  RTC_DCHECK(data_channel_transport_);
 
   for (const auto& channel : sctp_data_channels_n_) {
     if (channel->sid_n().HasValue())

pc/data_channel_controller.h

@@ -94,7 +94,6 @@ class DataChannelController : public SctpDataChannelControllerInterface,
   bool HasUsedDataChannels() const;
 
   // Accessors
-  DataChannelTransportInterface* data_channel_transport() const;
   void set_data_channel_transport(DataChannelTransportInterface* transport);
 
   // Called when the transport for the data channels is closed or destroyed.
@@ -135,11 +134,6 @@ class DataChannelController : public SctpDataChannelControllerInterface,
                                 absl::optional<rtc::SSLRole> fallback_ssl_role)
       RTC_RUN_ON(network_thread());
 
-  // Called from SendData when data_channel_transport() is true.
-  RTCError DataChannelSendData(StreamId sid,
-                               const SendDataParams& params,
-                               const rtc::CopyOnWriteBuffer& payload);
-
   // Called when all data channels need to be notified of a transport channel
   // (calls OnTransportChannelCreated on the signaling thread).
   void NotifyDataChannelsOfTransportCreated();
@@ -147,9 +141,8 @@ class DataChannelController : public SctpDataChannelControllerInterface,
   // 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.
-  // TODO(bugs.webrtc.org/9987): Accessed on both signaling and network
+  DataChannelTransportInterface* data_channel_transport_
-  // thread.
+      RTC_GUARDED_BY(network_thread()) = nullptr;
-  DataChannelTransportInterface* data_channel_transport_ = nullptr;
   SctpSidAllocator sid_allocator_ RTC_GUARDED_BY(network_thread());
   std::vector<rtc::scoped_refptr<SctpDataChannel>> sctp_data_channels_n_
       RTC_GUARDED_BY(network_thread());

pc/sdp_offer_answer.cc

@@ -3890,15 +3890,10 @@ RTCError SdpOfferAnswerHandler::UpdateDataChannel(
     RTCError error(RTCErrorType::OPERATION_ERROR_WITH_DATA, sb.Release());
     error.set_error_detail(RTCErrorDetailType::DATA_CHANNEL_FAILURE);
     DestroyDataChannelTransport(error);
-  } else {
+  } else if (!CreateDataChannel(content.name)) {
-    if (!data_channel_controller()->data_channel_transport()) {
-      RTC_LOG(LS_INFO) << "Creating data channel, mid=" << content.mid();
-      if (!CreateDataChannel(content.name)) {
     return RTCError(RTCErrorType::INTERNAL_ERROR,
                     "Failed to create data channel.");
   }
-    }
-  }
   return RTCError::OK();
 }
 
@@ -5081,19 +5076,23 @@ RTCError SdpOfferAnswerHandler::CreateChannels(const SessionDescription& desc) {
   }
 
   const cricket::ContentInfo* data = cricket::GetFirstDataContent(&desc);
-  if (data && !data->rejected &&
+  if (data && !data->rejected && !CreateDataChannel(data->name)) {
-      !data_channel_controller()->data_channel_transport()) {
-    if (!CreateDataChannel(data->name)) {
     return RTCError(RTCErrorType::INTERNAL_ERROR,
                     "Failed to create data channel.");
   }
-  }
 
   return RTCError::OK();
 }
 
 bool SdpOfferAnswerHandler::CreateDataChannel(const std::string& mid) {
   RTC_DCHECK_RUN_ON(signaling_thread());
+  if (pc_->sctp_mid().has_value()) {
+    RTC_DCHECK_EQ(mid, *pc_->sctp_mid());
+    return true;  // data channel already created.
+  }
+
+  RTC_LOG(LS_INFO) << "Creating data channel, mid=" << mid;
+
   if (!context_->network_thread()->BlockingCall([this, &mid] {
         RTC_DCHECK_RUN_ON(context_->network_thread());
         return pc_->SetupDataChannelTransport_n(mid);