Fixed the racy code around the message-only window in base::MessagePumpForUI on Windows.

base/message_pump_win.cc

Index: base/message_pump_win.cc
diff --git a/base/message_pump_win.cc b/base/message_pump_win.cc
index fb962caaa2f45e2f83792f10dc3fe7bd147e5d23..4961c132d9f4b41ae2f351578d0791e222846c25 100644
--- a/base/message_pump_win.cc
+++ b/base/message_pump_win.cc
@@ -9,8 +9,6 @@
 #include "base/debug/trace_event.h"
 #include "base/message_loop.h"
 #include "base/metrics/histogram.h"
-#include "base/process_util.h"
-#include "base/win/wrapped_window_proc.h"
 
 namespace {
 
@@ -25,12 +23,13 @@ enum MessageLoopProblems {
 
 namespace base {
 
-static const wchar_t kWndClass[] = L"Chrome_MessagePumpWindow";
-
 // Message sent to get an additional time slice for pumping (processing) another
 // task (a series of such messages creates a continuous task pump).
 static const int kMsgHaveWork = WM_USER + 1;
 
+// Used by MessagePumpUI to wake up the thread and check any pending timers.
+static const int kTimerId = 1;
+
 //-----------------------------------------------------------------------------
 // MessagePumpWin public:
 
@@ -96,25 +95,30 @@ int MessagePumpWin::GetCurrentDelay() const {
 // MessagePumpForUI public:
 
 MessagePumpForUI::MessagePumpForUI()
-    : instance_(NULL),
-      message_filter_(new MessageFilter) {
-  InitMessageWnd();
+    : message_filter_(new MessageFilter),
+      window_(new win::MessageWindow()) {
+  CHECK(window_->Create(this));
 }
 
 MessagePumpForUI::~MessagePumpForUI() {
-  DestroyWindow(message_hwnd_);
-  UnregisterClass(kWndClass, instance_);
 }
 
 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.
+  BOOL result = FALSE;
+
+  {
+    // Take the lock to make sure the window will not be destroyed until
+    // PostMessage() returns below.
+    AutoLock lock(window_lock_);

[darin (slow to review), 2013/05/25 21:23:17]

Is it perhaps worth avoiding this lock when ScheduleWork is called on the same
thread as the MessagePumpForUI constructor?

Have you done any kind of performance testing?

[alexeypa (please no reviews), 2013/05/29 18:11:52]

Just an FYI.

I wrote a test that posts tasks to the loop keeping at most one task in the
queue (to make sure that we hit ScheduleWork() every time). The test repeats 20
warmup-and-measure loops. "Warmup" posts 100,000 tasks. "Measure" posts another
1,000,000 tasks.

The thread was affinitized to one logical core. It was running at real-time
priority.

The version with the lock was running about 2% slower. The version that was
checking thread ID (to avoid taking a lock on the same thread) was about 1%
slower.

Without the lock:

[0528/113104:INFO:rabbit.cc(100)] mean: 1.92923
[0528/113104:INFO:rabbit.cc(109)] deviation: 0.0138212

With the lock:

[0528/112843:INFO:rabbit.cc(100)] mean: 1.97287
[0528/112843:INFO:rabbit.cc(109)] deviation: 0.0171741

With thread ID check:

[0528/123213:INFO:rabbit.cc(100)] mean: 1.94465
[0528/123213:INFO:rabbit.cc(109)] deviation: 0.0481169

+    if (window_) {
+      // Make sure the MessagePump does some work for us.
+      if (PostMessage(window_->hwnd(), kMsgHaveWork, 0, 0))

[darin (slow to review), 2013/05/24 21:42:58]

Perhaps MessageWindow should be reference counted instead?  If we just need to
retain the HWND across a PostMessage call, then InterlockedIncrement +
InterlockedDecrement seems cheaper than an AutoLock.

[alexeypa (please no reviews), 2013/05/24 22:22:48]

No, the problem is that DestroyWindow() should be called on the right thread.
WillDestroyCurrentMessageLoop() is the very last call on that thread, so it
should wait until a concurrent PostMessage() call returns on a different thread.
Because otherwise by the time PostMessage() returns the thread owning the window
might be destroyed already. 

+        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
@@ -130,6 +134,8 @@ void MessagePumpForUI::ScheduleWork() {
 }
 
 void MessagePumpForUI::ScheduleDelayedWork(const TimeTicks& delayed_work_time) {
+  DCHECK(window_->CalledOnValidThread());
+
   //
   // We would *like* to provide high resolution timers.  Windows timers using
   // SetTimer() have a 10ms granularity.  We have to use WM_TIMER as a wakeup
@@ -159,8 +165,7 @@ void MessagePumpForUI::ScheduleDelayedWork(const TimeTicks& delayed_work_time) {
 
   // Create a WM_TIMER event that will wake us up to check for any pending
   // timers (in case we are running within a nested, external sub-pump).
-  BOOL ret = SetTimer(message_hwnd_, reinterpret_cast<UINT_PTR>(this),
-                      delay_msec, NULL);
+  BOOL ret = SetTimer(window_->hwnd(), kTimerId, delay_msec, NULL);
   if (ret)
     return;
   // If we can't set timers, we are in big trouble... but cross our fingers for
@@ -170,6 +175,15 @@ void MessagePumpForUI::ScheduleDelayedWork(const TimeTicks& delayed_work_time) {
                             MESSAGE_LOOP_PROBLEM_MAX);
 }
 
+void MessagePumpForUI::WillDestroyCurrentMessageLoop() {
+  DCHECK(window_->CalledOnValidThread());
+
+  // Synchronize with MessagePumpForUI::ScheduleWork() that can access |window_|
+  // on arbitrary thread.
+  AutoLock lock(window_lock_);
+  window_.reset();
+}
+
 void MessagePumpForUI::PumpOutPendingPaintMessages() {
   // If we are being called outside of the context of Run, then don't try to do
   // any work.
@@ -197,21 +211,28 @@ void MessagePumpForUI::PumpOutPendingPaintMessages() {
 //-----------------------------------------------------------------------------
 // MessagePumpForUI private:
 
-// static
-LRESULT CALLBACK MessagePumpForUI::WndProcThunk(
-    HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam) {
+bool MessagePumpForUI::HandleMessage(HWND hwnd,
+                                     UINT message,
+                                     WPARAM wparam,
+                                     LPARAM lparam,
+                                     LRESULT* result) {
   switch (message) {
     case kMsgHaveWork:
-      reinterpret_cast<MessagePumpForUI*>(wparam)->HandleWorkMessage();
+      HandleWorkMessage();
       break;
+
     case WM_TIMER:
-      reinterpret_cast<MessagePumpForUI*>(wparam)->HandleTimerMessage();
+      HandleTimerMessage();
       break;
   }
-  return DefWindowProc(hwnd, message, wparam, lparam);
+
+  // Do default processing for all messages.
+  return false;
 }
 
 void MessagePumpForUI::DoRunLoop() {
+  DCHECK(window_->CalledOnValidThread());
+
   // IF this was just a simple PeekMessage() loop (servicing all possible work
   // queues), then Windows would try to achieve the following order according
   // to MSDN documentation about PeekMessage with no filter):
@@ -249,7 +270,7 @@ void MessagePumpForUI::DoRunLoop() {
     // don't want to disturb that timer if it is already in flight.  However,
     // if we did do all remaining delayed work, then lets kill the WM_TIMER.
     if (more_work_is_plausible && delayed_work_time_.is_null())
-      KillTimer(message_hwnd_, reinterpret_cast<UINT_PTR>(this));
+      KillTimer(window_->hwnd(), kTimerId);
     if (state_->should_quit)
       break;
 
@@ -267,20 +288,6 @@ void MessagePumpForUI::DoRunLoop() {
   }
 }
 
-void MessagePumpForUI::InitMessageWnd() {
-  WNDCLASSEX wc = {0};
-  wc.cbSize = sizeof(wc);
-  wc.lpfnWndProc = base::win::WrappedWindowProc<WndProcThunk>;
-  wc.hInstance = base::GetModuleFromAddress(wc.lpfnWndProc);
-  wc.lpszClassName = kWndClass;
-  instance_ = wc.hInstance;
-  RegisterClassEx(&wc);
-
-  message_hwnd_ =
-      CreateWindow(kWndClass, 0, 0, 0, 0, 0, 0, HWND_MESSAGE, 0, instance_, 0);
-  DCHECK(message_hwnd_);
-}
-
 void MessagePumpForUI::WaitForWork() {
   // Wait until a message is available, up to the time needed by the timer
   // manager to fire the next set of timers.
@@ -338,7 +345,7 @@ void MessagePumpForUI::HandleWorkMessage() {
 }
 
 void MessagePumpForUI::HandleTimerMessage() {
-  KillTimer(message_hwnd_, reinterpret_cast<UINT_PTR>(this));
+  KillTimer(window_->hwnd(), kTimerId);
 
   // If we are being called outside of the context of Run, then don't do
   // anything.  This could correspond to a MessageBox call or something of
@@ -382,7 +389,7 @@ bool MessagePumpForUI::ProcessMessageHelper(const MSG& msg) {
   }
 
   // While running our main message pump, we discard kMsgHaveWork messages.
-  if (msg.message == kMsgHaveWork && msg.hwnd == message_hwnd_)
+  if (msg.message == kMsgHaveWork && msg.hwnd == window_->hwnd())
     return ProcessPumpReplacementMessage();
 
   if (CallMsgFilter(const_cast<MSG*>(&msg), kMessageFilterCode))
@@ -429,7 +436,7 @@ bool MessagePumpForUI::ProcessPumpReplacementMessage() {
   }
 
   DCHECK(!have_message || kMsgHaveWork != msg.message ||
-         msg.hwnd != message_hwnd_);
+         msg.hwnd != window_->hwnd());
 
   // Since we discarded a kMsgHaveWork message, we must update the flag.
   int old_have_work = InterlockedExchange(&have_work_, 0);