[Synchronization] Fix a crash in WaitableEventWatcher

base/synchronization/waitable_event_watcher_unittest.cc

 // Copyright (c) 2012 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_watcher.h"
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 20 matching lines @@
 
 void QuitWhenSignaled(WaitableEvent* event) {
   MessageLoop::current()->QuitWhenIdle();
 }
 
 class DecrementCountContainer {
  public:
   explicit DecrementCountContainer(int* counter) : counter_(counter) {
   }
   void OnWaitableEventSignaled(WaitableEvent* object) {
+    // NOTE: |object| may be already deleted.
     --(*counter_);
   }
  private:
   int* counter_;
 };
 
 void RunTest_BasicSignal(MessageLoop::Type message_loop_type) {
   MessageLoop message_loop(message_loop_type);
 
   // A manual-reset event that is not yet signaled.
@@ skipping 59 matching lines @@
   {
     WaitableEventWatcher watcher;
     {
       MessageLoop message_loop(message_loop_type);
 
       watcher.StartWatching(&event, BindOnce(&QuitWhenSignaled));
     }
   }
 }
 
-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.
 
   MessageLoop message_loop(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. 

+  }
+}
+
 }  // namespace
 
 //-----------------------------------------------------------------------------
 
 TEST(WaitableEventWatcherTest, BasicSignal) {
   for (int i = 0; i < kNumTestingMessageLoops; i++) {
     RunTest_BasicSignal(testing_message_loops[i]);
   }
 }
 
@@ skipping 10 matching lines @@
 }
 
 TEST(WaitableEventWatcherTest, OutlivesMessageLoop) {
   for (int i = 0; i < kNumTestingMessageLoops; i++) {
     RunTest_OutlivesMessageLoop(testing_message_loops[i]);
   }
 }
 
 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);
+  }
+}
+
 }  // namespace base