From 49c35d377bdf53c8e69e91c08ad30d16fbd408da Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Mon, 27 Nov 2023 12:55:39 +0100 Subject: [PATCH] In PeerConnection postpone RtcEventLog destruction This is done as a preparation to move RtcEventLog ownership into Environment where destruction happens later, when all users of the Environment are deleted. Bug: webrtc:15656 Change-Id: I2a72c74f1fabb1e25c5200aa47a5d61e4b3d9cd9 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/328301 Reviewed-by: Harald Alvestrand Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/main@{#41272} --- pc/peer_connection.cc | 20 +++----- pc/peer_connection.h | 7 +-- pc/peer_connection_integrationtest.cc | 70 ++++++++++++++++++++++++--- 3 files changed, 74 insertions(+), 23 deletions(-) diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 48cdce2e3d..611d5b4720 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -629,7 +629,6 @@ PeerConnection::PeerConnection( observer_(dependencies.observer), is_unified_plan_(is_unified_plan), event_log_(std::move(event_log)), - event_log_ptr_(event_log_.get()), async_dns_resolver_factory_( std::move(dependencies.async_dns_resolver_factory)), port_allocator_(std::move(dependencies.allocator)), @@ -648,7 +647,9 @@ PeerConnection::PeerConnection( dtls_enabled_(dtls_enabled), data_channel_controller_(this), message_handler_(signaling_thread()), - weak_factory_(this) {} + weak_factory_(this) { + RTC_CHECK(event_log_); +} PeerConnection::~PeerConnection() { TRACE_EVENT0("webrtc", "PeerConnection::~PeerConnection"); @@ -699,13 +700,11 @@ PeerConnection::~PeerConnection() { sctp_mid_s_.reset(); SetSctpTransportName(""); - // call_ and event_log_ must be destroyed on the worker thread. + // call_ must be destroyed on the worker thread. worker_thread()->BlockingCall([this] { RTC_DCHECK_RUN_ON(worker_thread()); worker_thread_safety_->SetNotAlive(); call_.reset(); - // The event log must outlive call (and any other object that uses it). - event_log_.reset(); }); data_channel_controller_.PrepareForShutdown(); @@ -797,7 +796,7 @@ JsepTransportController* PeerConnection::InitializeTransportController_n( config.transport_observer = this; config.rtcp_handler = InitializeRtcpCallback(); config.un_demuxable_packet_handler = InitializeUnDemuxablePacketHandler(); - config.event_log = event_log_ptr_; + config.event_log = event_log_.get(); #if defined(ENABLE_EXTERNAL_AUTH) config.enable_external_auth = true; #endif @@ -1931,8 +1930,7 @@ void PeerConnection::Close() { RTC_DCHECK_RUN_ON(worker_thread()); worker_thread_safety_->SetNotAlive(); call_.reset(); - // The event log must outlive call (and any other object that uses it). - event_log_.reset(); + StopRtcEventLog_w(); }); ReportUsagePattern(); @@ -2255,7 +2253,7 @@ bool PeerConnection::StartRtcEventLog_w( std::unique_ptr output, int64_t output_period_ms) { RTC_DCHECK_RUN_ON(worker_thread()); - if (!event_log_) { + if (!worker_thread_safety_->alive()) { return false; } return event_log_->StartLogging(std::move(output), output_period_ms); @@ -2263,9 +2261,7 @@ bool PeerConnection::StartRtcEventLog_w( void PeerConnection::StopRtcEventLog_w() { RTC_DCHECK_RUN_ON(worker_thread()); - if (event_log_) { - event_log_->StopLogging(); - } + event_log_->StopLogging(); } absl::optional PeerConnection::GetSctpSslRole_n() { diff --git a/pc/peer_connection.h b/pc/peer_connection.h index a345089191..b91626575f 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -611,11 +611,8 @@ class PeerConnection : public PeerConnectionInternal, const bool is_unified_plan_; // The EventLog needs to outlive `call_` (and any other object that uses it). - std::unique_ptr event_log_ RTC_GUARDED_BY(worker_thread()); - - // Points to the same thing as `event_log_`. Since it's const, we may read the - // pointer (but not touch the object) from any thread. - RtcEventLog* const event_log_ptr_ RTC_PT_GUARDED_BY(worker_thread()); + const std::unique_ptr event_log_ + RTC_PT_GUARDED_BY(worker_thread()); IceConnectionState ice_connection_state_ RTC_GUARDED_BY(signaling_thread()) = kIceConnectionNew; diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index 7e3da9bac1..35e8c2b002 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -104,6 +104,12 @@ namespace webrtc { namespace { +using ::testing::AtLeast; +using ::testing::InSequence; +using ::testing::MockFunction; +using ::testing::NiceMock; +using ::testing::Return; + class PeerConnectionIntegrationTest : public PeerConnectionIntegrationBaseTest, public ::testing::WithParamInterface { @@ -2812,12 +2818,10 @@ TEST_P(PeerConnectionIntegrationTest, RtcEventLogOutputWriteCalled) { ASSERT_TRUE(CreatePeerConnectionWrappers()); ConnectFakeSignaling(); - auto output = std::make_unique>(); - ON_CALL(*output, IsActive()).WillByDefault(::testing::Return(true)); - ON_CALL(*output, Write(::testing::A())) - .WillByDefault(::testing::Return(true)); - EXPECT_CALL(*output, Write(::testing::A())) - .Times(::testing::AtLeast(1)); + auto output = std::make_unique>(); + ON_CALL(*output, IsActive).WillByDefault(Return(true)); + ON_CALL(*output, Write).WillByDefault(Return(true)); + EXPECT_CALL(*output, Write).Times(AtLeast(1)); EXPECT_TRUE(caller()->pc()->StartRtcEventLog(std::move(output), RtcEventLog::kImmediateOutput)); @@ -2826,6 +2830,60 @@ TEST_P(PeerConnectionIntegrationTest, RtcEventLogOutputWriteCalled) { ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); } +TEST_P(PeerConnectionIntegrationTest, RtcEventLogOutputWriteCalledOnStop) { + // This test uses check point to ensure log is written before peer connection + // is destroyed. + // https://google.github.io/googletest/gmock_cook_book.html#UsingCheckPoints + MockFunction test_is_complete; + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignaling(); + + auto output = std::make_unique>(); + ON_CALL(*output, IsActive).WillByDefault(Return(true)); + ON_CALL(*output, Write).WillByDefault(Return(true)); + InSequence s; + EXPECT_CALL(*output, Write).Times(AtLeast(1)); + EXPECT_CALL(test_is_complete, Call); + + // Use large output period to prevent this test pass for the wrong reason. + EXPECT_TRUE(caller()->pc()->StartRtcEventLog(std::move(output), + /*output_period_ms=*/100'000)); + + caller()->AddAudioVideoTracks(); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + + caller()->pc()->StopRtcEventLog(); + test_is_complete.Call(); +} + +TEST_P(PeerConnectionIntegrationTest, RtcEventLogOutputWriteCalledOnClose) { + // This test uses check point to ensure log is written before peer connection + // is destroyed. + // https://google.github.io/googletest/gmock_cook_book.html#UsingCheckPoints + MockFunction test_is_complete; + ASSERT_TRUE(CreatePeerConnectionWrappers()); + ConnectFakeSignaling(); + + auto output = std::make_unique>(); + ON_CALL(*output, IsActive).WillByDefault(Return(true)); + ON_CALL(*output, Write).WillByDefault(Return(true)); + InSequence s; + EXPECT_CALL(*output, Write).Times(AtLeast(1)); + EXPECT_CALL(test_is_complete, Call); + + // Use large output period to prevent this test pass for the wrong reason. + EXPECT_TRUE(caller()->pc()->StartRtcEventLog(std::move(output), + /*output_period_ms=*/100'000)); + + caller()->AddAudioVideoTracks(); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + + caller()->pc()->Close(); + test_is_complete.Call(); +} + // Test that if candidates are only signaled by applying full session // descriptions (instead of using AddIceCandidate), the peers can connect to // each other and exchange media.