Move OS_WIN specific logic in MessagePumpDefault::Run into waitable_event_win.cc

base/synchronization/waitable_event_win.cc

Index: base/synchronization/waitable_event_win.cc
diff --git a/base/synchronization/waitable_event_win.cc b/base/synchronization/waitable_event_win.cc
index d80cabb3ff3ae864fb25f54448885d2c252c5f2b..df274aab39773dc12334a8c704b17dedfde2272a 100644
--- a/base/synchronization/waitable_event_win.cc
+++ b/base/synchronization/waitable_event_win.cc
@@ -7,6 +7,7 @@
 #include <windows.h>
 #include <stddef.h>
 
+#include <algorithm>
 #include <utility>
 
 #include "base/debug/activity_tracker.h"
@@ -44,7 +45,10 @@ void WaitableEvent::Signal() {
 }
 
 bool WaitableEvent::IsSignaled() {
-  return TimedWait(TimeDelta());
+  DWORD result = WaitForSingleObject(handle_.Get(), 0);
+  DCHECK(result == WAIT_OBJECT_0 || result == WAIT_TIMEOUT)
+      << "Unexpected WaitForSingleObject result " << result;
+  return result == WAIT_OBJECT_0;
 }
 
 void WaitableEvent::Wait() {
@@ -58,30 +62,64 @@ void WaitableEvent::Wait() {
   DCHECK_EQ(WAIT_OBJECT_0, result) << "WaitForSingleObject failed";
 }
 
-bool WaitableEvent::TimedWait(const TimeDelta& max_time) {
+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& max_delta) {
   // Record the event that this thread is blocking upon (for hang diagnosis).
   base::debug::ScopedEventWaitActivity event_activity(this);
+  base::ThreadRestrictions::AssertWaitAllowed();

[danakj, 2016/11/16 22:57:29]

This should be after the check for is_zero right?

[stanisc, 2016/11/17 19:42:22]

That's what I had before, but I thought you suggested putting the assert at the
top. Actually I've just realized you wanted to put the assert just above
event_activity.

Done.

 
-  DCHECK_GE(max_time, TimeDelta());
-  if (!max_time.is_zero())
-    base::ThreadRestrictions::AssertWaitAllowed();
-
-  // Truncate the timeout to milliseconds. The API specifies that this method
-  // can return in less than |max_time| (when returning false), as the argument
-  // is the maximum time that a caller is willing to wait.
-  DWORD timeout = saturated_cast<DWORD>(max_time.InMilliseconds());
-
-  DWORD result = WaitForSingleObject(handle_.Get(), timeout);
-  switch (result) {
-    case WAIT_OBJECT_0:
-      return true;
-    case WAIT_TIMEOUT:
-      return false;
-  }
-  // It is most unexpected that this should ever fail.  Help consumers learn
-  // about it if it should ever fail.
-  NOTREACHED() << "WaitForSingleObject failed";
-  return false;
+  DCHECK_GE(max_delta, TimeDelta());
+  if (max_delta.is_zero())
+    return IsSignaled();
+
+  TimeTicks now(TimeTicks::Now());
+  // TimeTicks takes care of overflow including the cases when max_delta
+  // is a maximum value.
+  return WaitUntil(handle_.Get(), now, now + max_delta);
+}
+
+bool WaitableEvent::TimedWaitUntil(const TimeTicks& end_time) {
+  // Record the event that this thread is blocking upon (for hang diagnosis).
+  base::debug::ScopedEventWaitActivity event_activity(this);
+  base::ThreadRestrictions::AssertWaitAllowed();

[danakj, 2016/11/16 22:57:29]

same, but we should check for is_zero explicitly here too not just if the
timeout is done?

[stanisc, 2016/11/17 19:42:22]

Moved the assert below the check. Shouldn't zero TimeTicks be already handled by
the condition. I could add an explicit check it it seems it would be redundant.

[danakj, 2016/11/17 21:11:08]

0 would be handled, but other things too. I don't want the assert to be skipped
for any non-0 cases.

[stanisc, 2016/11/18 02:13:47]

OK, I've added a special case for zero TimeTicks.

+
+  TimeTicks now(TimeTicks::Now());
+  if (end_time <= now)
+    return IsSignaled();
+
+  return WaitUntil(handle_.Get(), now, end_time);
 }
 
 // static