Make sure that base::MessagePumpForUI from different modules are isolated from each other and add p…

base/message_pump_win.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/message_pump_win.h"
 
 #include <math.h>
 
 #include "base/message_loop.h"
 #include "base/metrics/histogram.h"
+#include "base/process_util.h"
+#include "base/stringprintf.h"
 #include "base/win/wrapped_window_proc.h"
 
+namespace {
+
+// The ID of the timer used by the UI message pump.
+const int kMessagePumpTimerId = 0;
+
+}  // namespace
+
 namespace base {
 
-static const wchar_t kWndClass[] = L"Chrome_MessagePumpWindow";
+static const wchar_t kWndClassFormat[] = L"Chrome_MessagePumpWindow%p";
 
 // 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;
 
 //-----------------------------------------------------------------------------
 // MessagePumpWin public:
 
 void MessagePumpWin::AddObserver(MessagePumpObserver* observer) {
   observers_.AddObserver(observer);
@@ skipping 49 matching lines @@
   int delay = static_cast<int>(timeout);
   if (delay < 0)
     delay = 0;
 
   return delay;
 }
 
 //-----------------------------------------------------------------------------
 // MessagePumpForUI public:
 
-MessagePumpForUI::MessagePumpForUI() {
+MessagePumpForUI::MessagePumpForUI()
+    : atom_(0),
+      instance_(NULL),
+      message_hwnd_(NULL) {
   InitMessageWnd();
 }
 
 MessagePumpForUI::~MessagePumpForUI() {
-  DestroyWindow(message_hwnd_);
-  UnregisterClass(kWndClass, GetModuleHandle(NULL));
+  if (message_hwnd_ != NULL)
+    DestroyWindow(message_hwnd_);
+
+  if (atom_ != 0)
+    UnregisterClass(reinterpret_cast<const char16*>(atom_), instance_);
 }
 
 void MessagePumpForUI::ScheduleWork() {
   if (InterlockedExchange(&have_work_, 1))
     return;  // Someone else continued the pumping.
 
   // Make sure the MessagePump does some work for us.
-  PostMessage(message_hwnd_, kMsgHaveWork, reinterpret_cast<WPARAM>(this), 0);
+  PostMessage(message_hwnd_, kMsgHaveWork, 0, 0);
 }
 
 void MessagePumpForUI::ScheduleDelayedWork(const TimeTicks& delayed_work_time) {
   //
   // 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
   // mechanism because the application can enter modal windows loops where it
   // is not running our MessageLoop; the only way to have our timers fire in
   // these cases is to post messages there.
   //
@@ skipping 12 matching lines @@
   //
   delayed_work_time_ = delayed_work_time;
 
   int delay_msec = GetCurrentDelay();
   DCHECK_GE(delay_msec, 0);
   if (delay_msec < USER_TIMER_MINIMUM)
     delay_msec = USER_TIMER_MINIMUM;
 
   // 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).
-  SetTimer(message_hwnd_, reinterpret_cast<UINT_PTR>(this), delay_msec, NULL);
+  SetTimer(message_hwnd_, kMessagePumpTimerId, delay_msec, NULL);
 }
 
 void MessagePumpForUI::PumpOutPendingPaintMessages() {
   // If we are being called outside of the context of Run, then don't try to do
   // any work.
   if (!state_)
     return;
 
   // Create a mini-message-pump to force immediate processing of only Windows
   // WM_PAINT messages.  Don't provide an infinite loop, but do enough peeking
@@ skipping 12 matching lines @@
   // Histogram what was really being used, to help to adjust kMaxPeekCount.
   DHISTOGRAM_COUNTS("Loop.PumpOutPendingPaintMessages Peeks", peek_count);
 }
 
 //-----------------------------------------------------------------------------
 // MessagePumpForUI private:
 
 // static
 LRESULT CALLBACK MessagePumpForUI::WndProcThunk(
     HWND hwnd, UINT message, WPARAM wparam, LPARAM lparam) {
-  switch (message) {
-    case kMsgHaveWork:
-      reinterpret_cast<MessagePumpForUI*>(wparam)->HandleWorkMessage();
-      break;
-    case WM_TIMER:
-      reinterpret_cast<MessagePumpForUI*>(wparam)->HandleTimerMessage();
-      break;
+  // Retrieve |this| from the user data, associated with the window.
+  MessagePumpForUI* self = reinterpret_cast<MessagePumpForUI*>(
+      GetWindowLongPtr(hwnd, GWLP_USERDATA));
+  if (self != NULL) {

[jar (doing other things), 2012/05/15 21:39:42]

Perchance we should not check the value of |self|, and should instead crash on
line 186 or 190 if we don't have a value to service the cases?

[alexeypa (please no reviews), 2012/05/15 22:10:27]

|self| should never be NULL except for WM_CREATE message. So yes, we should
probably crash instead of silently ignoring the error.

+    switch (message) {
+      case kMsgHaveWork:
+        self->HandleWorkMessage();
+        break;
+      case WM_TIMER:
+        DCHECK(wparam == kMessagePumpTimerId);
+        self->HandleTimerMessage();
+        break;
+    }
   }
   return DefWindowProc(hwnd, message, wparam, lparam);
 }
 
 void MessagePumpForUI::DoRunLoop() {
   // 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):
   //    * Sent messages
   //    * Posted messages
@@ skipping 22 matching lines @@
     if (state_->should_quit)
       break;
 
     more_work_is_plausible |=
         state_->delegate->DoDelayedWork(&delayed_work_time_);
     // If we did not process any delayed work, then we can assume that our
     // existing WM_TIMER if any will fire when delayed work should run.  We
     // 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(message_hwnd_, kMessagePumpTimerId);
     if (state_->should_quit)
       break;
 
     if (more_work_is_plausible)
       continue;
 
     more_work_is_plausible = state_->delegate->DoIdleWork();
     if (state_->should_quit)
       break;
 
     if (more_work_is_plausible)
       continue;
 
     WaitForWork();  // Wait (sleep) until we have work to do again.
   }
 }
 
 void MessagePumpForUI::InitMessageWnd() {
-  HINSTANCE hinst = GetModuleHandle(NULL);
+  // Register a unique window class for each instance of UI pump.
+  string16 class_name = base::StringPrintf(kWndClassFormat, this);
+  WNDPROC window_procedure = &base::win::WrappedWindowProc<WndProcThunk>;
 
-  WNDCLASSEX wc = {0};
+  // RegisterClassEx uses a handle of the module containing the window procedure
+  // to distinguish identically named classes registered in different modules.
+  instance_ = GetModuleFromAddress(window_procedure);
+
+  WNDCLASSEXW wc = {0};
   wc.cbSize = sizeof(wc);
-  wc.lpfnWndProc = base::win::WrappedWindowProc<WndProcThunk>;
-  wc.hInstance = hinst;
-  wc.lpszClassName = kWndClass;
-  RegisterClassEx(&wc);
+  wc.lpfnWndProc = window_procedure;
+  wc.hInstance = instance_;
+  wc.lpszClassName = class_name.c_str();
+  atom_ = RegisterClassEx(&wc);
+  if (atom_ == 0) {
+    DCHECK(atom_);

[alexeypa (please no reviews), 2012/05/15 22:10:27]

We should also CHECK here.

+    return;
+  }
 
-  message_hwnd_ =
-      CreateWindow(kWndClass, 0, 0, 0, 0, 0, 0, HWND_MESSAGE, 0, hinst, 0);
-  DCHECK(message_hwnd_);
+  // Create the message-only window.
+  message_hwnd_ = CreateWindow(
+      reinterpret_cast<const char16*>(atom_), 0, 0, 0, 0, 0, 0, HWND_MESSAGE, 0,
+      instance_, 0);
+  if (message_hwnd_ == NULL) {
+    DCHECK(message_hwnd_);
+    return;

[jar (doing other things), 2012/05/15 21:39:42]

These return on error condition appear to mask a significant fault, as we won't
have set the |this| pointer for retrieval, and hence all handling of tasks and
timers in nested message loops will silently be ignored.... simulating a hang
:-(

Shouldn't we just CHECK() and crash here, and on line 269?

[alexeypa (please no reviews), 2012/05/15 22:10:27]

Those DCHECKs migrated from the previous version of the code to this CL. I think
CHECK is a better way to handle errors returned by RegisterClassEx and
CreateWindow. 

Now, ignoring the errors returned by RegisterClass(Ex) (and, maybe,
CreateWindow) seems to be a more common pattern in Chromium code than we would
want.

+  }
+
+  // Store |this| so that the window procedure could retrieve it later.
+  SetWindowLongPtr(message_hwnd_,

[alexeypa (please no reviews), 2012/05/15 22:10:27]

The return value of SetWindowLongPtr should be checked too.

+                   GWLP_USERDATA,
+                   reinterpret_cast<LONG_PTR>(this));
 }
 
 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.
   int delay = GetCurrentDelay();
   if (delay < 0)  // Negative value means no timers waiting.
     delay = INFINITE;
 
   DWORD result;
@@ skipping 38 matching lines @@
   // messages that may be in the Windows message queue.
   ProcessPumpReplacementMessage();
 
   // Now give the delegate a chance to do some work.  He'll let us know if he
   // needs to do more work.
   if (state_->delegate->DoWork())
     ScheduleWork();
 }
 
 void MessagePumpForUI::HandleTimerMessage() {
-  KillTimer(message_hwnd_, reinterpret_cast<UINT_PTR>(this));
+  KillTimer(message_hwnd_, kMessagePumpTimerId);
 
   // 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
   // that sort.
   if (!state_)
     return;
 
   state_->delegate->DoDelayedWork(&delayed_work_time_);
   if (!delayed_work_time_.is_null()) {
     // A bit gratuitous to set delayed_work_time_ again, but oh well.
@@ skipping 260 matching lines @@
 
 void MessagePumpForIO::WillProcessIOEvent() {
   FOR_EACH_OBSERVER(IOObserver, io_observers_, WillProcessIOEvent());
 }
 
 void MessagePumpForIO::DidProcessIOEvent() {
   FOR_EACH_OBSERVER(IOObserver, io_observers_, DidProcessIOEvent());
 }
 
 }  // namespace base