Make sure that WaitableEvent::TimedWait works fine with large timeouts.

TimedWait(TimeDelta::Max()) should do the same as Wait() (although should not
be used, given that there is a dedicated method to do that).

More generally, TimedWait should accept random, unreasonable large timeouts.

BUG=465948
TESTS=base_unittests

Review URL: https://codereview.chromium.org/999753005

Cr-Commit-Position: refs/heads/master@{#322469}

4 files changed, 43 insertions, 32 deletions

diff --git a/base/synchronization/waitable_event.h b/base/synchronization/waitable_event.h
index c35af54..ffdd617 100644
--- a/base/synchronization/waitable_event.h
+++ b/base/synchronization/waitable_event.h
@@ -81,6 +81,8 @@ class BASE_EXPORT WaitableEvent {
   // does not necessarily mean that max_time was exceeded.
   //
   // TimedWait can synchronise its own destruction like |Wait|.
+  //
+  // Passing a negative |max_time| is not supported.
   bool TimedWait(const TimeDelta& max_time);
 
 #if defined(OS_WIN)
diff --git a/base/synchronization/waitable_event_posix.cc b/base/synchronization/waitable_event_posix.cc
index 696ffc7..04a262b 100644
--- a/base/synchronization/waitable_event_posix.cc
+++ b/base/synchronization/waitable_event_posix.cc
@@ -151,14 +151,14 @@ class SyncWaiter : public WaitableEvent::Waiter {
 };
 
 void WaitableEvent::Wait() {
-  bool result = TimedWait(TimeDelta::FromSeconds(-1));
+  bool result = TimedWait(TimeDelta::Max());
   DCHECK(result) << "TimedWait() should never fail with infinite timeout";
 }
 
 bool WaitableEvent::TimedWait(const TimeDelta& max_time) {
+  DCHECK_GE(max_time, TimeDelta());
   base::ThreadRestrictions::AssertWaitAllowed();
   const TimeTicks end_time(TimeTicks::Now() + max_time);
-  const bool finite_time = max_time.ToInternalValue() >= 0;
 
   kernel_->lock_.Acquire();
   if (kernel_->signaled_) {
@@ -184,7 +184,7 @@ bool WaitableEvent::TimedWait(const TimeDelta& max_time) {
   for (;;) {
     const TimeTicks current_time(TimeTicks::Now());
 
-    if (sw.fired() || (finite_time && current_time >= end_time)) {
+    if (sw.fired() || current_time >= end_time) {
       const bool return_value = sw.fired();
 
       // We can't acquire @lock_ before releasing the SyncWaiter lock (because
@@ -207,12 +207,7 @@ bool WaitableEvent::TimedWait(const TimeDelta& max_time) {
       return return_value;
     }
 
-    if (finite_time) {
-      const TimeDelta max_wait(end_time - current_time);
-      sw.cv()->TimedWait(max_wait);
-    } else {
-      sw.cv()->Wait();
-    }
+    sw.cv()->TimedWait(end_time - current_time);
   }
 }
 
diff --git a/base/synchronization/waitable_event_unittest.cc b/base/synchronization/waitable_event_unittest.cc
index abba935..392a923 100644
--- a/base/synchronization/waitable_event_unittest.cc
+++ b/base/synchronization/waitable_event_unittest.cc
@@ -73,29 +73,27 @@ TEST(WaitableEventTest, WaitManyShortcut) {
 
 class WaitableEventSignaler : public PlatformThread::Delegate {
  public:
-  WaitableEventSignaler(double seconds, WaitableEvent* ev)
-      : seconds_(seconds),
-        ev_(ev) {
+  WaitableEventSignaler(TimeDelta delay, WaitableEvent* event)
+      : delay_(delay),
+        event_(event) {
   }
 
   void ThreadMain() override {
-    PlatformThread::Sleep(TimeDelta::FromSecondsD(seconds_));
-    ev_->Signal();
+    PlatformThread::Sleep(delay_);
+    event_->Signal();
   }
 
  private:
-  const double seconds_;
-  WaitableEvent *const ev_;
+  const TimeDelta delay_;
+  WaitableEvent* event_;
 };
 
+// Tests that a WaitableEvent can be safely deleted when |Wait| is done without
+// additional synchronization.
 TEST(WaitableEventTest, WaitAndDelete) {
-  // This test tests that if a WaitableEvent can be safely deleted
-  // when |Wait| is done without additional synchrnization.
-  // If this test crashes, it is a bug.
-
   WaitableEvent* ev = new WaitableEvent(false, false);
 
-  WaitableEventSignaler signaler(0.01, ev);
+  WaitableEventSignaler signaler(TimeDelta::FromMilliseconds(10), ev);
   PlatformThreadHandle thread;
   PlatformThread::Create(0, &signaler, &thread);
 
@@ -105,16 +103,14 @@ TEST(WaitableEventTest, WaitAndDelete) {
   PlatformThread::Join(thread);
 }
 
+// Tests that a WaitableEvent can be safely deleted when |WaitMany| is done
+// without additional synchronization.
 TEST(WaitableEventTest, WaitMany) {
-  // This test tests that if a WaitableEvent can be safely deleted
-  // when |WaitMany| is done without additional synchrnization.
-  // If this test crashes, it is a bug.
-
   WaitableEvent* ev[5];
   for (unsigned i = 0; i < 5; ++i)
     ev[i] = new WaitableEvent(false, false);
 
-  WaitableEventSignaler signaler(0.01, ev[2]);
+  WaitableEventSignaler signaler(TimeDelta::FromMilliseconds(10), ev[2]);
   PlatformThreadHandle thread;
   PlatformThread::Create(0, &signaler, &thread);
 
@@ -127,4 +123,22 @@ TEST(WaitableEventTest, WaitMany) {
   EXPECT_EQ(2u, index);
 }
 
+// Tests that using TimeDelta::Max() on TimedWait() is not the same as passing
+// a timeout of 0. (crbug.com/465948)
+TEST(WaitableEventTest, TimedWait) {
+  WaitableEvent* ev = new WaitableEvent(false, false);
+
+  TimeDelta thread_delay = TimeDelta::FromMilliseconds(10);
+  WaitableEventSignaler signaler(thread_delay, ev);
+  PlatformThreadHandle thread;
+  TimeTicks start = TimeTicks::Now();
+  PlatformThread::Create(0, &signaler, &thread);
+
+  ev->TimedWait(TimeDelta::Max());
+  EXPECT_GE(TimeTicks::Now() - start, thread_delay);
+  delete ev;
+
+  PlatformThread::Join(thread);
+}
+
 }  // namespace base
diff --git a/base/synchronization/waitable_event_win.cc b/base/synchronization/waitable_event_win.cc
index 770c582..6bec4b4 100644
--- a/base/synchronization/waitable_event_win.cc
+++ b/base/synchronization/waitable_event_win.cc
@@ -4,10 +4,10 @@
 
 #include "base/synchronization/waitable_event.h"
 
-#include <math.h>
 #include <windows.h>
 
 #include "base/logging.h"
+#include "base/numerics/safe_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 
@@ -37,7 +37,7 @@ void WaitableEvent::Signal() {
 }
 
 bool WaitableEvent::IsSignaled() {
-  return TimedWait(TimeDelta::FromMilliseconds(0));
+  return TimedWait(TimeDelta());
 }
 
 void WaitableEvent::Wait() {
@@ -50,13 +50,13 @@ void WaitableEvent::Wait() {
 
 bool WaitableEvent::TimedWait(const TimeDelta& max_time) {
   base::ThreadRestrictions::AssertWaitAllowed();
-  DCHECK(max_time >= TimeDelta::FromMicroseconds(0));
+  DCHECK_GE(max_time, TimeDelta());
   // Be careful here.  TimeDelta has a precision of microseconds, but this API
   // is in milliseconds.  If there are 5.5ms left, should the delay be 5 or 6?
   // It should be 6 to avoid returning too early.
-  double timeout = ceil(max_time.InMillisecondsF());
-  DWORD result = WaitForSingleObject(handle_.Get(),
-                                     static_cast<DWORD>(timeout));
+  DWORD timeout = saturated_cast<DWORD>(max_time.InMillisecondsRoundedUp());
+
+  DWORD result = WaitForSingleObject(handle_.Get(), timeout);
   switch (result) {
     case WAIT_OBJECT_0:
       return true;