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 <hta@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#41272}
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 48cdce2..611d5b4 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -629,7 +629,6 @@
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 @@
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 @@
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 @@
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 @@
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 @@
std::unique_ptr<RtcEventLogOutput> 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 @@
void PeerConnection::StopRtcEventLog_w() {
RTC_DCHECK_RUN_ON(worker_thread());
- if (event_log_) {
- event_log_->StopLogging();
- }
+ event_log_->StopLogging();
}
absl::optional<rtc::SSLRole> PeerConnection::GetSctpSslRole_n() {
diff --git a/pc/peer_connection.h b/pc/peer_connection.h
index a345089..b916265 100644
--- a/pc/peer_connection.h
+++ b/pc/peer_connection.h
@@ -611,11 +611,8 @@
const bool is_unified_plan_;
// The EventLog needs to outlive `call_` (and any other object that uses it).
- std::unique_ptr<RtcEventLog> 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<RtcEventLog> 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 7e3da9b..35e8c2b 100644
--- a/pc/peer_connection_integrationtest.cc
+++ b/pc/peer_connection_integrationtest.cc
@@ -104,6 +104,12 @@
namespace {
+using ::testing::AtLeast;
+using ::testing::InSequence;
+using ::testing::MockFunction;
+using ::testing::NiceMock;
+using ::testing::Return;
+
class PeerConnectionIntegrationTest
: public PeerConnectionIntegrationBaseTest,
public ::testing::WithParamInterface<SdpSemantics> {
@@ -2812,12 +2818,10 @@
ASSERT_TRUE(CreatePeerConnectionWrappers());
ConnectFakeSignaling();
- auto output = std::make_unique<testing::NiceMock<MockRtcEventLogOutput>>();
- ON_CALL(*output, IsActive()).WillByDefault(::testing::Return(true));
- ON_CALL(*output, Write(::testing::A<absl::string_view>()))
- .WillByDefault(::testing::Return(true));
- EXPECT_CALL(*output, Write(::testing::A<absl::string_view>()))
- .Times(::testing::AtLeast(1));
+ auto output = std::make_unique<NiceMock<MockRtcEventLogOutput>>();
+ 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 @@
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<void()> test_is_complete;
+ ASSERT_TRUE(CreatePeerConnectionWrappers());
+ ConnectFakeSignaling();
+
+ auto output = std::make_unique<NiceMock<MockRtcEventLogOutput>>();
+ 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<void()> test_is_complete;
+ ASSERT_TRUE(CreatePeerConnectionWrappers());
+ ConnectFakeSignaling();
+
+ auto output = std::make_unique<NiceMock<MockRtcEventLogOutput>>();
+ 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.
