[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/memory/ptr_util.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/platform_thread.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace base {
 
 namespace {
 
 // The message loops on which each waitable event timer should be tested.
 const MessageLoop::Type testing_message_loops[] = {
   MessageLoop::TYPE_DEFAULT,
   MessageLoop::TYPE_IO,
 #if !defined(OS_IOS)  // iOS does not allow direct running of the UI loop.
   MessageLoop::TYPE_UI,
 #endif
 };
 
 const int kNumTestingMessageLoops = arraysize(testing_message_loops);
 
 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 deferred_watcher_deletion) {
   // Delete the WaitableEvent out from under the Watcher. This is explictly
   // allowed by the interface.
+  // If |deferred_watcher_delete| is true the watcher will be deleted in another
+  // callstack. (Calling the dtor can make a difference for the behavior
+  // of the test)

[danakj, 2017/05/02 14:46:22]

nit: .

[atuchin, 2017/05/03 13:01:58]

Done.

 
   MessageLoop message_loop(message_loop_type);
 
   {
-    WaitableEventWatcher watcher;
+    auto watcher = base::MakeUnique<WaitableEventWatcher>();
 
-    WaitableEvent* event =
-        new WaitableEvent(WaitableEvent::ResetPolicy::AUTOMATIC,
-                          WaitableEvent::InitialState::NOT_SIGNALED);
+    auto event = base::MakeUnique<WaitableEvent>(
+        WaitableEvent::ResetPolicy::AUTOMATIC,
+        WaitableEvent::InitialState::NOT_SIGNALED);
 
-    watcher.StartWatching(event, BindOnce(&QuitWhenSignaled));
-    delete event;
+    watcher->StartWatching(event.get(), BindOnce(&QuitWhenSignaled));
+    event.reset();
+    if (deferred_watcher_deletion) {
+      base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE,

[danakj, 2017/05/02 14:46:22]

nit: SequencedTaskRunnerHandle

[atuchin, 2017/05/03 13:01:58]

Done.

+                                                      watcher.release());
+    }
   }
 }
 
+void RunTest_SignalAndDelete(MessageLoop::Type message_loop_type,
+                             bool deferred_watcher_deletion) {
+  // Signal and immediately delete the WaitableEvent out from under the Watcher.
+  // If |deferred_watcher_delete| is true the watcher will be deleted in another
+  // callstack. (Calling the dtor can make a difference for the behavior

[danakj, 2017/05/02 14:46:21]

It would be nice if you instead explained how it makes a difference, what
ordering it changes. Like if you do this then something else occurs before the
destructor is deleted etc.

[atuchin, 2017/05/03 13:01:58]

Ok, I rewrote the comment, trying to avoid platform specific details (because
the tests is common for Windows and POSIX).

[danakj, 2017/05/03 14:50:03]

FWIW, IMO it's fine to say "On windows, ..." in a test comment.

+  // of the test)

[danakj, 2017/05/02 14:46:22]

nit: .

[atuchin, 2017/05/03 13:01:58]

Done.

+
+  MessageLoop message_loop(message_loop_type);
+
+  {
+    auto watcher = base::MakeUnique<WaitableEventWatcher>();
+
+    auto event = base::MakeUnique<WaitableEvent>(
+        WaitableEvent::ResetPolicy::AUTOMATIC,
+        WaitableEvent::InitialState::NOT_SIGNALED);
+
+    watcher->StartWatching(event.get(), BindOnce(&QuitWhenSignaled));
+    event->Signal();
+    event.reset();
+    if (deferred_watcher_deletion) {
+      base::ThreadTaskRunnerHandle::Get()->DeleteSoon(FROM_HERE,

[danakj, 2017/05/02 14:46:22]

nit: SequencedTaskRunnerHandle

[atuchin, 2017/05/03 13:01:58]

Done.

+                                                      watcher.release());
+    }
+  }
+}
+
 }  // 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