From d91969ed79f1eb820ab854dc6a36068afad74232 Mon Sep 17 00:00:00 2001 From: Steve Anton Date: Thu, 30 May 2019 12:27:03 -0700 Subject: [PATCH] Explicitly close PeerConnections when using ScopedFieldTrials Since ScopedFieldTrials is not thread safe, if it goes out of scope while other threads are still running and possibly querying the field trials then it's possible to hit an MSAN failure. Bug: webrtc:10694 Change-Id: I93c94f1008e4478d98ec1545bbc0a7536739e479 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/139460 Reviewed-by: Qingsi Wang Commit-Queue: Steve Anton Cr-Commit-Position: refs/heads/master@{#28116} --- pc/peer_connection_integrationtest.cc | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index 0fc2e152c3..7b3163ba0e 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -1580,6 +1580,11 @@ class PeerConnectionIntegrationBaseTest : public ::testing::Test { return expectations_correct; } + void ClosePeerConnections() { + caller()->pc()->Close(); + callee()->pc()->Close(); + } + void TestNegotiatedCipherSuite( const PeerConnectionFactory::Options& caller_options, const PeerConnectionFactory::Options& callee_options, @@ -3207,8 +3212,7 @@ TEST_P(PeerConnectionIntegrationTest, // Closing the PeerConnections destroys the ports before the ScopedFakeClock. // If this is not done a DCHECK can be hit in ports.cc, because a large // negative number is calculated for the rtt due to the global clock changing. - caller()->pc()->Close(); - callee()->pc()->Close(); + ClosePeerConnections(); } // This test sets up a call between two parties with audio, video and but only @@ -4498,8 +4502,7 @@ TEST_P(PeerConnectionIntegrationTest, EndToEndConnectionTimeWithTurnTurnPair) { // Closing the PeerConnections destroys the ports before the ScopedFakeClock. // If this is not done a DCHECK can be hit in ports.cc, because a large // negative number is calculated for the rtt due to the global clock changing. - caller()->pc()->Close(); - callee()->pc()->Close(); + ClosePeerConnections(); } // Verify that a TurnCustomizer passed in through RTCConfiguration @@ -5020,8 +5023,7 @@ TEST_P(PeerConnectionIntegrationTest, ClosingConnectionStopsPacketFlow) { media_expectations.CalleeExpectsSomeAudioAndVideo(); ASSERT_TRUE(ExpectNewFrames(media_expectations)); // Close PeerConnections. - caller()->pc()->Close(); - callee()->pc()->Close(); + ClosePeerConnections(); // Pump messages for a second, and ensure no new packets end up sent. uint32_t sent_packets_a = virtual_socket_server()->sent_packets(); WAIT(false, 1000); @@ -5147,6 +5149,9 @@ TEST_P(PeerConnectionIntegrationTest, RegatherAfterChangingIceTransportType) { callee()->pc()->SetConfiguration(callee_config); EXPECT_EQ_WAIT(cricket::LOCAL_PORT_TYPE, callee()->last_candidate_gathered().type(), kDefaultTimeout); + + // PeerConnections must be closed before ScopedFieldTrials goes out of scope. + ClosePeerConnections(); } INSTANTIATE_TEST_SUITE_P(PeerConnectionIntegrationTest,