Reland "Fix unsynchronized access to mid_to_transport_ in JsepTransportController"
This reverts commit 6b143c1c0686519bc9d73223c1350cee286c8d78.
Reason for revert:
Relanding with updated expectations for SctpTransport::Information
based on TransceiverStateSurfacer in Chromium.
Original change's description:
> Revert "Fix unsynchronized access to mid_to_transport_ in JsepTransportController"
>
> This reverts commit 6cd405850467683cf10d05028ea0f644a68a91a4.
>
> Reason for revert: Breaks WebRTC Chromium FYI Bots
>
> First failure:
> https://ci.chromium.org/p/chromium/builders/webrtc.fyi/WebRTC%20Chromium%20FYI%20Android%20Tests%20%28dbg%29%20%28L%20Nexus5%29/1925
>
> Failed tests:
> WebRtcDataBrowserTest.CallWithSctpDataAndMedia
> WebRtcDataBrowserTest.CallWithSctpDataOnly
>
> Original change's description:
> > Fix unsynchronized access to mid_to_transport_ in JsepTransportController
> >
> > * Added several thread checks to JTC to help with programmer errors.
> > * Avoid a few Invokes() to the network thread here and there such
> > as for fetching sctp transport name for getStats(). The transport
> > name is now cached when it changes on the network thread.
> > * JsepTransportController instances now get deleted on the network
> > thread rather than on the signaling thread + issuing an Invoke()
> > in the dtor.
> > * Moved some thread hops from JTC over to PC which is where the problem
> > exists and also (imho) makes it easier to see where hops happen in
> > the PC code.
> > * The sctp transport is now started asynchronously when we push down the
> > media description.
> > * PeerConnection proxy calls GetSctpTransport directly on the network
> > thread instead of to the signaling thread + blocking on the network
> > thread.
> > * The above changes simplified things for webrtc::SctpTransport which
> > allowed for removing locking from that class and delete some code.
> >
> > Bug: webrtc:9987, webrtc:12445
> > Change-Id: Ic89a9426e314e1b93c81751d4f732f05fa448fbc
> > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/205620
> > Commit-Queue: Tommi <tommi@webrtc.org>
> > Reviewed-by: Harald Alvestrand <hta@webrtc.org>
> > Cr-Commit-Position: refs/heads/master@{#33191}
>
> TBR=tommi@webrtc.org,hta@webrtc.org
>
> Change-Id: I7b2913d5133807589461105cf07eff3e9bb7157e
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: webrtc:9987
> Bug: webrtc:12445
> Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/206466
> Reviewed-by: Guido Urdaneta <guidou@webrtc.org>
> Commit-Queue: Guido Urdaneta <guidou@webrtc.org>
> Cr-Commit-Position: refs/heads/master@{#33204}
TBR=tommi@webrtc.org,hta@webrtc.org,guidou@webrtc.org
# Not skipping CQ checks because this is a reland.
Bug: webrtc:9987
Bug: webrtc:12445
Change-Id: Icb205cbac493ed3b881d71ea3af4fb9018701bf4
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/206560
Reviewed-by: Tommi <tommi@webrtc.org>
Reviewed-by: Guido Urdaneta <guidou@webrtc.org>
Commit-Queue: Tommi <tommi@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33219}
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 406c44d..087cffc 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -489,12 +489,17 @@
sdp_handler_->ResetSessionDescFactory();
}
- transport_controller_.reset();
- // port_allocator_ lives on the network thread and should be destroyed there.
+ // port_allocator_ and transport_controller_ live on the network thread and
+ // should be destroyed there.
network_thread()->Invoke<void>(RTC_FROM_HERE, [this] {
RTC_DCHECK_RUN_ON(network_thread());
+ transport_controller_.reset();
port_allocator_.reset();
+ if (network_thread_safety_) {
+ network_thread_safety_->SetNotAlive();
+ network_thread_safety_ = nullptr;
+ }
});
// call_ and event_log_ must be destroyed on the worker thread.
worker_thread()->Invoke<void>(RTC_FROM_HERE, [this] {
@@ -527,13 +532,15 @@
}
// The port allocator lives on the network thread and should be initialized
- // there.
+ // there. Also set up the task safety flag for canceling pending tasks on
+ // the network thread when closing.
// TODO(bugs.webrtc.org/12427): See if we can piggyback on this call and
// initialize all the |transport_controller_->Subscribe*| calls below on the
// network thread via this invoke.
const auto pa_result =
network_thread()->Invoke<InitializePortAllocatorResult>(
RTC_FROM_HERE, [this, &stun_servers, &turn_servers, &configuration] {
+ network_thread_safety_ = PendingTaskSafetyFlag::Create();
return InitializePortAllocator_n(stun_servers, turn_servers,
configuration);
});
@@ -832,6 +839,16 @@
return AddTransceiver(track, RtpTransceiverInit());
}
+RtpTransportInternal* PeerConnection::GetRtpTransport(const std::string& mid) {
+ RTC_DCHECK_RUN_ON(signaling_thread());
+ return network_thread()->Invoke<RtpTransportInternal*>(
+ RTC_FROM_HERE, [this, &mid] {
+ auto rtp_transport = transport_controller_->GetRtpTransport(mid);
+ RTC_DCHECK(rtp_transport);
+ return rtp_transport;
+ });
+}
+
RTCErrorOr<rtc::scoped_refptr<RtpTransceiverInterface>>
PeerConnection::AddTransceiver(
rtc::scoped_refptr<MediaStreamTrackInterface> track,
@@ -1588,11 +1605,11 @@
rtc::scoped_refptr<SctpTransportInterface> PeerConnection::GetSctpTransport()
const {
- RTC_DCHECK_RUN_ON(signaling_thread());
- if (!sctp_mid_s_) {
+ RTC_DCHECK_RUN_ON(network_thread());
+ if (!sctp_mid_n_)
return nullptr;
- }
- return transport_controller_->GetSctpTransport(*sctp_mid_s_);
+
+ return transport_controller_->GetSctpTransport(*sctp_mid_n_);
}
const SessionDescriptionInterface* PeerConnection::local_description() const {
@@ -1673,11 +1690,16 @@
// WebRTC session description factory, the session description factory would
// call the transport controller.
sdp_handler_->ResetSessionDescFactory();
- transport_controller_.reset();
rtp_manager_->Close();
- network_thread()->Invoke<void>(
- RTC_FROM_HERE, [this] { port_allocator_->DiscardCandidatePool(); });
+ network_thread()->Invoke<void>(RTC_FROM_HERE, [this] {
+ transport_controller_.reset();
+ port_allocator_->DiscardCandidatePool();
+ if (network_thread_safety_) {
+ network_thread_safety_->SetNotAlive();
+ network_thread_safety_ = nullptr;
+ }
+ });
worker_thread()->Invoke<void>(RTC_FROM_HERE, [this] {
RTC_DCHECK_RUN_ON(worker_thread());
@@ -1831,6 +1853,17 @@
}
}
+void PeerConnection::SetSctpDataMid(const std::string& mid) {
+ RTC_DCHECK_RUN_ON(signaling_thread());
+ sctp_mid_s_ = mid;
+}
+
+void PeerConnection::ResetSctpDataMid() {
+ RTC_DCHECK_RUN_ON(signaling_thread());
+ sctp_mid_s_.reset();
+ sctp_transport_name_s_.clear();
+}
+
void PeerConnection::OnSctpDataChannelClosed(DataChannelInterface* channel) {
// Since data_channel_controller doesn't do signals, this
// signal is relayed here.
@@ -2044,13 +2077,8 @@
absl::optional<std::string> PeerConnection::sctp_transport_name() const {
RTC_DCHECK_RUN_ON(signaling_thread());
- if (sctp_mid_s_ && transport_controller_) {
- auto dtls_transport = transport_controller_->GetDtlsTransport(*sctp_mid_s_);
- if (dtls_transport) {
- return dtls_transport->transport_name();
- }
- return absl::optional<std::string>();
- }
+ if (sctp_mid_s_ && transport_controller_)
+ return sctp_transport_name_s_;
return absl::optional<std::string>();
}
@@ -2289,6 +2317,15 @@
data_channel_controller_.set_data_channel_transport(transport);
data_channel_controller_.SetupDataChannelTransport_n();
sctp_mid_n_ = mid;
+ auto dtls_transport = transport_controller_->GetDtlsTransport(mid);
+ if (dtls_transport) {
+ signaling_thread()->PostTask(
+ ToQueuedTask(signaling_thread_safety_.flag(),
+ [this, name = dtls_transport->transport_name()] {
+ RTC_DCHECK_RUN_ON(signaling_thread());
+ sctp_transport_name_s_ = std::move(name);
+ }));
+ }
// 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
@@ -2633,9 +2670,19 @@
if (base_channel) {
ret = base_channel->SetRtpTransport(rtp_transport);
}
+
if (mid == sctp_mid_n_) {
data_channel_controller_.OnTransportChanged(data_channel_transport);
+ if (dtls_transport) {
+ signaling_thread()->PostTask(ToQueuedTask(
+ signaling_thread_safety_.flag(),
+ [this, name = dtls_transport->internal()->transport_name()] {
+ RTC_DCHECK_RUN_ON(signaling_thread());
+ sctp_transport_name_s_ = std::move(name);
+ }));
+ }
}
+
return ret;
}
@@ -2645,6 +2692,23 @@
return observer_;
}
+void PeerConnection::StartSctpTransport(int local_port,
+ int remote_port,
+ int max_message_size) {
+ RTC_DCHECK_RUN_ON(signaling_thread());
+ if (!sctp_mid_s_)
+ return;
+
+ network_thread()->PostTask(ToQueuedTask(
+ network_thread_safety_,
+ [this, mid = *sctp_mid_s_, local_port, remote_port, max_message_size] {
+ rtc::scoped_refptr<SctpTransport> sctp_transport =
+ transport_controller()->GetSctpTransport(mid);
+ if (sctp_transport)
+ sctp_transport->Start(local_port, remote_port, max_message_size);
+ }));
+}
+
CryptoOptions PeerConnection::GetCryptoOptions() {
RTC_DCHECK_RUN_ON(signaling_thread());
// TODO(bugs.webrtc.org/9891) - Remove PeerConnectionFactory::CryptoOptions
