Fix race condition around rtc::ScopedFakeClock.

We make sure the fake clock is constructed first thing,
so that all subsequent calls to GetClockForTesting() are
consistent and non-racy.

This proper scoping also allows to remove some explicit
destructions which are no longer necessary.

Bug: webrtc:11282
Change-Id: Id9263617c2e2b025b17d9bcb9cd415d651405a8a
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/166043
Commit-Queue: Yves Gerey <yvesg@google.com>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#30309}

pc/peer_connection_integrationtest.cc

@@ -1711,6 +1711,30 @@ class PeerConnectionIntegrationTest
       : PeerConnectionIntegrationBaseTest(GetParam()) {}
 };
 
+// Fake clock must be set before threads are started to prevent race on
+// Set/GetClockForTesting().
+// To achieve that, multiple inheritance is used as a mixin pattern
+// where order of construction is finely controlled.
+// This also ensures peerconnection is closed before switching back to non-fake
+// clock, avoiding other races and DCHECK failures such as in rtp_sender.cc.
+class FakeClockForTest : public rtc::ScopedFakeClock {
+ protected:
+  FakeClockForTest() {
+    // Some things use a time of "0" as a special value, so we need to start out
+    // the fake clock at a nonzero time.
+    // TODO(deadbeef): Fix this.
+    AdvanceTime(webrtc::TimeDelta::seconds(1));
+  }
+
+  // Explicit handle.
+  ScopedFakeClock& FakeClock() { return *this; }
+};
+
+// Ensure FakeClockForTest is constructed first (see class for rationale).
+class PeerConnectionIntegrationTestWithFakeClock
+    : public FakeClockForTest,
+      public PeerConnectionIntegrationTest {};
+
 class PeerConnectionIntegrationTestPlanB
     : public PeerConnectionIntegrationBaseTest {
  protected:
@@ -3257,15 +3281,11 @@ TEST_P(PeerConnectionIntegrationTest,
 // transport has detected that a channel is writable and thus data can be
 // received before the data channel state changes to open. That is hard to test
 // but the same buffering is expected to be used in that case.
-TEST_P(PeerConnectionIntegrationTest,
+//
+// Use fake clock and simulated network delay so that we predictably can wait
+// until an SCTP message has been delivered without "sleep()"ing.
+TEST_P(PeerConnectionIntegrationTestWithFakeClock,
        DataBufferedUntilRtpDataChannelObserverRegistered) {
-  // Use fake clock and simulated network delay so that we predictably can wait
-  // until an SCTP message has been delivered without "sleep()"ing.
-  rtc::ScopedFakeClock fake_clock;
-  // Some things use a time of "0" as a special value, so we need to start out
-  // the fake clock at a nonzero time.
-  // TODO(deadbeef): Fix this.
-  fake_clock.AdvanceTime(webrtc::TimeDelta::seconds(1));
   virtual_socket_server()->set_delay_mean(5);  // 5 ms per hop.
   virtual_socket_server()->UpdateDelayDistribution();
 
@@ -3278,30 +3298,26 @@ TEST_P(PeerConnectionIntegrationTest,
   caller()->CreateAndSetAndSignalOffer();
   ASSERT_TRUE(caller()->data_channel() != nullptr);
   ASSERT_TRUE_SIMULATED_WAIT(callee()->data_channel() != nullptr,
-                             kDefaultTimeout, fake_clock);
+                             kDefaultTimeout, FakeClock());
   ASSERT_TRUE_SIMULATED_WAIT(caller()->data_observer()->IsOpen(),
-                             kDefaultTimeout, fake_clock);
+                             kDefaultTimeout, FakeClock());
   ASSERT_EQ_SIMULATED_WAIT(DataChannelInterface::kOpen,
                            callee()->data_channel()->state(), kDefaultTimeout,
-                           fake_clock);
+                           FakeClock());
 
   // Unregister the observer which is normally automatically registered.
   callee()->data_channel()->UnregisterObserver();
   // Send data and advance fake clock until it should have been received.
   std::string data = "hello world";
   caller()->data_channel()->Send(DataBuffer(data));
-  SIMULATED_WAIT(false, 50, fake_clock);
+  SIMULATED_WAIT(false, 50, FakeClock());
 
   // Attach data channel and expect data to be received immediately. Note that
   // EXPECT_EQ_WAIT is used, such that the simulated clock is not advanced any
   // further, but data can be received even if the callback is asynchronous.
   MockDataChannelObserver new_observer(callee()->data_channel());
   EXPECT_EQ_SIMULATED_WAIT(data, new_observer.last_message(), kDefaultTimeout,
-                           fake_clock);
+                           FakeClock());
-  // 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.
-  ClosePeerConnections();
 }
 
 // This test sets up a call between two parties with audio, video and but only
