[Synchronization] Fix a crash in WaitableEventWatcher

base/synchronization/waitable_event_watcher_unittest.cc

Index: base/synchronization/waitable_event_watcher_unittest.cc
diff --git a/base/synchronization/waitable_event_watcher_unittest.cc b/base/synchronization/waitable_event_watcher_unittest.cc
index 9df72892c3c3813adf8d1286c7bdf6dfefe48930..d99a75699a2d51c12937f093e27da517a70939fb 100644
--- a/base/synchronization/waitable_event_watcher_unittest.cc
+++ b/base/synchronization/waitable_event_watcher_unittest.cc
@@ -38,6 +38,7 @@ class DecrementCountContainer {
   explicit DecrementCountContainer(int* counter) : counter_(counter) {
   }
   void OnWaitableEventSignaled(WaitableEvent* object) {
+    // NOTE: |object| may be already deleted.
     --(*counter_);
   }
  private:
@@ -117,7 +118,7 @@ void RunTest_OutlivesMessageLoop(MessageLoop::Type message_loop_type) {
   }
 }
 
-void RunTest_DeleteUnder(MessageLoop::Type message_loop_type) {
+void RunTest_DeleteUnder(MessageLoop::Type message_loop_type, bool with_delay) {
   // Delete the WaitableEvent out from under the Watcher. This is explictly
   // allowed by the interface.
 
@@ -126,12 +127,35 @@ void RunTest_DeleteUnder(MessageLoop::Type message_loop_type) {
   {
     WaitableEventWatcher watcher;
 
-    WaitableEvent* event =
-        new WaitableEvent(WaitableEvent::ResetPolicy::AUTOMATIC,
-                          WaitableEvent::InitialState::NOT_SIGNALED);
+    auto event = base::MakeUnique<WaitableEvent>(

[danakj, 2017/04/27 20:01:08]

needs ptr_util.h

[atuchin, 2017/04/28 06:44:29]

Done.

+        WaitableEvent::ResetPolicy::AUTOMATIC,
+        WaitableEvent::InitialState::NOT_SIGNALED);
 
-    watcher.StartWatching(event, BindOnce(&QuitWhenSignaled));
-    delete event;
+    watcher.StartWatching(event.get(), BindOnce(&QuitWhenSignaled));
+    event.reset();
+    if (with_delay)
+      PlatformThread::Sleep(TimeDelta::FromMilliseconds(10));

[danakj, 2017/04/27 20:01:08]

What is this sleep meant to do?

[atuchin, 2017/04/28 06:44:29]

Unfortunately, without that sleep the test fails to reproduce the race.

My suggestion: the crash happens when started thread in win thread pool failed
tries to look a invalidated handle.  
And that sleep changes the execution order between the current thread and the
win thread pool thread (who watches for the handle). 

Yes, putting sleep in tests don't make me happy, but I don't know an another way
to catch the race here.
We cannot rely on windows internals (and all works fine on win10).

My approach is to check both variants (with sleep() and without). I added a
comment.
May be you suggest an another?

[danakj, 2017/04/28 17:54:49]

IIUC we don't have the ability to run code on this thread? Else we could have it
signal the test to continue. What if you make 2 WaitableEvents, and delete the
first when the 2nd is signalled.. or vice versa? Maybe that misses the race?

[atuchin, 2017/05/02 07:41:19]

Yes, we don't have a such ability.
But I have found another solution. The problem with crash reproduction in the
test is connected with dtor of WaitableEventWatcher (it calls UnregisterWaitEx
on the handle: ~WaitableEventWatcher() -> ~ObjectWatcher() -> StopWatching() ->
UnregisterWaitEx()). I moved dtor away from the current stack and tests started
to work fine without sleep (crashed on Win7 without the fix).

That approach didn't work for my short example above, but now I think it was
because the test program immediately shuts down after the handle manipulations
(I should add waiting after).
Thanks!

+  }
+}
+
+void RunTest_SignalAndDelete(MessageLoop::Type message_loop_type,
+                             bool with_delay) {
+  // Signal and immediately delete the WaitableEvent out from under the Watcher.
+
+  MessageLoop message_loop(message_loop_type);
+
+  {
+    WaitableEventWatcher watcher;
+
+    auto event = base::MakeUnique<WaitableEvent>(
+        WaitableEvent::ResetPolicy::AUTOMATIC,
+        WaitableEvent::InitialState::NOT_SIGNALED);
+
+    watcher.StartWatching(event.get(), BindOnce(&QuitWhenSignaled));
+    event->Signal();
+    event.reset();
+    if (with_delay)
+      PlatformThread::Sleep(TimeDelta::FromMilliseconds(10));

[danakj, 2017/04/27 20:01:08]

Also this sleep?

[atuchin, 2017/04/28 06:44:29]

See the answer in another one. 

   }
 }
 
@@ -165,7 +189,15 @@ TEST(WaitableEventWatcherTest, OutlivesMessageLoop) {
 
 TEST(WaitableEventWatcherTest, DeleteUnder) {
   for (int i = 0; i < kNumTestingMessageLoops; i++) {
-    RunTest_DeleteUnder(testing_message_loops[i]);
+    RunTest_DeleteUnder(testing_message_loops[i], false);
+    RunTest_DeleteUnder(testing_message_loops[i], true);
+  }
+}
+
+TEST(WaitableEventWatcherTest, SignalAndDelete) {
+  for (int i = 0; i < kNumTestingMessageLoops; i++) {
+    RunTest_SignalAndDelete(testing_message_loops[i], false);
+    RunTest_SignalAndDelete(testing_message_loops[i], true);
   }
 }