[WaitableEvent] Remove TimedWaitUntil

It only had two callers and it'll become error prone once
ScopedTaskEnvironment's MOCK_TIME mode always mocks TimeTicks::Now().
TimeDelta based TimedWait()'s will remain real-time-all-the-time but
TimeWaitUntil() could be passed
TimeTicks::Now() + TimeDelta::FromSeconds(1) but wait forever or return
right away because Now() is mocked and is effectively a random value
compared to system time.

Context for MOCK_TIME : https://chromium-review.googlesource.com/c/chromium/src/+/1707870

This also enabled an opportunity to clean up the implementations
to invoke TimeTicks::Now() even less than before. All system calls
for waiting are based on delta. So we always use the initial
|wait_delta| and we only need to lookup Now() if the API can
spuriously return too early and |wait_delta| isn't Max().

R=kylechar@chromium.org

Bug: 905412
Change-Id: Ic73ee7ea286d023d2adee624478ff650f94767a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1708841Reviewed-by: kylechar <kylechar@chromium.org>
Reviewed-by: Joe Mason <joenotcharles@google.com>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Auto-Submit: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#682232}

base/synchronization/waitable_event.h

@@ -34,7 +34,6 @@
 namespace base {
 
 class TimeDelta;
-class TimeTicks;
 
 // A WaitableEvent can be a useful thread synchronization tool when you want to
 // allow one thread to wait for another thread to finish some work. For
@@ -98,17 +97,12 @@ class BASE_EXPORT WaitableEvent {
   void Wait();
 
   // Wait up until wait_delta has passed for the event to be signaled.  Returns
-  // true if the event was signaled.
+  // true if the event was signaled. Handles spurious wakeups and guarantees
+  // that |wait_delta| will have elapsed if this returns false.
   //
   // TimedWait can synchronise its own destruction like |Wait|.
   bool TimedWait(const TimeDelta& wait_delta);
 
-  // Wait up until end_time deadline has passed for the event to be signaled.
-  // Return true if the event was signaled.
-  //
-  // TimedWaitUntil can synchronise its own destruction like |Wait|.
-  bool TimedWaitUntil(const TimeTicks& end_time);
-
 #if defined(OS_WIN)
   HANDLE handle() const { return handle_.Get(); }
 #endif

base/synchronization/waitable_event_mac.cc

@@ -18,6 +18,7 @@
 #include "base/posix/eintr_wrapper.h"
 #include "base/threading/scoped_blocking_call.h"
 #include "base/threading/thread_restrictions.h"
+#include "base/time/time.h"
 #include "build/build_config.h"
 
 namespace base {
@@ -103,15 +104,14 @@ bool WaitableEvent::IsSignaled() {
 }
 
 void WaitableEvent::Wait() {
-  bool result = TimedWaitUntil(TimeTicks::Max());
+  bool result = TimedWait(TimeDelta::Max());
   DCHECK(result) << "TimedWait() should never fail with infinite timeout";
 }
 
 bool WaitableEvent::TimedWait(const TimeDelta& wait_delta) {
-  return TimedWaitUntil(TimeTicks::Now() + wait_delta);
-}
+  if (wait_delta <= TimeDelta())
+    return IsSignaled();
 
-bool WaitableEvent::TimedWaitUntil(const TimeTicks& end_time) {
   // Record the event that this thread is blocking upon (for hang diagnosis) and
   // consider blocked for scheduling purposes. Ignore this for non-blocking
   // WaitableEvents.
@@ -128,7 +128,7 @@ bool WaitableEvent::TimedWaitUntil(const TimeTicks& end_time) {
 
   mach_msg_option_t options = MACH_RCV_MSG;
 
-  if (!end_time.is_max())
+  if (!wait_delta.is_max())
     options |= MACH_RCV_TIMEOUT | MACH_RCV_INTERRUPT;
 
   mach_msg_size_t rcv_size = sizeof(msg);
@@ -139,21 +139,32 @@ bool WaitableEvent::TimedWaitUntil(const TimeTicks& end_time) {
     rcv_size = 0;
   }
 
-  kern_return_t kr;
-  mach_msg_timeout_t timeout = MACH_MSG_TIMEOUT_NONE;
-  do {
-    if (!end_time.is_max()) {
-      timeout = std::max<int64_t>(
-          0, (end_time - TimeTicks::Now()).InMillisecondsRoundedUp());
-    }
+  // TimeTicks takes care of overflow but we special case is_max() nonetheless
+  // to avoid invoking Now() unnecessarily (same for the increment step of the
+  // for loop if the condition variable returns early).
+  // Ref: https://crbug.com/910524#c7
+  const TimeTicks end_time =
+      wait_delta.is_max() ? TimeTicks::Max() : TimeTicks::Now() + wait_delta;
+  // Fake |kr| value to boostrap the for loop.
+  kern_return_t kr = MACH_RCV_INTERRUPTED;
+  for (mach_msg_timeout_t timeout = wait_delta.is_max()
+                                        ? MACH_MSG_TIMEOUT_NONE
+                                        : wait_delta.InMillisecondsRoundedUp();
+       // If the thread is interrupted during mach_msg(), the system call will
+       // be restarted. However, the libsyscall wrapper does not adjust the
+       // timeout by the amount of time already waited. Using MACH_RCV_INTERRUPT
+       // will instead return from mach_msg(), so that the call can be retried
+       // with an adjusted timeout.
+       kr == MACH_RCV_INTERRUPTED;
+       timeout =
+           end_time.is_max()
+               ? MACH_MSG_TIMEOUT_NONE
+               : std::max<int64_t>(
+                     0,
+                     (end_time - TimeTicks::Now()).InMillisecondsRoundedUp())) {
     kr = mach_msg(&msg.header, options, 0, rcv_size, receive_right_->Name(),
                   timeout, MACH_PORT_NULL);
-    // If the thread is interrupted during mach_msg(), the system call
-    // will be restarted. However, the libsyscall wrapper does not adjust
-    // the timeout by the amount of time already waited.
-    // Using MACH_RCV_INTERRUPT will instead return from mach_msg(),
-    // so that the call can be retried with an adjusted timeout.
-  } while (kr == MACH_RCV_INTERRUPTED);
+  }
 
   if (kr == KERN_SUCCESS) {
     return true;

base/synchronization/waitable_event_perftest.cc

@@ -37,7 +37,7 @@ class TraceWaitableEvent {
 
   bool TimedWaitUntil(const TimeTicks& end_time) {
     ElapsedTimer timer;
-    const bool signaled = event_.TimedWaitUntil(end_time);
+    const bool signaled = event_.TimedWait(end_time - timer.Begin());
     total_wait_time_ += timer.Elapsed();
     ++wait_samples_;
     return signaled;

base/synchronization/waitable_event_posix.cc

@@ -16,6 +16,7 @@
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/scoped_blocking_call.h"
 #include "base/threading/thread_restrictions.h"
+#include "base/time/time.h"
 
 // -----------------------------------------------------------------------------
 // A WaitableEvent on POSIX is implemented as a wait-list. Currently we don't
@@ -152,17 +153,14 @@ class SyncWaiter : public WaitableEvent::Waiter {
 };
 
 void WaitableEvent::Wait() {
-  bool result = TimedWaitUntil(TimeTicks::Max());
+  bool result = TimedWait(TimeDelta::Max());
   DCHECK(result) << "TimedWait() should never fail with infinite timeout";
 }
 
 bool WaitableEvent::TimedWait(const TimeDelta& wait_delta) {
-  // TimeTicks takes care of overflow including the cases when wait_delta
-  // is a maximum value.
-  return TimedWaitUntil(TimeTicks::Now() + wait_delta);
-}
+  if (wait_delta <= TimeDelta())
+    return IsSignaled();
 
-bool WaitableEvent::TimedWaitUntil(const TimeTicks& end_time) {
   // Record the event that this thread is blocking upon (for hang diagnosis) and
   // consider it blocked for scheduling purposes. Ignore this for non-blocking
   // WaitableEvents.
@@ -174,8 +172,6 @@ bool WaitableEvent::TimedWaitUntil(const TimeTicks& end_time) {
     scoped_blocking_call.emplace(BlockingType::MAY_BLOCK);
   }
 
-  const bool finite_time = !end_time.is_max();
-
   kernel_->lock_.Acquire();
   if (kernel_->signaled_) {
     if (!kernel_->manual_reset_) {
@@ -196,45 +192,45 @@ bool WaitableEvent::TimedWaitUntil(const TimeTicks& end_time) {
   Enqueue(&sw);
   kernel_->lock_.Release();
   // We are violating locking order here by holding the SyncWaiter lock but not
-  // the WaitableEvent lock. However, this is safe because we don't lock @lock_
+  // the WaitableEvent lock. However, this is safe because we don't lock |lock_|
   // again before unlocking it.
 
-  for (;;) {
-    // Only sample Now() if waiting for a |finite_time|.
-    Optional<TimeTicks> current_time;
-    if (finite_time)
-      current_time = TimeTicks::Now();
-
-    if (sw.fired() || (finite_time && *current_time >= end_time)) {
-      const bool return_value = sw.fired();
-
-      // We can't acquire @lock_ before releasing the SyncWaiter lock (because
-      // of locking order), however, in between the two a signal could be fired
-      // and @sw would accept it, however we will still return false, so the
-      // signal would be lost on an auto-reset WaitableEvent. Thus we call
-      // Disable which makes sw::Fire return false.
-      sw.Disable();
-      sw.lock()->Release();
-
-      // This is a bug that has been enshrined in the interface of
-      // WaitableEvent now: |Dequeue| is called even when |sw.fired()| is true,
-      // even though it'll always return false in that case. However, taking
-      // the lock ensures that |Signal| has completed before we return and
-      // means that a WaitableEvent can synchronise its own destruction.
-      kernel_->lock_.Acquire();
-      kernel_->Dequeue(&sw, &sw);
-      kernel_->lock_.Release();
-
-      return return_value;
-    }
-
-    if (finite_time) {
-      const TimeDelta max_wait(end_time - *current_time);
-      sw.cv()->TimedWait(max_wait);
-    } else {
+  // TimeTicks takes care of overflow but we special case is_max() nonetheless
+  // to avoid invoking Now() unnecessarily (same for the increment step of the
+  // for loop if the condition variable returns early).
+  // Ref: https://crbug.com/910524#c7
+  const TimeTicks end_time =
+      wait_delta.is_max() ? TimeTicks::Max() : TimeTicks::Now() + wait_delta;
+  for (TimeDelta remaining = wait_delta; remaining > TimeDelta() && !sw.fired();
+       remaining = end_time.is_max() ? TimeDelta::Max()
+                                     : end_time - TimeTicks::Now()) {
+    if (end_time.is_max())
       sw.cv()->Wait();
-    }
+    else
+      sw.cv()->TimedWait(remaining);
   }
+
+  // Get the SyncWaiter signaled state before releasing the lock.
+  const bool return_value = sw.fired();
+
+  // We can't acquire |lock_| before releasing the SyncWaiter lock (because of
+  // locking order), however, in between the two a signal could be fired and
+  // |sw| would accept it, however we will still return false, so the signal
+  // would be lost on an auto-reset WaitableEvent. Thus we call Disable which
+  // makes sw::Fire return false.
+  sw.Disable();
+  sw.lock()->Release();
+
+  // This is a bug that has been enshrined in the interface of WaitableEvent
+  // now: |Dequeue| is called even when |sw.fired()| is true, even though it'll
+  // always return false in that case. However, taking the lock ensures that
+  // |Signal| has completed before we return and means that a WaitableEvent can
+  // synchronise its own destruction.
+  kernel_->lock_.Acquire();
+  kernel_->Dequeue(&sw, &sw);
+  kernel_->lock_.Release();
+
+  return return_value;
 }
 
 // -----------------------------------------------------------------------------

base/synchronization/waitable_event_unittest.cc

@@ -235,40 +235,31 @@ TEST(WaitableEventTest, SubMsTimedWait) {
   EXPECT_GE(TimeTicks::Now() - start_time, delay);
 }
 
-// Tests that TimedWaitUntil can be safely used with various end_time deadline
-// values.
-TEST(WaitableEventTest, TimedWaitUntil) {
-  WaitableEvent ev(WaitableEvent::ResetPolicy::AUTOMATIC,
-                   WaitableEvent::InitialState::NOT_SIGNALED);
-
-  TimeTicks start_time(TimeTicks::Now());
-  TimeDelta delay = TimeDelta::FromMilliseconds(10);
-
-  // Should be OK to wait for the current time or time in the past.
-  // That should end promptly and be equivalent to IsSignalled.
-  EXPECT_FALSE(ev.TimedWaitUntil(start_time));
-  EXPECT_FALSE(ev.TimedWaitUntil(start_time - delay));
-
-  // Should be OK to wait for zero TimeTicks().
-  EXPECT_FALSE(ev.TimedWaitUntil(TimeTicks()));
-
-  // Waiting for a time in the future shouldn't end before the deadline
-  // if the event isn't signalled.
-  EXPECT_FALSE(ev.TimedWaitUntil(start_time + delay));
-  EXPECT_GE(TimeTicks::Now() - start_time, delay);
+// Tests that timeouts of zero return immediately (true if already signaled,
+// false otherwise).
+TEST(WaitableEventTest, ZeroTimeout) {
+  WaitableEvent ev;
+  TimeTicks start_time = TimeTicks::Now();
+  EXPECT_FALSE(ev.TimedWait(TimeDelta()));
+  EXPECT_LT(TimeTicks::Now() - start_time, TimeDelta::FromMilliseconds(1));
 
-  // Test that passing TimeTicks::Max to TimedWaitUntil is valid and isn't
-  // the same as passing TimeTicks(). Also verifies that signaling event
-  // ends the wait promptly.
-  WaitableEventSignaler signaler(delay, &ev);
-  PlatformThreadHandle thread;
+  ev.Signal();
   start_time = TimeTicks::Now();
-  PlatformThread::Create(0, &signaler, &thread);
+  EXPECT_TRUE(ev.TimedWait(TimeDelta()));
+  EXPECT_LT(TimeTicks::Now() - start_time, TimeDelta::FromMilliseconds(1));
+}
 
-  EXPECT_TRUE(ev.TimedWaitUntil(TimeTicks::Max()));
-  EXPECT_GE(TimeTicks::Now() - start_time, delay);
+// Same as ZeroTimeout for negative timeouts.
+TEST(WaitableEventTest, NegativeTimeout) {
+  WaitableEvent ev;
+  TimeTicks start_time = TimeTicks::Now();
+  EXPECT_FALSE(ev.TimedWait(TimeDelta::FromMilliseconds(-10)));
+  EXPECT_LT(TimeTicks::Now() - start_time, TimeDelta::FromMilliseconds(1));
 
-  PlatformThread::Join(thread);
+  ev.Signal();
+  start_time = TimeTicks::Now();
+  EXPECT_TRUE(ev.TimedWait(TimeDelta::FromMilliseconds(-10)));
+  EXPECT_LT(TimeTicks::Now() - start_time, TimeDelta::FromMilliseconds(1));
 }
 
 }  // namespace base

base/synchronization/waitable_event_win.cc

@@ -72,44 +72,8 @@ void WaitableEvent::Wait() {
   DCHECK_EQ(WAIT_OBJECT_0, result);
 }
 
-namespace {
-
-// Helper function called from TimedWait and TimedWaitUntil.
-bool WaitUntil(HANDLE handle, const TimeTicks& now, const TimeTicks& end_time) {
-  TimeDelta delta = end_time - now;
-  DCHECK_GT(delta, TimeDelta());
-
-  do {
-    // On Windows, waiting for less than 1 ms results in WaitForSingleObject
-    // returning promptly which may result in the caller code spinning.
-    // We need to ensure that we specify at least the minimally possible 1 ms
-    // delay unless the initial timeout was exactly zero.
-    delta = std::max(delta, TimeDelta::FromMilliseconds(1));
-    // Truncate the timeout to milliseconds.
-    DWORD timeout_ms = saturated_cast<DWORD>(delta.InMilliseconds());
-    DWORD result = WaitForSingleObject(handle, timeout_ms);
-    DCHECK(result == WAIT_OBJECT_0 || result == WAIT_TIMEOUT)
-        << "Unexpected WaitForSingleObject result " << result;
-    switch (result) {
-      case WAIT_OBJECT_0:
-        return true;
-      case WAIT_TIMEOUT:
-        // TimedWait can time out earlier than the specified |timeout| on
-        // Windows. To make this consistent with the posix implementation we
-        // should guarantee that TimedWait doesn't return earlier than the
-        // specified |max_time| and wait again for the remaining time.
-        delta = end_time - TimeTicks::Now();
-        break;
-    }
-  } while (delta > TimeDelta());
-  return false;
-}
-
-}  // namespace
-
 bool WaitableEvent::TimedWait(const TimeDelta& wait_delta) {
-  DCHECK_GE(wait_delta, TimeDelta());
-  if (wait_delta.is_zero())
+  if (wait_delta <= TimeDelta())
     return IsSignaled();
 
   // Record the event that this thread is blocking upon (for hang diagnosis) and
@@ -123,32 +87,36 @@ bool WaitableEvent::TimedWait(const TimeDelta& wait_delta) {
     scoped_blocking_call.emplace(BlockingType::MAY_BLOCK);
   }
 
-  TimeTicks now(TimeTicks::Now());
-  // TimeTicks takes care of overflow including the cases when wait_delta
-  // is a maximum value.
-  return WaitUntil(handle_.Get(), now, now + wait_delta);
-}
-
-bool WaitableEvent::TimedWaitUntil(const TimeTicks& end_time) {
-  if (end_time.is_null())
-    return IsSignaled();
-
-  // Record the event that this thread is blocking upon (for hang diagnosis) and
-  // consider it blocked for scheduling purposes. Ignore this for non-blocking
-  // WaitableEvents.
-  Optional<debug::ScopedEventWaitActivity> event_activity;
-  Optional<internal::ScopedBlockingCallWithBaseSyncPrimitives>
-      scoped_blocking_call;
-  if (waiting_is_blocking_) {
-    event_activity.emplace(this);
-    scoped_blocking_call.emplace(BlockingType::MAY_BLOCK);
+  // TimeTicks takes care of overflow but we special case is_max() nonetheless
+  // to avoid invoking Now() unnecessarily.
+  // WaitForSingleObject(handle_.Get(), INFINITE) doesn't spuriously wakeup so
+  // we don't need to worry about is_max() for the increment phase of the loop.
+  const TimeTicks end_time =
+      wait_delta.is_max() ? TimeTicks::Max() : TimeTicks::Now() + wait_delta;
+  for (TimeDelta remaining = wait_delta; remaining > TimeDelta();
+       remaining = end_time - TimeTicks::Now()) {
+    // Truncate the timeout to milliseconds, rounded up to avoid spinning
+    // (either by returning too early or because a < 1ms timeout on Windows
+    // tends to return immediately).
+    const DWORD timeout_ms =
+        remaining.is_max()
+            ? INFINITE
+            : saturated_cast<DWORD>(remaining.InMillisecondsRoundedUp());
+    const DWORD result = WaitForSingleObject(handle_.Get(), timeout_ms);
+    DCHECK(result == WAIT_OBJECT_0 || result == WAIT_TIMEOUT)
+        << "Unexpected WaitForSingleObject result " << result;
+    switch (result) {
+      case WAIT_OBJECT_0:
+        return true;
+      case WAIT_TIMEOUT:
+        // TimedWait can time out earlier than the specified |timeout| on
+        // Windows. To make this consistent with the posix implementation we
+        // should guarantee that TimedWait doesn't return earlier than the
+        // specified |max_time| and wait again for the remaining time.
+        continue;
+    }
   }
-
-  TimeTicks now(TimeTicks::Now());
-  if (end_time <= now)
-    return IsSignaled();
-
-  return WaitUntil(handle_.Get(), now, end_time);
+  return false;
 }
 
 // static

base/test/scoped_task_environment.h

@@ -103,7 +103,6 @@ class ScopedTaskEnvironment {
     // MOCK_TIME as expected, e.g.:
     //   PlatformThread::Sleep
     //   WaitableEvent::TimedWait
-    //   WaitableEvent::TimedWaitUntil
     //   ConditionVariable::TimedWait
     //
     // TODO(crbug.com/905412): Make MOCK_TIME always mock Time/TimeTicks::Now().

chrome/chrome_cleaner/components/system_report_component.cc

@@ -707,13 +707,14 @@ void ReportInstalledExtensions(JsonParserAPI* json_parser,
   GetNonWhitelistedDefaultExtensions(json_parser, &default_extension_policies,
                                      &default_extensions_done);
 
-  // Wait for all asynchronous parsing to be done
+  // Wait for all asynchronous parsing to be done with a single timeout for all
+  // phases combined.
   const base::TimeTicks end_time =
       base::TimeTicks::Now() +
       base::TimeDelta::FromMilliseconds(kParseAttemptTimeoutMilliseconds);
-  extension_settings_done.TimedWaitUntil(end_time);
-  master_preferences_done.TimedWaitUntil(end_time);
-  default_extensions_done.TimedWaitUntil(end_time);
+  extension_settings_done.TimedWait(end_time - base::TimeTicks::Now());
+  master_preferences_done.TimedWait(end_time - base::TimeTicks::Now());
+  default_extensions_done.TimedWait(end_time - base::TimeTicks::Now());
 
   // Log extensions that were found
   for (const ExtensionPolicyRegistryEntry& policy :

chrome/chrome_cleaner/scanner/force_installed_extension_scanner_impl.cc

@@ -92,7 +92,8 @@ ForceInstalledExtensionScannerImpl::GetForceInstalledExtensions(
     result.emplace(std::move(extension));
   }
 
-  non_whitelist_default_extensions_done.TimedWaitUntil(end_time);
+  non_whitelist_default_extensions_done.TimedWait(end_time -
+                                                  base::TimeTicks::Now());
   while (policy_files_default_extensions.size() > 0) {
     ExtensionPolicyFile file =
         std::move(policy_files_default_extensions.back());
@@ -109,7 +110,7 @@ ForceInstalledExtensionScannerImpl::GetForceInstalledExtensions(
     result.emplace(std::move(extension));
   }
 
-  settings_force_installed_done.TimedWaitUntil(end_time);
+  settings_force_installed_done.TimedWait(end_time - base::TimeTicks::Now());
   while (policy_registry_entries_force_installed.size() > 0) {
     ExtensionPolicyRegistryEntry entry =
         std::move(policy_registry_entries_force_installed.back());
@@ -126,7 +127,7 @@ ForceInstalledExtensionScannerImpl::GetForceInstalledExtensions(
     result.emplace(std::move(extension));
   }
 
-  master_preferences_done.TimedWaitUntil(end_time);
+  master_preferences_done.TimedWait(end_time - base::TimeTicks::Now());
   while (policy_files_master_preferences.size() > 0) {
     ExtensionPolicyFile file =
         std::move(policy_files_master_preferences.back());