@@ -4432,16 +4448,16 @@ class PeerConnectionIntegrationIceStatesTest
   std::unique_ptr<cricket::TestStunServer> stun_server_;
 };
 
+// Ensure FakeClockForTest is constructed first (see class for rationale).
+class PeerConnectionIntegrationIceStatesTestWithFakeClock
+    : public FakeClockForTest,
+      public PeerConnectionIntegrationIceStatesTest {};
+
 // Tests that the PeerConnection goes through all the ICE gathering/connection
 // states over the duration of the call. This includes Disconnected and Failed
 // states, induced by putting a firewall between the peers and waiting for them
 // to time out.
-TEST_P(PeerConnectionIntegrationIceStatesTest, VerifyIceStates) {
+TEST_P(PeerConnectionIntegrationIceStatesTestWithFakeClock, VerifyIceStates) {
-  rtc::ScopedFakeClock fake_clock;
-  // Some things use a time of "0" as a special value, so we need to start out
-  // the fake clock at a nonzero time.
-  fake_clock.AdvanceTime(TimeDelta::seconds(1));
-
   const SocketAddress kStunServerAddress =
       SocketAddress("99.99.99.1", cricket::STUN_SERVER_PORT);
   StartStunServer(kStunServerAddress);
@@ -4476,10 +4492,10 @@ TEST_P(PeerConnectionIntegrationIceStatesTest, VerifyIceStates) {
 
   ASSERT_EQ_SIMULATED_WAIT(PeerConnectionInterface::kIceConnectionCompleted,
                            caller()->ice_connection_state(), kDefaultTimeout,
-                           fake_clock);
+                           FakeClock());
   ASSERT_EQ_SIMULATED_WAIT(PeerConnectionInterface::kIceConnectionCompleted,
                            caller()->standardized_ice_connection_state(),
-                           kDefaultTimeout, fake_clock);
+                           kDefaultTimeout, FakeClock());
 
   // Verify that the observer was notified of the intermediate transitions.
   EXPECT_THAT(caller()->ice_connection_state_history(),
@@ -4506,20 +4522,20 @@ TEST_P(PeerConnectionIntegrationIceStatesTest, VerifyIceStates) {
   RTC_LOG(LS_INFO) << "Firewall rules applied";
   ASSERT_EQ_SIMULATED_WAIT(PeerConnectionInterface::kIceConnectionDisconnected,
                            caller()->ice_connection_state(), kDefaultTimeout,
-                           fake_clock);
+                           FakeClock());
   ASSERT_EQ_SIMULATED_WAIT(PeerConnectionInterface::kIceConnectionDisconnected,
                            caller()->standardized_ice_connection_state(),
-                           kDefaultTimeout, fake_clock);
+                           kDefaultTimeout, FakeClock());
 
   // Let ICE re-establish by removing the firewall rules.
   firewall()->ClearRules();
   RTC_LOG(LS_INFO) << "Firewall rules cleared";
   ASSERT_EQ_SIMULATED_WAIT(PeerConnectionInterface::kIceConnectionCompleted,
                            caller()->ice_connection_state(), kDefaultTimeout,
-                           fake_clock);
+                           FakeClock());
   ASSERT_EQ_SIMULATED_WAIT(PeerConnectionInterface::kIceConnectionCompleted,
                            caller()->standardized_ice_connection_state(),
-                           kDefaultTimeout, fake_clock);
+                           kDefaultTimeout, FakeClock());
 
   // According to RFC7675, if there is no response within 30 seconds then the
   // peer should consider the other side to have rejected the connection. This
@@ -4531,26 +4547,16 @@ TEST_P(PeerConnectionIntegrationIceStatesTest, VerifyIceStates) {
   RTC_LOG(LS_INFO) << "Firewall rules applied again";
   ASSERT_EQ_SIMULATED_WAIT(PeerConnectionInterface::kIceConnectionFailed,
                            caller()->ice_connection_state(), kConsentTimeout,
-                           fake_clock);
+                           FakeClock());
   ASSERT_EQ_SIMULATED_WAIT(PeerConnectionInterface::kIceConnectionFailed,
                            caller()->standardized_ice_connection_state(),
-                           kConsentTimeout, fake_clock);
+                           kConsentTimeout, FakeClock());
-
-  // We need to manually close the peerconnections before the fake clock goes
-  // out of scope, or we trigger a DCHECK in rtp_sender.cc when we briefly
-  // return to using non-faked time.
-  delete SetCallerPcWrapperAndReturnCurrent(nullptr);
-  delete SetCalleePcWrapperAndReturnCurrent(nullptr);
 }
 
 // Tests that if the connection doesn't get set up properly we eventually reach
 // the "failed" iceConnectionState.
