From ac025898e1e76a688dea5ea9e3681dd8dc2ce856 Mon Sep 17 00:00:00 2001 From: Karl Wiberg Date: Tue, 26 Mar 2019 13:08:37 +0100 Subject: [PATCH] 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 Reviewed-by: Steve Anton Cr-Commit-Position: refs/heads/master@{#27287} --- pc/peer_connection.cc | 3 ++- pc/peer_connection.h | 9 +++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index ec2f656422..878bcb5f6f 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -7063,6 +7063,7 @@ bool PeerConnection::OnTransportChanged( RtpTransportInternal* rtp_transport, rtc::scoped_refptr 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 @@ bool PeerConnection::OnTransportChanged( 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 9c2222ac60..9e9abe40b0 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -1030,8 +1030,7 @@ class PeerConnection : public PeerConnectionInternal, bool OnTransportChanged(const std::string& mid, RtpTransportInternal* rtp_transport, rtc::scoped_refptr 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 @@ class PeerConnection : public PeerConnectionInternal, 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_ 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 stats_ RTC_GUARDED_BY(signaling_thread()); // A pointer is passed to senders_