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

base/synchronization/waitable_event_win.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/synchronization/waitable_event.h"
 
 #include <windows.h>
 #include <stddef.h>
 
+#include <algorithm>
 #include <utility>
 
 #include "base/debug/activity_tracker.h"
 #include "base/logging.h"
 #include "base/numerics/safe_conversions.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/time/time.h"
 
 namespace base {
 
@@ skipping 17 matching lines @@
 
 void WaitableEvent::Reset() {
   ResetEvent(handle_.Get());
 }
 
 void WaitableEvent::Signal() {
   SetEvent(handle_.Get());
 }
 
 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() {
   // Record the event that this thread is blocking upon (for hang diagnosis).
   base::debug::ScopedEventWaitActivity event_activity(this);
 
   base::ThreadRestrictions::AssertWaitAllowed();
   DWORD result = WaitForSingleObject(handle_.Get(), INFINITE);
   // It is most unexpected that this should ever fail.  Help consumers learn
   // about it if it should ever fail.
   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();
+  DCHECK_GE(max_delta, TimeDelta());
+  if (max_delta.is_zero())
+    return IsSignaled();
 
-  // 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());
+  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);
+}
 
-  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;
+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
 size_t WaitableEvent::WaitMany(WaitableEvent** events, size_t count) {
   DCHECK(count) << "Cannot wait on no events";
 
   // Record an event (the first) that this thread is blocking upon.
   base::debug::ScopedEventWaitActivity event_activity(events[0]);
 
   base::ThreadRestrictions::AssertWaitAllowed();
@@ skipping 12 matching lines @@
                              INFINITE);  // no timeout
   if (result >= WAIT_OBJECT_0 + count) {
     DPLOG(FATAL) << "WaitForMultipleObjects failed";
     return 0;
   }
 
   return result - WAIT_OBJECT_0;
 }
 
 }  // namespace base