Allow multiple base::MessagePumpForUI instances to be created simultanenously on Windows.

Allow multiple base::MessagePumpForUI instances to be created simultanenously on Windows.

The current implementation of base::MessagePumpForUI on Windows registers a window class with a predefined name in order to create a message-only window. The window class is unregistered when base::MessagePumpForUI is deleted. This causes issues when two or more instances of base::MessagePumpForUI are created/destroyed simultanenously on different threads. For instance once thread can unregister the window class right before the other thread is trying to create a window using this class.

The CL addresses this problem by switching MessageWindow to implement a message-only window. It also moves MessageWindow from remoting/host/win to base/win along with the corresponding unit test.

MessageWindow registers a uniquely named window class per MessageWindow instance making sure that different MessageWindow objects do not share any resources. In the future this can be optimized further by registering a common window class shared by all MessageWindow objects in a thread-safe manner (by using LazyInstance for example).

BUG=241939
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201955

[alexeypa (please no reviews), 2013-05-21 21:56:44]

PTAL.

base/* - darin@
remoting/* and the rest - wez@

[Wez, 2013-05-21 22:57:54]

remoting/ LGTM

[darin (slow to review), 2013-05-22 22:53:23]

https://chromiumcodereview.appspot.com/15261005/diff/8001/base/message_pump_w...
File base/message_pump_win.cc (left):

https://chromiumcodereview.appspot.com/15261005/diff/8001/base/message_pump_w...
base/message_pump_win.cc:275: wc.lpszClassName = kWndClass;
wouldn't it be a simpler change to just give each window class a unique name? 
just use StaticAtomicSequenceNumber?

[alexeypa (please no reviews), 2013-05-22 23:04:48]

https://chromiumcodereview.appspot.com/15261005/diff/8001/base/message_pump_w...
File base/message_pump_win.cc (left):

https://chromiumcodereview.appspot.com/15261005/diff/8001/base/message_pump_w...
base/message_pump_win.cc:275: wc.lpszClassName = kWndClass;
On 2013/05/22 22:53:24, darin wrote:
> wouldn't it be a simpler change to just give each window class a unique name? 
> just use StaticAtomicSequenceNumber?

This is pretty much what this CL does. It is just that we already had an
implementation of a generic message only window class in src/remoting, so I
moved it base.

I think it is worth moving that code to base to avoid having slightly different
implementations of exactly the same thing.

[darin (slow to review), 2013-05-23 00:07:13]

OK, makes sense.

https://chromiumcodereview.appspot.com/15261005/diff/8001/base/message_pump_w...
File base/message_pump_win.cc (right):

https://chromiumcodereview.appspot.com/15261005/diff/8001/base/message_pump_w...
base/message_pump_win.cc:109: reinterpret_cast<WPARAM>(this), 0);
it looks like it is no longer necessary to stuff |this| in the WPARAM field.

https://chromiumcodereview.appspot.com/15261005/diff/8001/base/message_pump_w...
base/message_pump_win.cc:156: BOOL ret = SetTimer(window_->hwnd(),
reinterpret_cast<UINT_PTR>(this),
ditto.

https://chromiumcodereview.appspot.com/15261005/diff/8001/base/win/message_wi...
File base/win/message_window.cc (right):

https://chromiumcodereview.appspot.com/15261005/diff/8001/base/win/message_wi...
base/win/message_window.cc:22: class_name_ =
base::StringPrintf(kClassNameFormat, this);
can't the address of this be reused?  hence my suggestion of using an atomic
sequence number.

[alexeypa (please no reviews), 2013-05-23 17:45:53]

https://chromiumcodereview.appspot.com/15261005/diff/8001/base/message_pump_w...
File base/message_pump_win.cc (right):

https://chromiumcodereview.appspot.com/15261005/diff/8001/base/message_pump_w...
base/message_pump_win.cc:109: reinterpret_cast<WPARAM>(this), 0);
On 2013/05/23 00:07:14, darin wrote:
> it looks like it is no longer necessary to stuff |this| in the WPARAM field.

Done.

https://chromiumcodereview.appspot.com/15261005/diff/8001/base/message_pump_w...
base/message_pump_win.cc:156: BOOL ret = SetTimer(window_->hwnd(),
reinterpret_cast<UINT_PTR>(this),
On 2013/05/23 00:07:14, darin wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/15261005/diff/8001/base/win/message_wi...
File base/win/message_window.cc (right):

https://chromiumcodereview.appspot.com/15261005/diff/8001/base/win/message_wi...
base/win/message_window.cc:22: class_name_ =
base::StringPrintf(kClassNameFormat, this);
On 2013/05/23 00:07:14, darin wrote:
> can't the address of this be reused?  hence my suggestion of using an atomic
> sequence number.

The window class is unregistered in the MessageWindow's destructor. By the time
the address is reused the window class will be long gone.

[darin (slow to review), 2013-05-23 18:14:45]

On 2013/05/23 17:45:53, alexeypa wrote:
>
https://chromiumcodereview.appspot.com/15261005/diff/8001/base/win/message_wi...
> base/win/message_window.cc:22: class_name_ =
> base::StringPrintf(kClassNameFormat, this);
> On 2013/05/23 00:07:14, darin wrote:
> > can't the address of this be reused?  hence my suggestion of using an atomic
> > sequence number.
> 
> The window class is unregistered in the MessageWindow's destructor. By the
time
> the address is reused the window class will be long gone.

oh yeah, of course.

[darin (slow to review), 2013-05-23 18:20:51]

LGTM w/ nits:

https://chromiumcodereview.appspot.com/15261005/diff/25001/base/message_pump_...
File base/message_pump_win.cc (right):

https://chromiumcodereview.appspot.com/15261005/diff/25001/base/message_pump_...
base/message_pump_win.cc:100: CHECK(window_->Create(this)) << "Couldn't create a
message-only window.";
nit: No need to bake this text string into release builds.  I would just leave
it out or switch to a DCHECK.  The existing code just uses a DCHECK.

https://chromiumcodereview.appspot.com/15261005/diff/25001/base/message_pump_...
File base/message_pump_win.h (right):

https://chromiumcodereview.appspot.com/15261005/diff/25001/base/message_pump_...
base/message_pump_win.h:199: scoped_ptr<base::win::MessageWindow> window_;
nit: no need for base::

[alexeypa (please no reviews), 2013-05-23 19:02:29]

https://chromiumcodereview.appspot.com/15261005/diff/25001/base/message_pump_...
File base/message_pump_win.cc (right):

https://chromiumcodereview.appspot.com/15261005/diff/25001/base/message_pump_...
base/message_pump_win.cc:100: CHECK(window_->Create(this)) << "Couldn't create a
message-only window.";
On 2013/05/23 18:20:51, darin wrote:
> nit: No need to bake this text string into release builds.  I would just leave
> it out or switch to a DCHECK.  The existing code just uses a DCHECK.

Done.

https://chromiumcodereview.appspot.com/15261005/diff/25001/base/message_pump_...
File base/message_pump_win.h (right):

https://chromiumcodereview.appspot.com/15261005/diff/25001/base/message_pump_...
base/message_pump_win.h:199: scoped_ptr<base::win::MessageWindow> window_;
On 2013/05/23 18:20:51, darin wrote:
> nit: no need for base::

Done.

[commit-bot: I haz the power, 2013-05-23 19:06:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/15261005/37001

[commit-bot: I haz the power, 2013-05-23 19:20:00]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac&number...

[commit-bot: I haz the power, 2013-05-23 19:26:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/alexeypa@chromium.org/15261005/37001

[commit-bot: I haz the power, 2013-05-24 01:34:53]

Change committed as 201955