From ff84d86d9c57bf154e9329ac29f6cb0448be5e8e Mon Sep 17 00:00:00 2001 From: Markus Handell Date: Tue, 26 May 2020 18:22:24 +0200 Subject: [PATCH] P2PTransportChannel::OnCandidateResolved: fix resolver leak. This change fixes a problem where the eventual destruction of a completed resolver sometimes doesn't happen. This is because the destruction is posted to the network thread, and if it's destroyed before the closure is executed, the resolver is leaked. The fix is in three parts: 1. The resolver->Destroy call is performed on closure destruction to make sure it will always run. 2. The closure is executed with task queue. This because the RTC_DCHECK on thread:140 fires with the invoker_. 3. It's not possible to guarantee the context Destroy is called on due to TaskQueue semantics. Therefore SignalThread::Destroy was changed to accept any calling context and only requiring it's the last public call to the object. For unknown reasons, this leak doesn't trigger the leak checker, see referred bugs for further investigation. Bug: webrtc:7723, webrtc:11605, chromium:905542 Change-Id: I2681ff1d2416ccbc564974a65ac84781a9ed7aee Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/176125 Commit-Queue: Markus Handell Reviewed-by: Tommi Cr-Commit-Position: refs/heads/master@{#31359} --- p2p/BUILD.gn | 1 + p2p/base/p2p_transport_channel.cc | 6 +++--- p2p/base/p2p_transport_channel_unittest.cc | 13 ++++++++----- rtc_base/signal_thread.cc | 11 ++++++++++- rtc_base/signal_thread.h | 5 +++-- 5 files changed, 25 insertions(+), 11 deletions(-) diff --git a/p2p/BUILD.gn b/p2p/BUILD.gn index ae49deb264..e9c01cde3d 100644 --- a/p2p/BUILD.gn +++ b/p2p/BUILD.gn @@ -107,6 +107,7 @@ rtc_library("rtc_p2p") { "../rtc_base/memory:fifo_buffer", "../rtc_base/network:sent_packet", "../rtc_base/system:rtc_export", + "../rtc_base/task_utils:to_queued_task", "../rtc_base/third_party/base64", "../rtc_base/third_party/sigslot", "../system_wrappers:field_trial", diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index 73d12c7741..90d3e14d1c 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -30,6 +30,7 @@ #include "rtc_base/net_helper.h" #include "rtc_base/net_helpers.h" #include "rtc_base/string_encode.h" +#include "rtc_base/task_utils/to_queued_task.h" #include "rtc_base/time_utils.h" #include "system_wrappers/include/field_trial.h" #include "system_wrappers/include/metrics.h" @@ -1223,9 +1224,8 @@ void P2PTransportChannel::OnCandidateResolved( Candidate candidate = p->candidate_; resolvers_.erase(p); AddRemoteCandidateWithResolver(candidate, resolver); - invoker_.AsyncInvoke( - RTC_FROM_HERE, thread(), - rtc::Bind(&rtc::AsyncResolverInterface::Destroy, resolver, false)); + thread()->PostTask( + webrtc::ToQueuedTask([] {}, [resolver] { resolver->Destroy(false); })); } void P2PTransportChannel::AddRemoteCandidateWithResolver( diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index f0d5d66db1..a3cf9370e2 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -4844,10 +4844,13 @@ TEST_F(P2PTransportChannelTest, // address after the resolution completes. TEST_F(P2PTransportChannelTest, PeerReflexiveCandidateDuringResolvingHostCandidateWithMdnsName) { - NiceMock mock_async_resolver; + auto mock_async_resolver = new NiceMock(); + ON_CALL(*mock_async_resolver, Destroy).WillByDefault([mock_async_resolver] { + delete mock_async_resolver; + }); webrtc::MockAsyncResolverFactory mock_async_resolver_factory; EXPECT_CALL(mock_async_resolver_factory, Create()) - .WillOnce(Return(&mock_async_resolver)); + .WillOnce(Return(mock_async_resolver)); // ep1 and ep2 will only gather host candidates with addresses // kPublicAddrs[0] and kPublicAddrs[1], respectively. @@ -4874,7 +4877,7 @@ TEST_F(P2PTransportChannelTest, bool mock_async_resolver_started = false; // Not signaling done yet, and only make sure we are in the process of // resolution. - EXPECT_CALL(mock_async_resolver, Start(_)) + EXPECT_CALL(*mock_async_resolver, Start(_)) .WillOnce(InvokeWithoutArgs([&mock_async_resolver_started]() { mock_async_resolver_started = true; })); @@ -4887,7 +4890,7 @@ TEST_F(P2PTransportChannelTest, ResumeCandidates(1); ASSERT_TRUE_WAIT(ep1_ch1()->selected_connection() != nullptr, kMediumTimeout); // Let the mock resolver of ep2 receives the correct resolution. - EXPECT_CALL(mock_async_resolver, GetResolvedAddress(_, _)) + EXPECT_CALL(*mock_async_resolver, GetResolvedAddress(_, _)) .WillOnce(DoAll(SetArgPointee<1>(local_address), Return(true))); // Upon receiving a ping from ep1, ep2 adds a prflx candidate from the // unknown address and establishes a connection. @@ -4899,7 +4902,7 @@ TEST_F(P2PTransportChannelTest, ep2_ch1()->selected_connection()->remote_candidate().type()); // ep2 should also be able resolve the hostname candidate. The resolved remote // host candidate should be merged with the prflx remote candidate. - mock_async_resolver.SignalDone(&mock_async_resolver); + mock_async_resolver->SignalDone(mock_async_resolver); EXPECT_EQ_WAIT(LOCAL_PORT_TYPE, ep2_ch1()->selected_connection()->remote_candidate().type(), kMediumTimeout); diff --git a/rtc_base/signal_thread.cc b/rtc_base/signal_thread.cc index e100fbe179..8f0d597f03 100644 --- a/rtc_base/signal_thread.cc +++ b/rtc_base/signal_thread.cc @@ -31,11 +31,13 @@ SignalThread::SignalThread() } SignalThread::~SignalThread() { + rtc::CritScope lock(&cs_); RTC_DCHECK(refcount_ == 0); } bool SignalThread::SetName(const std::string& name, const void* obj) { EnterExit ee(this); + RTC_DCHECK(!destroy_called_); RTC_DCHECK(main_->IsCurrent()); RTC_DCHECK(kInit == state_); return worker_.SetName(name, obj); @@ -43,6 +45,7 @@ bool SignalThread::SetName(const std::string& name, const void* obj) { void SignalThread::Start() { EnterExit ee(this); + RTC_DCHECK(!destroy_called_); RTC_DCHECK(main_->IsCurrent()); if (kInit == state_ || kComplete == state_) { state_ = kRunning; @@ -55,7 +58,11 @@ void SignalThread::Start() { void SignalThread::Destroy(bool wait) { EnterExit ee(this); - RTC_DCHECK(main_->IsCurrent()); + // Sometimes the caller can't guarantee which thread will call Destroy, only + // that it will be the last thing it does. + // RTC_DCHECK(main_->IsCurrent()); + RTC_DCHECK(!destroy_called_); + destroy_called_ = true; if ((kInit == state_) || (kComplete == state_)) { refcount_--; } else if (kRunning == state_ || kReleasing == state_) { @@ -78,6 +85,7 @@ void SignalThread::Destroy(bool wait) { void SignalThread::Release() { EnterExit ee(this); + RTC_DCHECK(!destroy_called_); RTC_DCHECK(main_->IsCurrent()); if (kComplete == state_) { refcount_--; @@ -91,6 +99,7 @@ void SignalThread::Release() { bool SignalThread::ContinueWork() { EnterExit ee(this); + RTC_DCHECK(!destroy_called_); RTC_DCHECK(worker_.IsCurrent()); return worker_.ProcessMessages(0); } diff --git a/rtc_base/signal_thread.h b/rtc_base/signal_thread.h index d9e8ade9b0..9229ca1abb 100644 --- a/rtc_base/signal_thread.h +++ b/rtc_base/signal_thread.h @@ -144,8 +144,9 @@ class SignalThread : public sigslot::has_slots<>, protected MessageHandler { Thread* main_; Worker worker_; CriticalSection cs_; - State state_; - int refcount_; + State state_ RTC_GUARDED_BY(cs_); + int refcount_ RTC_GUARDED_BY(cs_); + bool destroy_called_ RTC_GUARDED_BY(cs_) = false; RTC_DISALLOW_COPY_AND_ASSIGN(SignalThread); };