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}
diff --git a/pc/data_channel_controller.cc b/pc/data_channel_controller.cc
index b1ebfc0..3545546 100644
--- a/pc/data_channel_controller.cc
+++ b/pc/data_channel_controller.cc
@@ -49,24 +49,27 @@
StreamId sid,
const SendDataParams& params,
const rtc::CopyOnWriteBuffer& payload) {
- if (data_channel_transport())
- return DataChannelSendData(sid, params, payload);
- RTC_LOG(LS_ERROR) << "SendData called before transport is ready";
- return RTCError(RTCErrorType::INVALID_STATE);
+ RTC_DCHECK_RUN_ON(network_thread());
+ 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()) {
- data_channel_transport()->OpenChannel(sid.stream_id_int());
+ if (data_channel_transport_) {
+ data_channel_transport_->OpenChannel(sid.stream_id_int());
}
}
void DataChannelController::RemoveSctpDataStream(StreamId sid) {
RTC_DCHECK_RUN_ON(network_thread());
- if (data_channel_transport()) {
- data_channel_transport()->CloseChannel(sid.stream_id_int());
+ if (data_channel_transport_) {
+ data_channel_transport_->CloseChannel(sid.stream_id_int());
}
}
@@ -178,11 +181,11 @@
void DataChannelController::OnTransportChanged(
DataChannelTransportInterface* new_data_channel_transport) {
RTC_DCHECK_RUN_ON(network_thread());
- if (data_channel_transport() &&
- data_channel_transport() != new_data_channel_transport) {
+ if (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 @@
}
}
-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())
diff --git a/pc/data_channel_controller.h b/pc/data_channel_controller.h
index 074b1fe..edd21c2 100644
--- a/pc/data_channel_controller.h
+++ b/pc/data_channel_controller.h
@@ -94,7 +94,6 @@
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 @@
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 @@
// 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
- // thread.
- DataChannelTransportInterface* data_channel_transport_ = nullptr;
+ DataChannelTransportInterface* data_channel_transport_
+ RTC_GUARDED_BY(network_thread()) = nullptr;
SctpSidAllocator sid_allocator_ RTC_GUARDED_BY(network_thread());
std::vector<rtc::scoped_refptr<SctpDataChannel>> sctp_data_channels_n_
RTC_GUARDED_BY(network_thread());
diff --git a/pc/sdp_offer_answer.cc b/pc/sdp_offer_answer.cc
index 90a6cd2..0363042 100644
--- a/pc/sdp_offer_answer.cc
+++ b/pc/sdp_offer_answer.cc
@@ -3890,14 +3890,9 @@
RTCError error(RTCErrorType::OPERATION_ERROR_WITH_DATA, sb.Release());
error.set_error_detail(RTCErrorDetailType::DATA_CHANNEL_FAILURE);
DestroyDataChannelTransport(error);
- } else {
- 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.");
- }
- }
+ } else if (!CreateDataChannel(content.name)) {
+ return RTCError(RTCErrorType::INTERNAL_ERROR,
+ "Failed to create data channel.");
}
return RTCError::OK();
}
@@ -5081,12 +5076,9 @@
}
const cricket::ContentInfo* data = cricket::GetFirstDataContent(&desc);
- if (data && !data->rejected &&
- !data_channel_controller()->data_channel_transport()) {
- if (!CreateDataChannel(data->name)) {
- return RTCError(RTCErrorType::INTERNAL_ERROR,
- "Failed to create data channel.");
- }
+ if (data && !data->rejected && !CreateDataChannel(data->name)) {
+ return RTCError(RTCErrorType::INTERNAL_ERROR,
+ "Failed to create data channel.");
}
return RTCError::OK();
@@ -5094,6 +5086,13 @@
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);
