From 89f095cb071f99fd5bb073bca65a9b04dda50a72 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henrik=20Bostr=C3=B6m?= Date: Mon, 3 Apr 2023 17:57:47 +0200 Subject: [PATCH] Deflake PeerConnectionSimulcastMediaFlowTests due to unstopped sources. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only in testing environments are the task queues shut down while sources still have media flowing. It's still not clear why heap-use-after-free happens, since it should be enough to close the PC, but it is clear that the crash is happening due to frames flowing while the test is shutting down, which is not something happening outside of testing. In an attempt to deflake, this CL makes sure to manually stop the test-only sources before closing the peer connection. Bug: webrtc:15018 Change-Id: I48ee131a8994c9c4caee1bb4875580d255b97da1 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/299944 Commit-Queue: Henrik Boström Auto-Submit: Henrik Boström Reviewed-by: Jeremy Leconte Cr-Commit-Position: refs/heads/main@{#39752} --- pc/peer_connection_simulcast_unittest.cc | 12 ------------ pc/test/fake_periodic_video_track_source.h | 1 + pc/test/peer_connection_test_wrapper.cc | 12 +++++++++++- pc/test/peer_connection_test_wrapper.h | 4 ++++ 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/pc/peer_connection_simulcast_unittest.cc b/pc/peer_connection_simulcast_unittest.cc index 5ffbe34971..f975d370c7 100644 --- a/pc/peer_connection_simulcast_unittest.cc +++ b/pc/peer_connection_simulcast_unittest.cc @@ -176,11 +176,7 @@ const webrtc::RTCOutboundRtpStreamStats* FindOutboundRtpByRid( namespace webrtc { constexpr TimeDelta kDefaultTimeout = TimeDelta::Seconds(5); - -// Only used by tests disabled under ASAN, avoids unsued variable compile error. -#if !defined(ADDRESS_SANITIZER) constexpr TimeDelta kLongTimeoutForRampingUp = TimeDelta::Seconds(30); -#endif // !defined(ADDRESS_SANITIZER) class PeerConnectionSimulcastTests : public ::testing::Test { public: @@ -1173,12 +1169,6 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, EXPECT_THAT(*outbound_rtps[0]->scalability_mode, StrEq("L1T1")); } -// TODO(https://crbug.com/webrtc/15018): Investigate heap-use-after free during -// shutdown of the test that is flakily happening on bots. It's not only -// happening on ASAN, but it is rare enough on non-ASAN that we don't have to -// disable everywhere. -#if !defined(ADDRESS_SANITIZER) - TEST_F(PeerConnectionSimulcastWithMediaFlowTests, SendingThreeEncodings_VP8_Simulcast) { rtc::scoped_refptr local_pc_wrapper = CreatePc(); @@ -1757,6 +1747,4 @@ TEST_F(PeerConnectionSimulcastWithMediaFlowTests, EXPECT_THAT(*outbound_rtps[2]->scalability_mode, StrEq("L1T3")); } -#endif // !defined(ADDRESS_SANITIZER) - } // namespace webrtc diff --git a/pc/test/fake_periodic_video_track_source.h b/pc/test/fake_periodic_video_track_source.h index 98a456f232..f91144d1cc 100644 --- a/pc/test/fake_periodic_video_track_source.h +++ b/pc/test/fake_periodic_video_track_source.h @@ -29,6 +29,7 @@ class FakePeriodicVideoTrackSource : public VideoTrackSource { ~FakePeriodicVideoTrackSource() = default; + FakePeriodicVideoSource& fake_periodic_source() { return source_; } const FakePeriodicVideoSource& fake_periodic_source() const { return source_; } diff --git a/pc/test/peer_connection_test_wrapper.cc b/pc/test/peer_connection_test_wrapper.cc index cd66eaec78..112f04d66b 100644 --- a/pc/test/peer_connection_test_wrapper.cc +++ b/pc/test/peer_connection_test_wrapper.cc @@ -39,7 +39,6 @@ #include "p2p/base/fake_port_allocator.h" #include "p2p/base/port_allocator.h" #include "pc/test/fake_periodic_video_source.h" -#include "pc/test/fake_periodic_video_track_source.h" #include "pc/test/fake_rtc_certificate_generator.h" #include "pc/test/mock_peer_connection_observers.h" #include "rtc_base/gunit.h" @@ -129,6 +128,9 @@ PeerConnectionTestWrapper::PeerConnectionTestWrapper( PeerConnectionTestWrapper::~PeerConnectionTestWrapper() { RTC_DCHECK_RUN_ON(&pc_thread_checker_); + // To avoid flaky bot failures, make sure fake sources are stopped prior to + // closing the peer connections. See https://crbug.com/webrtc/15018. + StopFakeVideoSources(); // Either network_thread or worker_thread might be active at this point. // Relying on ~PeerConnection to properly wait for them doesn't work, // as a vptr race might occur (before we enter the destruction body). @@ -392,6 +394,7 @@ PeerConnectionTestWrapper::GetUserMedia( auto source = rtc::make_ref_counted( config, /* remote */ false); + fake_video_sources_.push_back(source); std::string videotrack_label = stream_id + kVideoTrackLabelBase; rtc::scoped_refptr video_track( @@ -401,3 +404,10 @@ PeerConnectionTestWrapper::GetUserMedia( } return stream; } + +void PeerConnectionTestWrapper::StopFakeVideoSources() { + for (const auto& fake_video_source : fake_video_sources_) { + fake_video_source->fake_periodic_source().Stop(); + } + fake_video_sources_.clear(); +} diff --git a/pc/test/peer_connection_test_wrapper.h b/pc/test/peer_connection_test_wrapper.h index 23a6996c30..cfc423f81a 100644 --- a/pc/test/peer_connection_test_wrapper.h +++ b/pc/test/peer_connection_test_wrapper.h @@ -29,6 +29,7 @@ #include "api/video/resolution.h" #include "pc/test/fake_audio_capture_module.h" #include "pc/test/fake_periodic_video_source.h" +#include "pc/test/fake_periodic_video_track_source.h" #include "pc/test/fake_video_track_renderer.h" #include "rtc_base/third_party/sigslot/sigslot.h" #include "rtc_base/thread.h" @@ -115,6 +116,7 @@ class PeerConnectionTestWrapper webrtc::Resolution resolution = { .width = webrtc::FakePeriodicVideoSource::kDefaultWidth, .height = webrtc::FakePeriodicVideoSource::kDefaultHeight}); + void StopFakeVideoSources(); private: void SetLocalDescription(webrtc::SdpType type, const std::string& sdp); @@ -136,6 +138,8 @@ class PeerConnectionTestWrapper std::unique_ptr renderer_; int num_get_user_media_calls_ = 0; bool pending_negotiation_; + std::vector> + fake_video_sources_; }; #endif // PC_TEST_PEER_CONNECTION_TEST_WRAPPER_H_