Revert of Improving the fake clock and using it to fix a flaky STUN timeout test. (patchset #10 id:180001 of https://codereview.webrtc.org/2024813004/ )

Reason for revert:
There seems to be a TSan warning that wasn't caught by the trybot: https://build.chromium.org/p/client.webrtc/builders/Linux%20Tsan%20v2/builds/6732/steps/peerconnection_unittests/logs/stdio

Apparently a thread is still alive even after destroying WebRTCSession. Need to think of a way to fix this, without adding a critical section around g_clock (since that would hurt performance).

Original issue's description:
> Improving the fake clock and using it to fix a flaky STUN timeout test.
>
> When the fake clock's time is advanced, it now ensures all pending
> queued messages have been dispatched. This allows us to write a
> "SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up
> until the target time.
>
> Useful in this case, where we know the STUN timeout should take a total
> of 9500ms, but it would be overly complex to write test code that waits
> for each individual timeout, ensures a STUN packet has been
> retransmited, etc.
>
> (The test described above *should* be written, but it belongs in
> p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc).
>
> Committed: https://crrev.com/ffbe0e17e2c9b7fe101023acf40574dc0c95631a
> Cr-Commit-Position: refs/heads/master@{#13043}

TBR=pthatcher@webrtc.org,tommi@webrtc.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true

Review-Url: https://codereview.webrtc.org/2038213002
Cr-Commit-Position: refs/heads/master@{#13045}

webrtc/base/base_tests.gyp

@@ -15,8 +15,6 @@
         'unittest_main.cc',
         # Also use this as a convenient dumping ground for misc files that are
         # included by multiple targets below.
-        'fakeclock.cc',
-        'fakeclock.h',
         'fakenetwork.h',
         'fakesslidentity.h',
         'faketaskrunner.h',
@@ -24,7 +22,6 @@
         'testbase64.h',
         'testechoserver.h',
         'testutils.h',
-        'timedelta.h',
       ],
       'defines': [
         'GTEST_RELATIVE_PATH',

webrtc/base/fakeclock.cc

@@ -27,24 +27,14 @@ void FakeClock::SetTimeNanos(uint64_t nanos) {
     time_ = nanos;
   }
   // If message queues are waiting in a socket select() with a timeout provided
-  // by the OS, they should wake up and dispatch all messages that are ready.
-  MessageQueueManager::ProcessAllMessageQueues();
+  // by the OS, they should wake up to check if there are any messages ready to
+  // be dispatched based on the fake time.
+  MessageQueueManager::WakeAllMessageQueues();
 }
 
 void FakeClock::AdvanceTime(TimeDelta delta) {
-  {
-    CritScope cs(&lock_);
-    time_ += delta.ToNanoseconds();
-  }
-  MessageQueueManager::ProcessAllMessageQueues();
-}
-
-ScopedFakeClock::ScopedFakeClock() {
-  prev_clock_ = SetClockForTesting(this);
-}
-
-ScopedFakeClock::~ScopedFakeClock() {
-  SetClockForTesting(prev_clock_);
+  CritScope cs(&lock_);
+  SetTimeNanos(time_ + delta.ToNanoseconds());
 }
 
 }  // namespace rtc

webrtc/base/fakeclock.h

@@ -19,8 +19,6 @@ namespace rtc {
 
 // Fake clock for use with unit tests, which does not tick on its own.
 // Starts at time 0.
-//
-// TODO(deadbeef): Unify with webrtc::SimulatedClock.
 class FakeClock : public ClockInterface {
  public:
   ~FakeClock() override {}
@@ -40,17 +38,6 @@ class FakeClock : public ClockInterface {
   uint64_t time_ GUARDED_BY(lock_) = 0u;
 };
 
-// Helper class that sets itself as the global clock in its constructor and
-// unsets it in its destructor.
-class ScopedFakeClock : public FakeClock {
- public:
-  ScopedFakeClock();
-  ~ScopedFakeClock() override;
-
- private:
-  ClockInterface* prev_clock_;
-};
-
 }  // namespace rtc
 
 #endif  // WEBRTC_BASE_FAKECLOCK_H_

webrtc/base/gunit.h

@@ -11,7 +11,6 @@
 #ifndef WEBRTC_BASE_GUNIT_H_
 #define WEBRTC_BASE_GUNIT_H_
 
-#include "webrtc/base/fakeclock.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/thread.h"
 #if defined(GTEST_RELATIVE_PATH)
@@ -21,12 +20,10 @@
 #endif
 
 // Wait until "ex" is true, or "timeout" expires.
-#define WAIT(ex, timeout)                               \
-  for (int64_t start = rtc::TimeMillis();               \
-       !(ex) && rtc::TimeMillis() < start + timeout;) { \
-    rtc::Thread::Current()->ProcessMessages(0);         \
-    rtc::Thread::Current()->SleepMs(1);                 \
-  }
+#define WAIT(ex, timeout)                             \
+  for (int64_t start = rtc::TimeMillis();             \
+       !(ex) && rtc::TimeMillis() < start + timeout;) \
+    rtc::Thread::Current()->ProcessMessages(1);
 
 // This returns the result of the test in res, so that we don't re-evaluate
 // the expression in the XXXX_WAIT macros below, since that causes problems
@@ -36,8 +33,7 @@
     int64_t start = rtc::TimeMillis();                    \
     res = (ex);                                           \
     while (!res && rtc::TimeMillis() < start + timeout) { \
-      rtc::Thread::Current()->ProcessMessages(0);         \
-      rtc::Thread::Current()->SleepMs(1);                 \
+      rtc::Thread::Current()->ProcessMessages(1);         \
       res = (ex);                                         \
     }                                                     \
   } while (0)
@@ -89,44 +85,4 @@
     } \
   } while (0)
 
