From 7b67e033842715d4941d46e726df8ca18d94d768 Mon Sep 17 00:00:00 2001 From: fdoray Date: Tue, 27 Sep 2016 05:23:33 -0700 Subject: [PATCH] Remove MessageLoop::current() from base::win::ObjectWatcher (reland). --- This is a reland of https://codereview.chromium.org/2125763003 which was reverted because it broke the Win10 builder. It should not break anything now that TestSimpleTaskRunner is thread-safe (https://codereview.chromium.org/2296923003) --- Why? The fact that there's a MessageLoop on the thread is an unnecessary implementation detail. When browser threads are migrated to base/task_scheduler, tasks will no longer have access to a MessageLoop but they will be able to get the current ThreadTaskRunnerHandle. Before this CL, ObjectWatcher implemented WillDestroyCurrentMessageLoop() to stop the watch when the MessageLoop responsible for running the callback was destroyed. This prevented the watch callback from being sent to a destroyed MessageLoop. Now that ObjectWatcher keeps a reference to a TaskRunner, this is no longer required. The TaskRunner can't be deleted while there are references to it. If the underlying MessageLoop has been deleted when the watch callback is posted, the callback will simply not run. Note that the watch will still be stopped when the ObjectWatcher is deleted. TBR=ben@chromium.org BUG=650318 Review-Url: https://codereview.chromium.org/2365423003 Cr-Commit-Position: refs/heads/master@{#421179} --- base/files/file_path_watcher_win.cc | 1 + base/process/kill_win.cc | 1 + base/win/object_watcher.cc | 54 +++++++++-------------- base/win/object_watcher.h | 41 ++++++++++------- chrome/common/service_process_util_win.cc | 1 + ui/gfx/font_fallback_win.cc | 1 + 6 files changed, 49 insertions(+), 50 deletions(-) diff --git a/base/files/file_path_watcher_win.cc b/base/files/file_path_watcher_win.cc index f2de78343ef096..9121fa976b5f26 100644 --- a/base/files/file_path_watcher_win.cc +++ b/base/files/file_path_watcher_win.cc @@ -11,6 +11,7 @@ #include "base/logging.h" #include "base/macros.h" #include "base/memory/ref_counted.h" +#include "base/message_loop/message_loop.h" #include "base/threading/thread_task_runner_handle.h" #include "base/time/time.h" #include "base/win/object_watcher.h" diff --git a/base/process/kill_win.cc b/base/process/kill_win.cc index 5ba524889ee7c8..ecb08421cfa230 100644 --- a/base/process/kill_win.cc +++ b/base/process/kill_win.cc @@ -8,6 +8,7 @@ #include #include +#include #include #include "base/bind.h" diff --git a/base/win/object_watcher.cc b/base/win/object_watcher.cc index 4df54a45d6bf90..9a7eea2b054eb8 100644 --- a/base/win/object_watcher.cc +++ b/base/win/object_watcher.cc @@ -6,19 +6,14 @@ #include "base/bind.h" #include "base/logging.h" +#include "base/threading/thread_task_runner_handle.h" namespace base { namespace win { //----------------------------------------------------------------------------- -ObjectWatcher::ObjectWatcher() - : object_(NULL), - wait_object_(NULL), - origin_loop_(NULL), - run_once_(true), - weak_factory_(this) { -} +ObjectWatcher::ObjectWatcher() : weak_factory_(this) {} ObjectWatcher::~ObjectWatcher() { StopWatching(); @@ -38,7 +33,7 @@ bool ObjectWatcher::StopWatching() { return false; // Make sure ObjectWatcher is used in a single-threaded fashion. - DCHECK_EQ(origin_loop_, MessageLoop::current()); + DCHECK(task_runner_->BelongsToCurrentThread()); // Blocking call to cancel the wait. Any callbacks already in progress will // finish before we return from this call. @@ -47,16 +42,12 @@ bool ObjectWatcher::StopWatching() { return false; } - weak_factory_.InvalidateWeakPtrs(); - object_ = NULL; - wait_object_ = NULL; - - origin_loop_->RemoveDestructionObserver(this); + Reset(); return true; } bool ObjectWatcher::IsWatching() const { - return object_ != NULL; + return object_ != nullptr; } HANDLE ObjectWatcher::GetWatchedObject() const { @@ -70,22 +61,18 @@ void CALLBACK ObjectWatcher::DoneWaiting(void* param, BOOLEAN timed_out) { // The destructor blocks on any callbacks that are in flight, so we know that // that is always a pointer to a valid ObjectWater. ObjectWatcher* that = static_cast(param); - that->origin_loop_->task_runner()->PostTask(FROM_HERE, that->callback_); + that->task_runner_->PostTask(FROM_HERE, that->callback_); if (that->run_once_) that->callback_.Reset(); } bool ObjectWatcher::StartWatchingInternal(HANDLE object, Delegate* delegate, bool execute_only_once) { - CHECK(delegate); - if (wait_object_) { - NOTREACHED() << "Already watching an object"; - return false; - } + DCHECK(delegate); + DCHECK(!wait_object_) << "Already watching an object"; + DCHECK(ThreadTaskRunnerHandle::IsSet()); - origin_loop_ = MessageLoop::current(); - if (!origin_loop_) - return false; + task_runner_ = ThreadTaskRunnerHandle::Get(); run_once_ = execute_only_once; @@ -97,21 +84,17 @@ bool ObjectWatcher::StartWatchingInternal(HANDLE object, Delegate* delegate, // DoneWaiting can be synchronously called from RegisterWaitForSingleObject, // so set up all state now. - callback_ = base::Bind(&ObjectWatcher::Signal, weak_factory_.GetWeakPtr(), - delegate); + callback_ = + Bind(&ObjectWatcher::Signal, weak_factory_.GetWeakPtr(), delegate); object_ = object; if (!RegisterWaitForSingleObject(&wait_object_, object, DoneWaiting, this, INFINITE, wait_flags)) { DPLOG(FATAL) << "RegisterWaitForSingleObject failed"; - object_ = NULL; - wait_object_ = NULL; + Reset(); return false; } - // We need to know if the current message loop is going away so we can - // prevent the wait thread from trying to access a dead message loop. - origin_loop_->AddDestructionObserver(this); return true; } @@ -125,10 +108,13 @@ void ObjectWatcher::Signal(Delegate* delegate) { delegate->OnObjectSignaled(object); } -void ObjectWatcher::WillDestroyCurrentMessageLoop() { - // Need to shutdown the watch so that we don't try to access the MessageLoop - // after this point. - StopWatching(); +void ObjectWatcher::Reset() { + callback_.Reset(); + object_ = nullptr; + wait_object_ = nullptr; + task_runner_ = nullptr; + run_once_ = true; + weak_factory_.InvalidateWeakPtrs(); } } // namespace win diff --git a/base/win/object_watcher.h b/base/win/object_watcher.h index e1e86fe73aa38e..a2821c114f1827 100644 --- a/base/win/object_watcher.h +++ b/base/win/object_watcher.h @@ -10,8 +10,9 @@ #include "base/base_export.h" #include "base/callback.h" #include "base/macros.h" +#include "base/memory/ref_counted.h" #include "base/memory/weak_ptr.h" -#include "base/message_loop/message_loop.h" +#include "base/single_thread_task_runner.h" namespace base { namespace win { @@ -46,22 +47,22 @@ namespace win { // If the object is already signaled before being watched, OnObjectSignaled is // still called after (but not necessarily immediately after) watch is started. // -// NOTE: Use of this class requires that there be a current message loop; -// otherwise, when the object is signaled, there would be no loop to post the -// callback task to. This means that you cannot use ObjectWatcher in test code -// that doesn't create a message loop (unless you add such a loop). -class BASE_EXPORT ObjectWatcher : public MessageLoop::DestructionObserver { +// NOTE: Except for the constructor, all public methods of this class must be +// called on the same thread. A ThreadTaskRunnerHandle must be set on that +// thread. +class BASE_EXPORT ObjectWatcher { public: class BASE_EXPORT Delegate { public: virtual ~Delegate() {} - // Called from the MessageLoop when a signaled object is detected. To - // continue watching the object, StartWatching must be called again. + // Called from the thread that started the watch when a signaled object is + // detected. To continue watching the object, StartWatching must be called + // again. virtual void OnObjectSignaled(HANDLE object) = 0; }; ObjectWatcher(); - ~ObjectWatcher() override; + ~ObjectWatcher(); // When the object is signaled, the given delegate is notified on the thread // where StartWatchingOnce is called. The ObjectWatcher is not responsible for @@ -99,15 +100,23 @@ class BASE_EXPORT ObjectWatcher : public MessageLoop::DestructionObserver { void Signal(Delegate* delegate); - // MessageLoop::DestructionObserver implementation: - void WillDestroyCurrentMessageLoop() override; + void Reset(); - // Internal state. + // A callback pre-bound to Signal() that is posted to the caller's task runner + // when the wait completes. Closure callback_; - HANDLE object_; // The object being watched - HANDLE wait_object_; // Returned by RegisterWaitForSingleObject - MessageLoop* origin_loop_; // Used to get back to the origin thread - bool run_once_; + + // The object being watched. + HANDLE object_ = nullptr; + + // The wait handle returned by RegisterWaitForSingleObject. + HANDLE wait_object_ = nullptr; + + // The task runner of the thread on which the watch was started. + scoped_refptr task_runner_; + + bool run_once_ = true; + WeakPtrFactory weak_factory_; DISALLOW_COPY_AND_ASSIGN(ObjectWatcher); diff --git a/chrome/common/service_process_util_win.cc b/chrome/common/service_process_util_win.cc index cd0cff9d1144f7..047acd90360fc4 100644 --- a/chrome/common/service_process_util_win.cc +++ b/chrome/common/service_process_util_win.cc @@ -4,6 +4,7 @@ #include "chrome/common/service_process_util.h" +#include #include #include "base/callback.h" diff --git a/ui/gfx/font_fallback_win.cc b/ui/gfx/font_fallback_win.cc index 66aba3536afe23..8af5604a3f0c71 100644 --- a/ui/gfx/font_fallback_win.cc +++ b/ui/gfx/font_fallback_win.cc @@ -14,6 +14,7 @@ #include "base/i18n/rtl.h" #include "base/macros.h" #include "base/memory/singleton.h" +#include "base/message_loop/message_loop.h" #include "base/profiler/scoped_tracker.h" #include "base/strings/string_split.h" #include "base/strings/string_util.h"