base: Take care of unexpected wakeups in condition_variable_win.cc.

base/synchronization/condition_variable_win.cc

Index: base/synchronization/condition_variable_win.cc
diff --git a/base/synchronization/condition_variable_win.cc b/base/synchronization/condition_variable_win.cc
index 8bece6eb34dbfc965ccee65b08051c54ad9fff79..17675d07dd2aa4d172bdcb12dc2c8c9a0ab715a5 100644
--- a/base/synchronization/condition_variable_win.cc
+++ b/base/synchronization/condition_variable_win.cc
@@ -10,6 +10,57 @@
 
 namespace base {
 
+namespace {
+
+// Infinite timeout in milliseconds.
+static const int64_t kInfiniteTimeout =
+    TimeDelta::FromMilliseconds(INFINITE).InMilliseconds();
+
+// Number of tries after SleepConditionVariableSRW() fails.
+static const int kMaxNumberOfTriesAfterFailure = 10;

[reveman, 2016/10/03 10:58:07]

do we need to limit the max number of retries? what if 10 retries is needed for
a valid use case?

[prashant.n, 2016/10/03 11:40:14]

Hmm. Actually MSDN documentation tells for ever while loop, no limits.

I thought putting some limits, so that we can control looping erroneous
conditions. May be we can decide on proper limit number.

In case CategorizedWorkerPool, while loop without limits is okay, but for other
users of condition variable it may cause forever waits. (Not confirmed in code
though.)

[reveman, 2016/10/03 13:23:40]

The situation is very similar to how we handle interrupts on posix platforms. In
that case we loop forever in debug builds and make 100 attempts in release
builds: 
https://cs.chromium.org/chromium/src/base/posix/eintr_wrapper.h?rcl=0&l=37

maybe do the same here?

+
+// As per MSDN documentation (https://goo.gl/tPHPkY), condition variables are
+// subject to spurious wakeups (those not associated with an explicit wake) and
+// stolen wakeups (another thread manages to run before the woken thread). This
+// function checks whether it's a real timeout and if not it retries
+// kMaxNumberOfTriesAfterFailure times after which it crashes.
+void SleepConditionVariable(PCONDITION_VARIABLE cv,
+                            PSRWLOCK srw_lock,
+                            BOOL infinite_timeout,
+                            DWORD timeout,
+                            ULONG flags) {
+  base::Time time_start = base::Time::Now();
+  int num_tries_after_failure = 0;
+
+  while (!SleepConditionVariableSRW(cv, srw_lock, timeout, flags)) {
+    // Note: Use ERROR_TIMEOUT with GetLastError().
+    DWORD error = GetLastError();
+
+    num_tries_after_failure++;
+    CHECK_LE(num_tries_after_failure, kMaxNumberOfTriesAfterFailure)
+        << "SleepConditionVariableSRW() failed after max number of retries "
+           "with error code ("
+        << error << ").";
+
+    if (infinite_timeout) {
+      // Ideally for infinite timeout, we should not get ERROR_TIMEOUT error.
+      DCHECK_NE(error, static_cast<DWORD>(ERROR_TIMEOUT));
+    } else {
+      if (error == ERROR_TIMEOUT)
+        break;
+
+      // Compute remaining timeout.
+      int64_t time_delta = (base::Time::Now() - time_start).InMilliseconds();
+      timeout -= time_delta;
+      // Break here as if timeout has occurred, otherwise continue to wait
+      // for the remaining timeout.
+      if (timeout <= 0)
+        break;
+    }
+  }
+}
+}
+
 ConditionVariable::ConditionVariable(Lock* user_lock)
     : srwlock_(user_lock->lock_.native_handle())
 #if DCHECK_IS_ON()
@@ -23,25 +74,28 @@ ConditionVariable::ConditionVariable(Lock* user_lock)
 ConditionVariable::~ConditionVariable() = default;
 
 void ConditionVariable::Wait() {
-  TimedWait(TimeDelta::FromMilliseconds(INFINITE));
+  base::ThreadRestrictions::AssertWaitAllowed();
+
+#if DCHECK_IS_ON()
+  user_lock_->CheckHeldAndUnmark();
+#endif
+
+  SleepConditionVariable(&cv_, srwlock_, true, kInfiniteTimeout, 0);
+
+#if DCHECK_IS_ON()
+  user_lock_->CheckUnheldAndMark();
+#endif
 }
 
 void ConditionVariable::TimedWait(const TimeDelta& max_time) {
   base::ThreadRestrictions::AssertWaitAllowed();
-  DWORD timeout = static_cast<DWORD>(max_time.InMilliseconds());
 
 #if DCHECK_IS_ON()
   user_lock_->CheckHeldAndUnmark();
 #endif
 
-  if (!SleepConditionVariableSRW(&cv_, srwlock_, timeout, 0)) {
-    // On failure, we only expect the CV to timeout. Any other error value means
-    // that we've unexpectedly woken up.
-    // Note that WAIT_TIMEOUT != ERROR_TIMEOUT. WAIT_TIMEOUT is used with the
-    // WaitFor* family of functions as a direct return value. ERROR_TIMEOUT is
-    // used with GetLastError().
-    DCHECK_EQ(static_cast<DWORD>(ERROR_TIMEOUT), GetLastError());
-  }
+  SleepConditionVariable(&cv_, srwlock_, false,
+                         static_cast<DWORD>(max_time.InMilliseconds()), 0);
 
 #if DCHECK_IS_ON()
   user_lock_->CheckUnheldAndMark();