-// Wait until "ex" is true, or "timeout" expires, using fake clock where
-// messages are processed every millisecond.
-#define SIMULATED_WAIT(ex, timeout, clock)                  \
-  for (int64_t start = rtc::TimeMillis();                   \
-       !(ex) && rtc::TimeMillis() < start + timeout;) {     \
-    clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1)); \
-  }
-
-// This returns the result of the test in res, so that we don't re-evaluate
-// the expression in the XXXX_WAIT macros below, since that causes problems
-// when the expression is only true the first time you check it.
-#define SIMULATED_WAIT_(ex, timeout, res, clock)              \
-  do {                                                        \
-    int64_t start = rtc::TimeMillis();                        \
-    res = (ex);                                               \
-    while (!res && rtc::TimeMillis() < start + timeout) {     \
-      clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1)); \
-      res = (ex);                                             \
-    }                                                         \
-  } while (0)
-
-// The typical EXPECT_XXXX, but done until true or a timeout with a fake clock.
-#define EXPECT_TRUE_SIMULATED_WAIT(ex, timeout, clock) \
-  do {                                                 \
-    bool res;                                          \
-    SIMULATED_WAIT_(ex, timeout, res, clock);          \
-    if (!res) {                                        \
-      EXPECT_TRUE(ex);                                 \
-    }                                                  \
-  } while (0)
-
-#define EXPECT_EQ_SIMULATED_WAIT(v1, v2, timeout, clock) \
-  do {                                                   \
-    bool res;                                            \
-    SIMULATED_WAIT_(v1 == v2, timeout, res, clock);      \
-    if (!res) {                                          \
-      EXPECT_EQ(v1, v2);                                 \
-    }                                                    \
-  } while (0)
-
 #endif  // WEBRTC_BASE_GUNIT_H_

webrtc/base/messagequeue.cc

@@ -9,14 +9,17 @@
  */
 #include <algorithm>
 
-#include "webrtc/base/atomicops.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/common.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/messagequeue.h"
-#include "webrtc/base/thread.h"
 #include "webrtc/base/trace_event.h"
 
