Make sure that the UI window created by base::MessagePumpForUI is destoyed on the same thread (Windows).

Currently the window created base::MessagePumpForUI can be destroyed on a wrong thread. base::MessagePumpForUI is a ref-counted class so it can (and does) outlive the owning base::MessageLoop. As the result DestroyWindow() can be called on a wrong thread. This makes TSAN unhappy and it reports races deep unside user32.dll.

Changes in this CL:
- The message pump is now notified when the owning message loop is being destroyed. The notification is used to free all resources that hve to be released on the base::MessageLoop's thread.
- MessagePumpForUI::ScheduleWork() synchronizes access to the message-only window handle to avoid posting messages to the window during or after its destruction.

BUG=241939

Review URL: https://chromiumcodereview.appspot.com/15709015

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@207278 0039d316-1c4b-4281-b951-d872f2087c98

14 files changed, 85 insertions, 13 deletions

diff --git a/base/message_loop/message_loop.cc b/base/message_loop/message_loop.cc
index 4e0c5f6..e443012 100644
--- a/base/message_loop/message_loop.cc
+++ b/base/message_loop/message_loop.cc
@@ -221,6 +221,9 @@ MessageLoop::~MessageLoop() {
       WillDestroyCurrentMessageLoop();
   message_loop_proxy_ = NULL;
 
+  // Stop the message pump and free any thread-bound resources.
+  pump_->Shutdown();
+
   // OK, now make it so that no one can find us.
   lazy_tls_ptr.Pointer()->Set(NULL);
 
diff --git a/base/message_loop/message_pump.h b/base/message_loop/message_pump.h
index 5b72232..a59d71b 100644
--- a/base/message_loop/message_pump.h
+++ b/base/message_loop/message_pump.h
@@ -119,6 +119,12 @@ class BASE_EXPORT MessagePump : public RefCountedThreadSafe<MessagePump> {
   // used on the thread that called Run.
   virtual void ScheduleDelayedWork(const TimeTicks& delayed_work_time) = 0;
 
+  // Stops the pump after exiting the last run loop. This method must be used on
+  // the same thread that called Run(). The method frees any resources
+  // affinitized to that thread because ~MessagePump() might be invoked on an
+  // arbitrary thread.
+  virtual void Shutdown() = 0;
+
  protected:
   virtual ~MessagePump();
   friend class RefCountedThreadSafe<MessagePump>;
diff --git a/base/message_loop/message_pump_android.cc b/base/message_loop/message_pump_android.cc
index d7bf9a9..9432c73 100644
--- a/base/message_loop/message_pump_android.cc
+++ b/base/message_loop/message_pump_android.cc
@@ -131,6 +131,9 @@ void MessagePumpForUI::ScheduleDelayedWork(const TimeTicks& delayed_work_time) {
       g_system_message_handler_obj.Get().obj(), millis);
 }
 
+void MessagePumpForUI::Shutdown() {
+}
+
 // static
 bool MessagePumpForUI::RegisterBindings(JNIEnv* env) {
   return RegisterNativesImpl(env);
diff --git a/base/message_loop/message_pump_android.h b/base/message_loop/message_pump_android.h
index fd934a7..0473d3e 100644
--- a/base/message_loop/message_pump_android.h
+++ b/base/message_loop/message_pump_android.h
@@ -26,6 +26,7 @@ class BASE_EXPORT MessagePumpForUI : public MessagePump {
   virtual void Quit() OVERRIDE;
   virtual void ScheduleWork() OVERRIDE;
   virtual void ScheduleDelayedWork(const TimeTicks& delayed_work_time) OVERRIDE;
+  virtual void Shutdown() OVERRIDE;
 
   virtual void Start(Delegate* delegate);
 
diff --git a/base/message_loop/message_pump_default.cc b/base/message_loop/message_pump_default.cc
index b36ff21..3edc859 100644
--- a/base/message_loop/message_pump_default.cc
+++ b/base/message_loop/message_pump_default.cc
@@ -82,4 +82,7 @@ void MessagePumpDefault::ScheduleDelayedWork(
   delayed_work_time_ = delayed_work_time;
 }
 
+void MessagePumpDefault::Shutdown() {
+}
+
 }  // namespace base
diff --git a/base/message_loop/message_pump_default.h b/base/message_loop/message_pump_default.h
index dd65973..4d5684d 100644
--- a/base/message_loop/message_pump_default.h
+++ b/base/message_loop/message_pump_default.h
@@ -20,6 +20,7 @@ class MessagePumpDefault : public MessagePump {
   virtual void Quit() OVERRIDE;
   virtual void ScheduleWork() OVERRIDE;
   virtual void ScheduleDelayedWork(const TimeTicks& delayed_work_time) OVERRIDE;
+  virtual void Shutdown() OVERRIDE;
 
  protected:
   virtual ~MessagePumpDefault() {}
diff --git a/base/message_loop/message_pump_glib.cc b/base/message_loop/message_pump_glib.cc
index de012fd..d6b3822 100644
--- a/base/message_loop/message_pump_glib.cc
+++ b/base/message_loop/message_pump_glib.cc
@@ -320,6 +320,9 @@ void MessagePumpGlib::ScheduleDelayedWork(const TimeTicks& delayed_work_time) {
   ScheduleWork();
 }
 
+void MessagePumpGlib::Shutdown() {
+}
+
 MessagePumpGlib::~MessagePumpGlib() {
   g_source_destroy(work_source_);
   g_source_unref(work_source_);
diff --git a/base/message_loop/message_pump_glib.h b/base/message_loop/message_pump_glib.h
index e45591b..fb63018 100644
--- a/base/message_loop/message_pump_glib.h
+++ b/base/message_loop/message_pump_glib.h
@@ -62,6 +62,7 @@ class BASE_EXPORT MessagePumpGlib : public MessagePump {
   virtual void Quit() OVERRIDE;
   virtual void ScheduleWork() OVERRIDE;
   virtual void ScheduleDelayedWork(const TimeTicks& delayed_work_time) OVERRIDE;
+  virtual void Shutdown() OVERRIDE;
 
  protected:
   virtual ~MessagePumpGlib();
diff --git a/base/message_loop/message_pump_libevent.cc b/base/message_loop/message_pump_libevent.cc
index 8de0db2..2a4cabe 100644
--- a/base/message_loop/message_pump_libevent.cc
+++ b/base/message_loop/message_pump_libevent.cc
@@ -302,6 +302,9 @@ void MessagePumpLibevent::ScheduleDelayedWork(
   delayed_work_time_ = delayed_work_time;
 }
 
+void MessagePumpLibevent::Shutdown() {
+}
+
 void MessagePumpLibevent::WillProcessIOEvent() {
   FOR_EACH_OBSERVER(IOObserver, io_observers_, WillProcessIOEvent());
 }
diff --git a/base/message_loop/message_pump_libevent.h b/base/message_loop/message_pump_libevent.h
index a54ef88..c430b9f 100644
--- a/base/message_loop/message_pump_libevent.h
+++ b/base/message_loop/message_pump_libevent.h
@@ -125,6 +125,7 @@ class BASE_EXPORT MessagePumpLibevent : public MessagePump {
   virtual void Quit() OVERRIDE;
   virtual void ScheduleWork() OVERRIDE;
   virtual void ScheduleDelayedWork(const TimeTicks& delayed_work_time) OVERRIDE;
+  virtual void Shutdown() OVERRIDE;
 
  protected:
   virtual ~MessagePumpLibevent();
diff --git a/base/message_loop/message_pump_mac.h b/base/message_loop/message_pump_mac.h
index 68c2a3d..db2bf8e 100644
--- a/base/message_loop/message_pump_mac.h
+++ b/base/message_loop/message_pump_mac.h
@@ -73,6 +73,7 @@ class MessagePumpCFRunLoopBase : public MessagePump {
 
   virtual void ScheduleWork() OVERRIDE;
   virtual void ScheduleDelayedWork(const TimeTicks& delayed_work_time) OVERRIDE;
+  virtual void Shutdown() OVERRIDE;
 
  protected:
   virtual ~MessagePumpCFRunLoopBase();
diff --git a/base/message_loop/message_pump_mac.mm b/base/message_loop/message_pump_mac.mm
index a419d48..0dcfc23 100644
--- a/base/message_loop/message_pump_mac.mm
+++ b/base/message_loop/message_pump_mac.mm
@@ -207,6 +207,9 @@ void MessagePumpCFRunLoopBase::ScheduleDelayedWork(
   CFRunLoopTimerSetNextFireDate(delayed_work_timer_, delayed_work_fire_time_);
 }
 
+void MessagePumpCFRunLoopBase::Shutdown() {
+}
+
 // Called from the run loop.
 // static
 void MessagePumpCFRunLoopBase::RunDelayedWorkTimer(CFRunLoopTimerRef timer,
diff --git a/base/message_loop/message_pump_win.cc b/base/message_loop/message_pump_win.cc
index 36a3210..2365494 100644
--- a/base/message_loop/message_pump_win.cc
+++ b/base/message_loop/message_pump_win.cc
@@ -67,6 +67,10 @@ void MessagePumpWin::RunWithDispatcher(
   state_ = previous_state;
 }
 
+void MessagePumpWin::Run(Delegate* delegate) {
+  RunWithDispatcher(delegate, NULL);
+}
+
 void MessagePumpWin::Quit() {
   DCHECK(state_);
   state_->should_quit = true;
@@ -103,20 +107,32 @@ MessagePumpForUI::MessagePumpForUI()
 }
 
 MessagePumpForUI::~MessagePumpForUI() {
-  DestroyWindow(message_hwnd_);
-  UnregisterClass(MAKEINTATOM(atom_),
-                  GetModuleFromAddress(&WndProcThunk));
+  DCHECK(!atom_);
+  DCHECK(!message_hwnd_);
 }
 
 void MessagePumpForUI::ScheduleWork() {
   if (InterlockedExchange(&have_work_, 1))
     return;  // Someone else continued the pumping.
 
-  // Make sure the MessagePump does some work for us.
-  BOOL ret = PostMessage(message_hwnd_, kMsgHaveWork,
-                         reinterpret_cast<WPARAM>(this), 0);
-  if (ret)
-    return;  // There was room in the Window Message queue.
+  // If the attempt to acquire the lock fails, the message loop is shutting down
+  // and the message window is about to be destroyed as well.
+  if (!message_hwnd_lock_.Try())
+    return;
+
+  {
+    base::AutoLock lock(message_hwnd_lock_, base::AutoLock::AlreadyAcquired());
+
+    // Do nothing if the window has been destroyed already.
+    if (!message_hwnd_)
+      return;
+
+    // Make sure the MessagePump does some work for us.
+    if (PostMessage(message_hwnd_, kMsgHaveWork,
+                    reinterpret_cast<WPARAM>(this), 0)) {
+      return;
+    }
+  }
 
   // We have failed to insert a have-work message, so there is a chance that we
   // will starve tasks/timers while sitting in a nested message loop.  Nested
@@ -172,6 +188,24 @@ void MessagePumpForUI::ScheduleDelayedWork(const TimeTicks& delayed_work_time) {
                             MESSAGE_LOOP_PROBLEM_MAX);
 }
 
+void MessagePumpForUI::Shutdown() {
+  HWND message_hwnd;
+  {
+    base::AutoLock lock(message_hwnd_lock_);
+
+    // Let ScheduleWork() know that the window has been destoyed.
+    message_hwnd = message_hwnd_;
+    message_hwnd_ = NULL;
+  }
+
+  // Destoy the window and its class. The destructor checks whether
+  // |message_hwnd_| and |atom_| are destroyed, so the variables should be
+  // cleared here.
+  DestroyWindow(message_hwnd);
+  UnregisterClass(MAKEINTATOM(atom_), GetModuleFromAddress(&WndProcThunk));
+  atom_ = 0;
+}
+
 void MessagePumpForUI::PumpOutPendingPaintMessages() {
   // If we are being called outside of the context of Run, then don't try to do
   // any work.
@@ -478,7 +512,7 @@ void MessagePumpForIO::ScheduleWork() {
     return;  // Post worked perfectly.
 
   // See comment in MessagePumpForUI::ScheduleWork() for this error recovery.
-  InterlockedExchange(&have_work_, 0);  // Clarify that we didn't succeed.
+  InterlockedExchange(&have_work_, 0);
   UMA_HISTOGRAM_ENUMERATION("Chrome.MessageLoopProblem", COMPLETION_POST_ERROR,
                             MESSAGE_LOOP_PROBLEM_MAX);
 }
@@ -490,6 +524,9 @@ void MessagePumpForIO::ScheduleDelayedWork(const TimeTicks& delayed_work_time) {
   delayed_work_time_ = delayed_work_time;
 }
 
+void MessagePumpForIO::Shutdown() {
+}
+
 void MessagePumpForIO::RegisterIOHandler(HANDLE file_handle,
                                          IOHandler* handler) {
   ULONG_PTR key = HandlerToKey(handler, true);
diff --git a/base/message_loop/message_pump_win.h b/base/message_loop/message_pump_win.h
index dbb41bd..b492678 100644
--- a/base/message_loop/message_pump_win.h
+++ b/base/message_loop/message_pump_win.h
@@ -16,6 +16,7 @@
 #include "base/message_loop/message_pump_dispatcher.h"
 #include "base/message_loop/message_pump_observer.h"
 #include "base/observer_list.h"
+#include "base/synchronization/lock.h"
 #include "base/time.h"
 #include "base/win/scoped_handle.h"
 
@@ -45,8 +46,8 @@ class BASE_EXPORT MessagePumpWin : public MessagePump {
   void RunWithDispatcher(Delegate* delegate, MessagePumpDispatcher* dispatcher);
 
   // MessagePump methods:
-  virtual void Run(Delegate* delegate) { RunWithDispatcher(delegate, NULL); }
-  virtual void Quit();
+  virtual void Run(Delegate* delegate) OVERRIDE;
+  virtual void Quit() OVERRIDE;
 
  protected:
   struct RunState {
@@ -166,8 +167,9 @@ class BASE_EXPORT MessagePumpForUI : public MessagePumpWin {
   void SetMessageFilter(scoped_ptr<MessageFilter> message_filter);
 
   // MessagePump methods:
-  virtual void ScheduleWork();
-  virtual void ScheduleDelayedWork(const TimeTicks& delayed_work_time);
+  virtual void ScheduleWork() OVERRIDE;
+  virtual void ScheduleDelayedWork(const TimeTicks& delayed_work_time) OVERRIDE;
+  virtual void Shutdown() OVERRIDE;
 
   // Applications can call this to encourage us to process all pending WM_PAINT
   // messages.  This method will process all paint messages the Windows Message
@@ -194,6 +196,9 @@ class BASE_EXPORT MessagePumpForUI : public MessagePumpWin {
   // A hidden message-only window.
   HWND message_hwnd_;
 
+  // Protectes access to |message_hwnd_|.
+  base::Lock message_hwnd_lock_;
+
   scoped_ptr<MessageFilter> message_filter_;
 };
 
@@ -327,6 +332,7 @@ class BASE_EXPORT MessagePumpForIO : public MessagePumpWin {
   // MessagePump methods:
   virtual void ScheduleWork();
   virtual void ScheduleDelayedWork(const TimeTicks& delayed_work_time);
+  virtual void Shutdown() OVERRIDE;
 
   // Register the handler to be used when asynchronous IO for the given file
   // completes. The registration persists as long as |file_handle| is valid, so