Fix misunderstanding: OnTransportChanged is called on network thread
Earlier CLs assumed that the object pointed to by call_ had to be
accessed on the worker thread. While this is generally the case,
Call::MediaTransportChange is explicitly thread safe, so
PeerConnection::OnTransportChanged doesn't have to run on the worker
thread for that reason.
Which is fortunate, because it actually runs on the network thread.
The RTC_RUN_ON(worker_thread()) annotation on the method declaration
was ineffective because this method is being called via a base class
pointer; replacing it with a call to
RTC_DCHECK_RUN_ON(worker_thread()) in the function body immediately
triggered assertions in the unit tests.
Bug: webrtc:9987
Change-Id: I08cf558a74f4ca2b2eff8ef4810ebbd1287a9726
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/129442
Commit-Queue: Karl Wiberg <kwiberg@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#27287}
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index ec2f656..878bcb5 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -7063,6 +7063,7 @@
RtpTransportInternal* rtp_transport,
rtc::scoped_refptr<DtlsTransport> dtls_transport,
MediaTransportInterface* media_transport) {
+ RTC_DCHECK_RUN_ON(network_thread());
RTC_DCHECK_RUNS_SERIALIZED(&use_media_transport_race_checker_);
bool ret = true;
auto base_channel = GetChannel(mid);
@@ -7076,7 +7077,7 @@
if (use_media_transport_) {
// Only pass media transport to call object if media transport is used
// for media (and not data channel).
- call_->MediaTransportChange(media_transport);
+ call_ptr_->MediaTransportChange(media_transport);
}
return ret;
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index 9c2222a..9e9abe4 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -1030,8 +1030,7 @@
bool OnTransportChanged(const std::string& mid,
RtpTransportInternal* rtp_transport,
rtc::scoped_refptr<DtlsTransport> dtls_transport,
- MediaTransportInterface* media_transport) override
- RTC_RUN_ON(worker_thread());
+ MediaTransportInterface* media_transport) override;
// Returns the observer. Will crash on CHECK if the observer is removed.
PeerConnectionObserver* Observer() const RTC_RUN_ON(signaling_thread());
@@ -1151,11 +1150,13 @@
bool remote_peer_supports_msid_ RTC_GUARDED_BY(signaling_thread()) = false;
+ // The unique_ptr belongs to the worker thread, but the Call object manages
+ // its own thread safety.
std::unique_ptr<Call> call_ RTC_GUARDED_BY(worker_thread());
// Points to the same thing as `call_`. Since it's const, we may read the
- // pointer (but not touch the object) from any thread.
- Call* const call_ptr_ RTC_PT_GUARDED_BY(worker_thread());
+ // pointer from any thread.
+ Call* const call_ptr_;
std::unique_ptr<StatsCollector> stats_
RTC_GUARDED_BY(signaling_thread()); // A pointer is passed to senders_