+namespace {
+
+enum { MSG_WAKE_MESSAGE_QUEUE = 1 };
+}
+
 namespace rtc {
 
 const int kMaxMsgLatency = 150;  // 150 ms
@@ -38,7 +41,8 @@ bool MessageQueueManager::IsInitialized() {
   return instance_ != NULL;
 }
 
-MessageQueueManager::MessageQueueManager() {}
+MessageQueueManager::MessageQueueManager() {
+}
 
 MessageQueueManager::~MessageQueueManager() {
 }
@@ -104,38 +108,28 @@ void MessageQueueManager::ClearInternal(MessageHandler *handler) {
     (*iter)->Clear(handler);
 }
 
-void MessageQueueManager::ProcessAllMessageQueues() {
+void MessageQueueManager::WakeAllMessageQueues() {
   if (!instance_) {
     return;
   }
-  return Instance()->ProcessAllMessageQueuesInternal();
+  return Instance()->WakeAllMessageQueuesInternal();
 }
 
-void MessageQueueManager::ProcessAllMessageQueuesInternal() {
+void MessageQueueManager::WakeAllMessageQueuesInternal() {
 #if CS_DEBUG_CHECKS  // CurrentThreadIsOwner returns true by default.
   ASSERT(!crit_.CurrentThreadIsOwner());  // See note above.
 #endif
-  // Post a delayed message at the current time and wait for it to be dispatched
-  // on all queues, which will ensure that all messages that came before it were
-  // also dispatched.
-  volatile int queues_not_done;
-  auto functor = [&queues_not_done] { AtomicOps::Decrement(&queues_not_done); };
-  FunctorMessageHandler<void, decltype(functor)> handler(functor);
-  {
-    CritScope cs(&crit_);
-    queues_not_done = static_cast<int>(message_queues_.size());
-    for (MessageQueue* queue : message_queues_) {
-      queue->PostDelayed(0, &handler);
-    }
-  }
-  // Note: One of the message queues may have been on this thread, which is why
-  // we can't synchronously wait for queues_not_done to go to 0; we need to
-  // process messages as well.
-  while (AtomicOps::AcquireLoad(&queues_not_done) > 0) {
-    rtc::Thread::Current()->ProcessMessages(0);
+  CritScope cs(&crit_);
+  for (MessageQueue* queue : message_queues_) {
+    // Posting an arbitrary message will force the message queue to wake up.
+    queue->Post(this, MSG_WAKE_MESSAGE_QUEUE);
   }
 }
 
+void MessageQueueManager::OnMessage(Message* pmsg) {
+  RTC_DCHECK(pmsg->message_id == MSG_WAKE_MESSAGE_QUEUE);
+}
+
 //------------------------------------------------------------------
 // MessageQueue
 MessageQueue::MessageQueue(SocketServer* ss, bool init_queue)

webrtc/base/messagequeue.h

@@ -37,7 +37,7 @@ class MessageQueue;
 
 // MessageQueueManager does cleanup of of message queues
 
-class MessageQueueManager {
+class MessageQueueManager : public MessageHandler {
  public:
   static void Add(MessageQueue *message_queue);
   static void Remove(MessageQueue *message_queue);
@@ -50,20 +50,21 @@ class MessageQueueManager {
   static bool IsInitialized();
 
   // Mainly for testing purposes, for use with a simulated clock.
-  // Ensures that all message queues have processed delayed messages
-  // up until the current point in time.
-  static void ProcessAllMessageQueues();
+  // Posts a no-op event on all message queues so they will wake from the
+  // socket server select() and process messages again.
+  static void WakeAllMessageQueues();
 
  private:
   static MessageQueueManager* Instance();
 
   MessageQueueManager();
-  ~MessageQueueManager();
+  ~MessageQueueManager() override;
 
   void AddInternal(MessageQueue *message_queue);
   void RemoveInternal(MessageQueue *message_queue);
   void ClearInternal(MessageHandler *handler);
-  void ProcessAllMessageQueuesInternal();
+  void WakeAllMessageQueuesInternal();
+  void OnMessage(Message* pmsg) override;
 
   static MessageQueueManager* instance_;
   // This list contains all live MessageQueues.

webrtc/base/signalthread_unittest.cc

@@ -140,9 +140,6 @@ class OwnerThread : public Thread, public sigslot::has_slots<> {
 // signal thread is still working.  This may happen
 // when shutting down the process.
 TEST_F(SignalThreadTest, OwnerThreadGoesAway) {
-  // We don't use |thread_| for this test, so destroy it.
-  thread_->Destroy(true);
-
   {
     std::unique_ptr<OwnerThread> owner(new OwnerThread(this));
     main_thread_ = owner.get();

webrtc/base/timeutils.cc

@@ -32,10 +32,8 @@ namespace rtc {
 
 ClockInterface* g_clock = nullptr;
 
-ClockInterface* SetClockForTesting(ClockInterface* clock) {
-  ClockInterface* prev = g_clock;
+void SetClock(ClockInterface* clock) {
   g_clock = clock;
-  return prev;
 }
 
 uint64_t TimeNanos() {

webrtc/base/timeutils.h

@@ -39,10 +39,8 @@ class ClockInterface {
 
 // Sets the global source of time. This is useful mainly for unit tests.
 //
-// Returns the previously set ClockInterface, or nullptr if none is set.
-//
-// Does not transfer ownership of the clock. SetClockForTesting(nullptr)
-// should be called before the ClockInterface is deleted.
+// Does not transfer ownership of the clock.
+// SetClock(nullptr) should be called before the ClockInterface is deleted.
 //
 // This method is not thread-safe; it should only be used when no other thread
 // is running (for example, at the start/end of a unit test, or start/end of
@@ -51,7 +49,7 @@ class ClockInterface {
 // TODO(deadbeef): Instead of having functions that access this global
 // ClockInterface, we may want to pass the ClockInterface into everything
 // that uses it, eliminating the need for a global variable and this function.
-ClockInterface* SetClockForTesting(ClockInterface* clock);
+void SetClock(ClockInterface* clock);
 
 // Returns the current time in milliseconds in 32 bits.
 uint32_t Time32();

webrtc/base/timeutils_unittest.cc

@@ -301,7 +301,7 @@ TEST(TimeDelta, NumericOperators) {
 // fake clock when it's set.
 TEST(FakeClock, TimeFunctionsUseFakeClock) {
   FakeClock clock;
-  SetClockForTesting(&clock);
+  SetClock(&clock);
 
   clock.SetTimeNanos(987654321u);
   EXPECT_EQ(987u, Time32());
@@ -310,7 +310,7 @@ TEST(FakeClock, TimeFunctionsUseFakeClock) {
   EXPECT_EQ(987654321u, TimeNanos());
   EXPECT_EQ(1000u, TimeAfter(13));
 
-  SetClockForTesting(nullptr);
+  SetClock(nullptr);
   // After it's unset, we should get a normal time.
   EXPECT_NE(987, TimeMillis());
 }
@@ -348,7 +348,7 @@ TEST(FakeClock, SettingTimeWakesThreads) {
   int64_t real_start_time_ms = TimeMillis();
 
   FakeClock clock;
-  SetClockForTesting(&clock);
+  SetClock(&clock);
 
   Thread worker;
   worker.Start();
@@ -359,25 +359,24 @@ TEST(FakeClock, SettingTimeWakesThreads) {
     message_handler_dispatched.Set();
   };
   FunctorMessageHandler<void, decltype(functor)> handler(functor);
-  worker.PostDelayed(60000, &handler);
+  worker.PostDelayed(10000, &handler);
 
   // Wait for a bit for the worker thread to be started and enter its socket
-  // select(). Otherwise this test would be trivial since the worker thread
-  // would process the event as soon as it was started.
+  // select().
   Thread::Current()->SleepMs(1000);
 
   // Advance the fake clock, expecting the worker thread to wake up
-  // and dispatch the message instantly.
-  clock.AdvanceTime(TimeDelta::FromSeconds(60u));
-  EXPECT_TRUE(message_handler_dispatched.Wait(0));
+  // and dispatch the message quickly.
+  clock.AdvanceTime(TimeDelta::FromSeconds(10u));
+  message_handler_dispatched.Wait(Event::kForever);
   worker.Stop();
 
-  SetClockForTesting(nullptr);
+  SetClock(nullptr);
 
-  // The message should have been dispatched long before the 60 seconds fully
-  // elapsed (just a sanity check).
+  // The message should have been dispatched long before the 10 seconds fully
+  // elapsed.
   int64_t real_end_time_ms = TimeMillis();
-  EXPECT_LT(real_end_time_ms - real_start_time_ms, 10000);
+  EXPECT_LT(real_end_time_ms - real_start_time_ms, 2000);
 }
 
 }  // namespace rtc