From 92430888fd93e4aa7c8a7f8e46fe358ad619add3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Thu, 18 Mar 2021 10:03:19 +0100 Subject: [PATCH] Add thread annotations to FakeIceTransport Bug: webrtc:12339 Change-Id: I29f5c910c60155cbb48c686e77b02ad3aa761fb1 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/211665 Reviewed-by: Harald Alvestrand Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/master@{#33500} --- p2p/base/fake_dtls_transport.h | 8 +- p2p/base/fake_ice_transport.h | 180 ++++++++++++++++++++++++--------- pc/channel_manager_unittest.cc | 33 +++--- pc/channel_unittest.cc | 50 +++++---- 4 files changed, 191 insertions(+), 80 deletions(-) diff --git a/p2p/base/fake_dtls_transport.h b/p2p/base/fake_dtls_transport.h index f17eddfe6e..daec1586e1 100644 --- a/p2p/base/fake_dtls_transport.h +++ b/p2p/base/fake_dtls_transport.h @@ -55,9 +55,15 @@ class FakeDtlsTransport : public DtlsTransportInternal { // If this constructor is called, a new fake ICE transport will be created, // and this FakeDtlsTransport will take the ownership. - explicit FakeDtlsTransport(const std::string& name, int component) + FakeDtlsTransport(const std::string& name, int component) : FakeDtlsTransport(std::make_unique(name, component)) { } + FakeDtlsTransport(const std::string& name, + int component, + rtc::Thread* network_thread) + : FakeDtlsTransport(std::make_unique(name, + component, + network_thread)) {} ~FakeDtlsTransport() override { if (dest_ && dest_->dest_ == this) { diff --git a/p2p/base/fake_ice_transport.h b/p2p/base/fake_ice_transport.h index f39da7cc89..58d83d761c 100644 --- a/p2p/base/fake_ice_transport.h +++ b/p2p/base/fake_ice_transport.h @@ -25,6 +25,9 @@ namespace cricket { +// All methods must be called on the network thread (which is either the thread +// calling the constructor, or the separate thread explicitly passed to the +// constructor). class FakeIceTransport : public IceTransportInternal { public: explicit FakeIceTransport(const std::string& name, @@ -34,6 +37,8 @@ class FakeIceTransport : public IceTransportInternal { component_(component), network_thread_(network_thread ? network_thread : rtc::Thread::Current()) {} + // Must be called either on the network thread, or after the network thread + // has been shut down. ~FakeIceTransport() override { if (dest_ && dest_->dest_ == this) { dest_->dest_ = nullptr; @@ -42,18 +47,31 @@ class FakeIceTransport : public IceTransportInternal { // If async, will send packets by "Post"-ing to message queue instead of // synchronously "Send"-ing. - void SetAsync(bool async) { async_ = async; } - void SetAsyncDelay(int delay_ms) { async_delay_ms_ = delay_ms; } + void SetAsync(bool async) { + RTC_DCHECK_RUN_ON(network_thread_); + async_ = async; + } + void SetAsyncDelay(int delay_ms) { + RTC_DCHECK_RUN_ON(network_thread_); + async_delay_ms_ = delay_ms; + } // SetWritable, SetReceiving and SetDestination are the main methods that can // be used for testing, to simulate connectivity or lack thereof. - void SetWritable(bool writable) { set_writable(writable); } - void SetReceiving(bool receiving) { set_receiving(receiving); } + void SetWritable(bool writable) { + RTC_DCHECK_RUN_ON(network_thread_); + set_writable(writable); + } + void SetReceiving(bool receiving) { + RTC_DCHECK_RUN_ON(network_thread_); + set_receiving(receiving); + } // Simulates the two transports connecting to each other. // If |asymmetric| is true this method only affects this FakeIceTransport. // If false, it affects |dest| as well. void SetDestination(FakeIceTransport* dest, bool asymmetric = false) { + RTC_DCHECK_RUN_ON(network_thread_); if (dest == dest_) { return; } @@ -75,12 +93,14 @@ class FakeIceTransport : public IceTransportInternal { void SetTransportState(webrtc::IceTransportState state, IceTransportState legacy_state) { + RTC_DCHECK_RUN_ON(network_thread_); transport_state_ = state; legacy_transport_state_ = legacy_state; SignalIceTransportStateChanged(this); } void SetConnectionCount(size_t connection_count) { + RTC_DCHECK_RUN_ON(network_thread_); size_t old_connection_count = connection_count_; connection_count_ = connection_count; if (connection_count) { @@ -94,6 +114,7 @@ class FakeIceTransport : public IceTransportInternal { } void SetCandidatesGatheringComplete() { + RTC_DCHECK_RUN_ON(network_thread_); if (gathering_state_ != kIceGatheringComplete) { gathering_state_ = kIceGatheringComplete; SignalGatheringState(this); @@ -102,16 +123,29 @@ class FakeIceTransport : public IceTransportInternal { // Convenience functions for accessing ICE config and other things. int receiving_timeout() const { + RTC_DCHECK_RUN_ON(network_thread_); return ice_config_.receiving_timeout_or_default(); } - bool gather_continually() const { return ice_config_.gather_continually(); } - const Candidates& remote_candidates() const { return remote_candidates_; } + bool gather_continually() const { + RTC_DCHECK_RUN_ON(network_thread_); + return ice_config_.gather_continually(); + } + const Candidates& remote_candidates() const { + RTC_DCHECK_RUN_ON(network_thread_); + return remote_candidates_; + } // Fake IceTransportInternal implementation. const std::string& transport_name() const override { return name_; } int component() const override { return component_; } - uint64_t IceTiebreaker() const { return tiebreaker_; } - IceMode remote_ice_mode() const { return remote_ice_mode_; } + uint64_t IceTiebreaker() const { + RTC_DCHECK_RUN_ON(network_thread_); + return tiebreaker_; + } + IceMode remote_ice_mode() const { + RTC_DCHECK_RUN_ON(network_thread_); + return remote_ice_mode_; + } const std::string& ice_ufrag() const { return ice_parameters_.ufrag; } const std::string& ice_pwd() const { return ice_parameters_.pwd; } const std::string& remote_ice_ufrag() const { @@ -126,6 +160,7 @@ class FakeIceTransport : public IceTransportInternal { } IceTransportState GetState() const override { + RTC_DCHECK_RUN_ON(network_thread_); if (legacy_transport_state_) { return *legacy_transport_state_; } @@ -143,6 +178,7 @@ class FakeIceTransport : public IceTransportInternal { } webrtc::IceTransportState GetIceTransportState() const override { + RTC_DCHECK_RUN_ON(network_thread_); if (transport_state_) { return *transport_state_; } @@ -159,21 +195,34 @@ class FakeIceTransport : public IceTransportInternal { return webrtc::IceTransportState::kConnected; } - void SetIceRole(IceRole role) override { role_ = role; } - IceRole GetIceRole() const override { return role_; } + void SetIceRole(IceRole role) override { + RTC_DCHECK_RUN_ON(network_thread_); + role_ = role; + } + IceRole GetIceRole() const override { + RTC_DCHECK_RUN_ON(network_thread_); + return role_; + } void SetIceTiebreaker(uint64_t tiebreaker) override { + RTC_DCHECK_RUN_ON(network_thread_); tiebreaker_ = tiebreaker; } void SetIceParameters(const IceParameters& ice_params) override { + RTC_DCHECK_RUN_ON(network_thread_); ice_parameters_ = ice_params; } void SetRemoteIceParameters(const IceParameters& params) override { + RTC_DCHECK_RUN_ON(network_thread_); remote_ice_parameters_ = params; } - void SetRemoteIceMode(IceMode mode) override { remote_ice_mode_ = mode; } + void SetRemoteIceMode(IceMode mode) override { + RTC_DCHECK_RUN_ON(network_thread_); + remote_ice_mode_ = mode; + } void MaybeStartGathering() override { + RTC_DCHECK_RUN_ON(network_thread_); if (gathering_state_ == kIceGatheringNew) { gathering_state_ = kIceGatheringGathering; SignalGatheringState(this); @@ -181,15 +230,21 @@ class FakeIceTransport : public IceTransportInternal { } IceGatheringState gathering_state() const override { + RTC_DCHECK_RUN_ON(network_thread_); return gathering_state_; } - void SetIceConfig(const IceConfig& config) override { ice_config_ = config; } + void SetIceConfig(const IceConfig& config) override { + RTC_DCHECK_RUN_ON(network_thread_); + ice_config_ = config; + } void AddRemoteCandidate(const Candidate& candidate) override { + RTC_DCHECK_RUN_ON(network_thread_); remote_candidates_.push_back(candidate); } void RemoveRemoteCandidate(const Candidate& candidate) override { + RTC_DCHECK_RUN_ON(network_thread_); auto it = absl::c_find(remote_candidates_, candidate); if (it == remote_candidates_.end()) { RTC_LOG(LS_INFO) << "Trying to remove a candidate which doesn't exist."; @@ -199,7 +254,10 @@ class FakeIceTransport : public IceTransportInternal { remote_candidates_.erase(it); } - void RemoveAllRemoteCandidates() override { remote_candidates_.clear(); } + void RemoveAllRemoteCandidates() override { + RTC_DCHECK_RUN_ON(network_thread_); + remote_candidates_.clear(); + } bool GetStats(IceTransportStats* ice_transport_stats) override { CandidateStats candidate_stats; @@ -220,17 +278,25 @@ class FakeIceTransport : public IceTransportInternal { } // Fake PacketTransportInternal implementation. - bool writable() const override { return writable_; } - bool receiving() const override { return receiving_; } + bool writable() const override { + RTC_DCHECK_RUN_ON(network_thread_); + return writable_; + } + bool receiving() const override { + RTC_DCHECK_RUN_ON(network_thread_); + return receiving_; + } // If combine is enabled, every two consecutive packets to be sent with // "SendPacket" will be combined into one outgoing packet. void combine_outgoing_packets(bool combine) { + RTC_DCHECK_RUN_ON(network_thread_); combine_outgoing_packets_ = combine; } int SendPacket(const char* data, size_t len, const rtc::PacketOptions& options, int flags) override { + RTC_DCHECK_RUN_ON(network_thread_); if (!dest_) { return -1; } @@ -240,8 +306,11 @@ class FakeIceTransport : public IceTransportInternal { rtc::CopyOnWriteBuffer packet(std::move(send_packet_)); if (async_) { invoker_.AsyncInvokeDelayed( - RTC_FROM_HERE, rtc::Thread::Current(), - [this, packet] { FakeIceTransport::SendPacketInternal(packet); }, + RTC_FROM_HERE, network_thread_, + [this, packet] { + RTC_DCHECK_RUN_ON(network_thread_); + FakeIceTransport::SendPacketInternal(packet); + }, async_delay_ms_); } else { SendPacketInternal(packet); @@ -253,10 +322,12 @@ class FakeIceTransport : public IceTransportInternal { } int SetOption(rtc::Socket::Option opt, int value) override { + RTC_DCHECK_RUN_ON(network_thread_); socket_options_[opt] = value; return true; } bool GetOption(rtc::Socket::Option opt, int* value) override { + RTC_DCHECK_RUN_ON(network_thread_); auto it = socket_options_.find(opt); if (it != socket_options_.end()) { *value = it->second; @@ -268,19 +339,27 @@ class FakeIceTransport : public IceTransportInternal { int GetError() override { return 0; } - rtc::CopyOnWriteBuffer last_sent_packet() { return last_sent_packet_; } + rtc::CopyOnWriteBuffer last_sent_packet() { + RTC_DCHECK_RUN_ON(network_thread_); + return last_sent_packet_; + } absl::optional network_route() const override { + RTC_DCHECK_RUN_ON(network_thread_); return network_route_; } void SetNetworkRoute(absl::optional network_route) { + RTC_DCHECK_RUN_ON(network_thread_); network_route_ = network_route; - network_thread_->Invoke( - RTC_FROM_HERE, [this] { SignalNetworkRouteChanged(network_route_); }); + network_thread_->Invoke(RTC_FROM_HERE, [this] { + RTC_DCHECK_RUN_ON(network_thread_); + SignalNetworkRouteChanged(network_route_); + }); } private: - void set_writable(bool writable) { + void set_writable(bool writable) + RTC_EXCLUSIVE_LOCKS_REQUIRED(network_thread_) { if (writable_ == writable) { return; } @@ -292,7 +371,8 @@ class FakeIceTransport : public IceTransportInternal { SignalWritableState(this); } - void set_receiving(bool receiving) { + void set_receiving(bool receiving) + RTC_EXCLUSIVE_LOCKS_REQUIRED(network_thread_) { if (receiving_ == receiving) { return; } @@ -300,7 +380,8 @@ class FakeIceTransport : public IceTransportInternal { SignalReceivingState(this); } - void SendPacketInternal(const rtc::CopyOnWriteBuffer& packet) { + void SendPacketInternal(const rtc::CopyOnWriteBuffer& packet) + RTC_EXCLUSIVE_LOCKS_REQUIRED(network_thread_) { if (dest_) { last_sent_packet_ = packet; dest_->SignalReadPacket(dest_, packet.data(), packet.size(), @@ -309,30 +390,35 @@ class FakeIceTransport : public IceTransportInternal { } rtc::AsyncInvoker invoker_; - std::string name_; - int component_; - FakeIceTransport* dest_ = nullptr; - bool async_ = false; - int async_delay_ms_ = 0; - Candidates remote_candidates_; - IceConfig ice_config_; - IceRole role_ = ICEROLE_UNKNOWN; - uint64_t tiebreaker_ = 0; - IceParameters ice_parameters_; - IceParameters remote_ice_parameters_; - IceMode remote_ice_mode_ = ICEMODE_FULL; - size_t connection_count_ = 0; - absl::optional transport_state_; - absl::optional legacy_transport_state_; - IceGatheringState gathering_state_ = kIceGatheringNew; - bool had_connection_ = false; - bool writable_ = false; - bool receiving_ = false; - bool combine_outgoing_packets_ = false; - rtc::CopyOnWriteBuffer send_packet_; - absl::optional network_route_; - std::map socket_options_; - rtc::CopyOnWriteBuffer last_sent_packet_; + const std::string name_; + const int component_; + FakeIceTransport* dest_ RTC_GUARDED_BY(network_thread_) = nullptr; + bool async_ RTC_GUARDED_BY(network_thread_) = false; + int async_delay_ms_ RTC_GUARDED_BY(network_thread_) = 0; + Candidates remote_candidates_ RTC_GUARDED_BY(network_thread_); + IceConfig ice_config_ RTC_GUARDED_BY(network_thread_); + IceRole role_ RTC_GUARDED_BY(network_thread_) = ICEROLE_UNKNOWN; + uint64_t tiebreaker_ RTC_GUARDED_BY(network_thread_) = 0; + IceParameters ice_parameters_ RTC_GUARDED_BY(network_thread_); + IceParameters remote_ice_parameters_ RTC_GUARDED_BY(network_thread_); + IceMode remote_ice_mode_ RTC_GUARDED_BY(network_thread_) = ICEMODE_FULL; + size_t connection_count_ RTC_GUARDED_BY(network_thread_) = 0; + absl::optional transport_state_ + RTC_GUARDED_BY(network_thread_); + absl::optional legacy_transport_state_ + RTC_GUARDED_BY(network_thread_); + IceGatheringState gathering_state_ RTC_GUARDED_BY(network_thread_) = + kIceGatheringNew; + bool had_connection_ RTC_GUARDED_BY(network_thread_) = false; + bool writable_ RTC_GUARDED_BY(network_thread_) = false; + bool receiving_ RTC_GUARDED_BY(network_thread_) = false; + bool combine_outgoing_packets_ RTC_GUARDED_BY(network_thread_) = false; + rtc::CopyOnWriteBuffer send_packet_ RTC_GUARDED_BY(network_thread_); + absl::optional network_route_ + RTC_GUARDED_BY(network_thread_); + std::map socket_options_ + RTC_GUARDED_BY(network_thread_); + rtc::CopyOnWriteBuffer last_sent_packet_ RTC_GUARDED_BY(network_thread_); rtc::Thread* const network_thread_; }; diff --git a/pc/channel_manager_unittest.cc b/pc/channel_manager_unittest.cc index 610d7979ab..c0dddd89cf 100644 --- a/pc/channel_manager_unittest.cc +++ b/pc/channel_manager_unittest.cc @@ -62,16 +62,6 @@ class ChannelManagerTest : public ::testing::Test { fme_->SetVideoCodecs(MAKE_VECTOR(kVideoCodecs)); } - std::unique_ptr CreateDtlsSrtpTransport() { - rtp_dtls_transport_ = std::make_unique( - "fake_dtls_transport", cricket::ICE_CANDIDATE_COMPONENT_RTP); - auto dtls_srtp_transport = std::make_unique( - /*rtcp_mux_required=*/true); - dtls_srtp_transport->SetDtlsTransports(rtp_dtls_transport_.get(), - /*rtcp_dtls_transport=*/nullptr); - return dtls_srtp_transport; - } - void TestCreateDestroyChannels(webrtc::RtpTransportInternal* rtp_transport) { cricket::VoiceChannel* voice_channel = cm_->CreateVoiceChannel( &fake_call_, cricket::MediaConfig(), rtp_transport, @@ -95,7 +85,6 @@ class ChannelManagerTest : public ::testing::Test { cm_->Terminate(); } - std::unique_ptr rtp_dtls_transport_; std::unique_ptr network_; std::unique_ptr worker_; std::unique_ptr @@ -178,8 +167,13 @@ TEST_F(ChannelManagerTest, SetVideoRtxEnabled) { TEST_F(ChannelManagerTest, CreateDestroyChannels) { EXPECT_TRUE(cm_->Init()); - auto rtp_transport = CreateDtlsSrtpTransport(); - TestCreateDestroyChannels(rtp_transport.get()); + auto rtp_dtls_transport = std::make_unique( + "fake_dtls_transport", cricket::ICE_CANDIDATE_COMPONENT_RTP); + auto dtls_srtp_transport = std::make_unique( + /*rtcp_mux_required=*/true); + dtls_srtp_transport->SetDtlsTransports(rtp_dtls_transport.get(), + /*rtcp_dtls_transport=*/nullptr); + TestCreateDestroyChannels(dtls_srtp_transport.get()); } TEST_F(ChannelManagerTest, CreateDestroyChannelsOnThread) { @@ -188,8 +182,17 @@ TEST_F(ChannelManagerTest, CreateDestroyChannelsOnThread) { EXPECT_TRUE(cm_->set_worker_thread(worker_.get())); EXPECT_TRUE(cm_->set_network_thread(network_.get())); EXPECT_TRUE(cm_->Init()); - auto rtp_transport = CreateDtlsSrtpTransport(); - TestCreateDestroyChannels(rtp_transport.get()); + auto rtp_dtls_transport = std::make_unique( + "fake_dtls_transport", cricket::ICE_CANDIDATE_COMPONENT_RTP, + network_.get()); + auto dtls_srtp_transport = std::make_unique( + /*rtcp_mux_required=*/true); + network_->Invoke( + RTC_FROM_HERE, [&rtp_dtls_transport, &dtls_srtp_transport] { + dtls_srtp_transport->SetDtlsTransports(rtp_dtls_transport.get(), + /*rtcp_dtls_transport=*/nullptr); + }); + TestCreateDestroyChannels(dtls_srtp_transport.get()); } } // namespace cricket diff --git a/pc/channel_unittest.cc b/pc/channel_unittest.cc index 4a0a6b4a15..ea4e828226 100644 --- a/pc/channel_unittest.cc +++ b/pc/channel_unittest.cc @@ -170,11 +170,12 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { } else { // Confirmed to work with KT_RSA and KT_ECDSA. fake_rtp_dtls_transport1_.reset(new cricket::FakeDtlsTransport( - "channel1", cricket::ICE_CANDIDATE_COMPONENT_RTP)); + "channel1", cricket::ICE_CANDIDATE_COMPONENT_RTP, network_thread_)); rtp1 = fake_rtp_dtls_transport1_.get(); if (!(flags1 & RTCP_MUX)) { fake_rtcp_dtls_transport1_.reset(new cricket::FakeDtlsTransport( - "channel1", cricket::ICE_CANDIDATE_COMPONENT_RTCP)); + "channel1", cricket::ICE_CANDIDATE_COMPONENT_RTCP, + network_thread_)); rtcp1 = fake_rtcp_dtls_transport1_.get(); } if (flags1 & DTLS) { @@ -199,11 +200,12 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { } else { // Confirmed to work with KT_RSA and KT_ECDSA. fake_rtp_dtls_transport2_.reset(new cricket::FakeDtlsTransport( - "channel2", cricket::ICE_CANDIDATE_COMPONENT_RTP)); + "channel2", cricket::ICE_CANDIDATE_COMPONENT_RTP, network_thread_)); rtp2 = fake_rtp_dtls_transport2_.get(); if (!(flags2 & RTCP_MUX)) { fake_rtcp_dtls_transport2_.reset(new cricket::FakeDtlsTransport( - "channel2", cricket::ICE_CANDIDATE_COMPONENT_RTCP)); + "channel2", cricket::ICE_CANDIDATE_COMPONENT_RTCP, + network_thread_)); rtcp2 = fake_rtcp_dtls_transport2_.get(); } if (flags2 & DTLS) { @@ -284,10 +286,14 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { auto rtp_transport = std::make_unique( rtcp_packet_transport == nullptr); - rtp_transport->SetRtpPacketTransport(rtp_packet_transport); - if (rtcp_packet_transport) { - rtp_transport->SetRtcpPacketTransport(rtcp_packet_transport); - } + network_thread_->Invoke( + RTC_FROM_HERE, + [&rtp_transport, rtp_packet_transport, rtcp_packet_transport] { + rtp_transport->SetRtpPacketTransport(rtp_packet_transport); + if (rtcp_packet_transport) { + rtp_transport->SetRtcpPacketTransport(rtcp_packet_transport); + } + }); return rtp_transport; } @@ -297,8 +303,12 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { auto dtls_srtp_transport = std::make_unique( rtcp_dtls_transport == nullptr); - dtls_srtp_transport->SetDtlsTransports(rtp_dtls_transport, - rtcp_dtls_transport); + network_thread_->Invoke( + RTC_FROM_HERE, + [&dtls_srtp_transport, rtp_dtls_transport, rtcp_dtls_transport] { + dtls_srtp_transport->SetDtlsTransports(rtp_dtls_transport, + rtcp_dtls_transport); + }); return dtls_srtp_transport; } @@ -1268,13 +1278,19 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> { fake_rtp_dtls_transport2_.get(), fake_rtcp_dtls_transport2_.get()); channel1_->SetRtpTransport(new_rtp_transport_.get()); - int option_val; - ASSERT_TRUE(fake_rtp_dtls_transport2_->GetOption( - rtc::Socket::Option::OPT_SNDBUF, &option_val)); - EXPECT_EQ(kSndBufSize, option_val); - ASSERT_TRUE(fake_rtp_dtls_transport2_->GetOption( - rtc::Socket::Option::OPT_RCVBUF, &option_val)); - EXPECT_EQ(kRcvBufSize, option_val); + bool rcv_success, send_success; + int rcv_buf, send_buf; + network_thread_->Invoke(RTC_FROM_HERE, [&] { + send_success = fake_rtp_dtls_transport2_->GetOption( + rtc::Socket::Option::OPT_SNDBUF, &send_buf); + rcv_success = fake_rtp_dtls_transport2_->GetOption( + rtc::Socket::Option::OPT_RCVBUF, &rcv_buf); + }); + + ASSERT_TRUE(send_success); + EXPECT_EQ(kSndBufSize, send_buf); + ASSERT_TRUE(rcv_success); + EXPECT_EQ(kRcvBufSize, rcv_buf); } void CreateSimulcastContent(const std::vector& rids,