-TEST_P(PeerConnectionIntegrationIceStatesTest, IceStateSetupFailure) {
+TEST_P(PeerConnectionIntegrationIceStatesTestWithFakeClock,
-  rtc::ScopedFakeClock fake_clock;
+       IceStateSetupFailure) {
-  // Some things use a time of "0" as a special value, so we need to start out
-  // the fake clock at a nonzero time.
-  fake_clock.AdvanceTime(TimeDelta::seconds(1));
-
   // Block connections to/from the caller and wait for ICE to become
   // disconnected.
   for (const auto& caller_address : CallerAddresses()) {
@@ -4570,13 +4576,7 @@ TEST_P(PeerConnectionIntegrationIceStatesTest, IceStateSetupFailure) {
   constexpr int kConsentTimeout = 30000;
   ASSERT_EQ_SIMULATED_WAIT(PeerConnectionInterface::kIceConnectionFailed,
                            caller()->standardized_ice_connection_state(),
-                           kConsentTimeout, fake_clock);
+                           kConsentTimeout, FakeClock());
-
-  // We need to manually close the peerconnections before the fake clock goes
-  // out of scope, or we trigger a DCHECK in rtp_sender.cc when we briefly
-  // return to using non-faked time.
-  delete SetCallerPcWrapperAndReturnCurrent(nullptr);
-  delete SetCalleePcWrapperAndReturnCurrent(nullptr);
 }
 
 // Tests that the best connection is set to the appropriate IPv4/IPv6 connection
@@ -4636,6 +4636,14 @@ INSTANTIATE_TEST_SUITE_P(
                    std::make_pair("IPv6 no STUN", kFlagsIPv6NoStun),
                    std::make_pair("IPv4 with STUN", kFlagsIPv4Stun))));
 
+INSTANTIATE_TEST_SUITE_P(
+    PeerConnectionIntegrationTest,
+    PeerConnectionIntegrationIceStatesTestWithFakeClock,
+    Combine(Values(SdpSemantics::kPlanB, SdpSemantics::kUnifiedPlan),
+            Values(std::make_pair("IPv4 no STUN", kFlagsIPv4NoStun),
+                   std::make_pair("IPv6 no STUN", kFlagsIPv6NoStun),
+                   std::make_pair("IPv4 with STUN", kFlagsIPv4Stun))));
+
 // This test sets up a call between two parties with audio and video.
 // During the call, the caller restarts ICE and the test verifies that
 // new ICE candidates are generated and audio and video still can flow, and the
@@ -4938,13 +4946,8 @@ TEST_F(PeerConnectionIntegrationTestPlanB, CanSendRemoteVideoTrack) {
 // 2. 9 media hops: Rest of the DTLS handshake. 3 hops in each direction when
 //                  using TURN<->TURN pair, and DTLS exchange is 4 packets,
 //                  the first of which should have arrived before the answer.
-TEST_P(PeerConnectionIntegrationTest, EndToEndConnectionTimeWithTurnTurnPair) {
+TEST_P(PeerConnectionIntegrationTestWithFakeClock,
-  rtc::ScopedFakeClock fake_clock;
+       EndToEndConnectionTimeWithTurnTurnPair) {
-  // Some things use a time of "0" as a special value, so we need to start out
-  // the fake clock at a nonzero time.
-  // TODO(deadbeef): Fix this.
-  fake_clock.AdvanceTime(webrtc::TimeDelta::seconds(1));
-
   static constexpr int media_hop_delay_ms = 50;
   static constexpr int signaling_trip_delay_ms = 500;
   // For explanation of these values, see comment above.
@@ -5013,7 +5016,7 @@ TEST_P(PeerConnectionIntegrationTest, EndToEndConnectionTimeWithTurnTurnPair) {
   caller()->SetOfferAnswerOptions(options);
   caller()->CreateAndSetAndSignalOffer();
   EXPECT_TRUE_SIMULATED_WAIT(DtlsConnected(), total_connection_time_ms,
-                             fake_clock);
+                             FakeClock());
   // 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.
@@ -5783,6 +5786,11 @@ INSTANTIATE_TEST_SUITE_P(PeerConnectionIntegrationTest,
                          Values(SdpSemantics::kPlanB,
                                 SdpSemantics::kUnifiedPlan));
 
+INSTANTIATE_TEST_SUITE_P(PeerConnectionIntegrationTest,
+                         PeerConnectionIntegrationTestWithFakeClock,
+                         Values(SdpSemantics::kPlanB,
+                                SdpSemantics::kUnifiedPlan));
+
 // Tests that verify interoperability between Plan B and Unified Plan
 // PeerConnections.
 class PeerConnectionIntegrationInteropTest