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}

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).
+    StopRtcEventLog_w();
-    event_log_.reset();
   });
   ReportUsagePattern();
 
@@ -2255,7 +2253,7 @@ bool PeerConnection::StartRtcEventLog_w(
     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 @@ 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<rtc::SSLRole> PeerConnection::GetSctpSslRole_n() {

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<RtcEventLog> event_log_ RTC_GUARDED_BY(worker_thread());
+  const std::unique_ptr<RtcEventLog> event_log_
-
+      RTC_PT_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());
 
   IceConnectionState ice_connection_state_ RTC_GUARDED_BY(signaling_thread()) =
       kIceConnectionNew;

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<SdpSemantics> {
@@ -2812,12 +2818,10 @@ TEST_P(PeerConnectionIntegrationTest, RtcEventLogOutputWriteCalled) {
   ASSERT_TRUE(CreatePeerConnectionWrappers());
   ConnectFakeSignaling();
 
-  auto output = std::make_unique<testing::NiceMock<MockRtcEventLogOutput>>();
+  auto output = std::make_unique<NiceMock<MockRtcEventLogOutput>>();
-  ON_CALL(*output, IsActive()).WillByDefault(::testing::Return(true));
+  ON_CALL(*output, IsActive).WillByDefault(Return(true));
-  ON_CALL(*output, Write(::testing::A<absl::string_view>()))
+  ON_CALL(*output, Write).WillByDefault(Return(true));
-      .WillByDefault(::testing::Return(true));
+  EXPECT_CALL(*output, Write).Times(AtLeast(1));
-  EXPECT_CALL(*output, Write(::testing::A<absl::string_view>()))
-      .Times(::testing::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<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.