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

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

The current implementation of base::MessagePumpForUI has a couple of problems:
- A window class registered for the message-only window has a predefined name. This leads to issues like registering the same class multiple times or unregistering it prematurely.
- base::MessagePumpForUI is a ref-counted class and can be destroyed on a different thread. As the result DestroyWindow() can be called on a wrong thread.

This CL introduces the following changes to address the above problems:
- MessageWindow is used to create the message-only window in base::MessagePumpForUI. This class registers a unique window class name for every window to make sure multiple message-only windows can exist at the same time. The implementation of MessageWindow is moved to src/base/win along with the corresponding unit test.
- 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

[alexeypa (please no reviews), 2013-05-24 19:52:58]

This is a 2nd attempt to land https://chromiumcodereview.appspot.com/15261005/.
The previous CL was reverted because it broke one of the XP debug bots.
MessageWindow instances were deleted on a wrong thread.

Patch set #1 - the original CL.
Patch set #2 - the fix for base::MessagePumpForUI deleting MessageWindow on a
wrong thread.

Please take a look:
- src/base/* - darin@
- the rest - wez@.

PS. The fact that base::MessagePump is ref-counted does not look right. PostXxx
methods still reside in base::MessageLoop. It should be possible to make
base::MessagePump non-refcounted with proper synchronisation. Not for this CL
though.

PS#2. Unfortunately there is no XP try bot.

[darin (slow to review), 2013-05-24 21:42:57]

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

https://chromiumcodereview.appspot.com/16020005/diff/2001/base/message_pump_w...
base/message_pump_win.cc:118: if (PostMessage(window_->hwnd(), kMsgHaveWork, 0,
0))
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.

[darin (slow to review), 2013-05-24 21:43:44]

Another solution might be to force the destruction of the MessagePump to its
originating thread.  You can do that by leveraging the second template argument
to RefCountedThreadSafe.

[alexeypa (please no reviews), 2013-05-24 22:22:40]

On 2013/05/24 21:43:44, darin wrote:
> Another solution might be to force the destruction of the MessagePump to its
> originating thread.  You can do that by leveraging the second template
argument
> to RefCountedThreadSafe.

To make this work we need to make sure that the MessageLoop's thread is still
alive, which might not be the case.

[alexeypa (please no reviews), 2013-05-24 22:22:47]

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

https://chromiumcodereview.appspot.com/16020005/diff/2001/base/message_pump_w...
base/message_pump_win.cc:118: if (PostMessage(window_->hwnd(), kMsgHaveWork, 0,
0))
On 2013/05/24 21:42:58, darin wrote:
> 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.

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.

[darin (slow to review), 2013-05-25 21:15:21]

On 2013/05/24 22:22:47, alexeypa wrote:
>
https://chromiumcodereview.appspot.com/16020005/diff/2001/base/message_pump_w...
> File base/message_pump_win.cc (right):
> 
>
https://chromiumcodereview.appspot.com/16020005/diff/2001/base/message_pump_w...
> base/message_pump_win.cc:118: if (PostMessage(window_->hwnd(), kMsgHaveWork,
0,
> 0))
> On 2013/05/24 21:42:58, darin wrote:
> > 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.
> 
> 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.

Great point.  I overlooked that DestroyWindow needs to happen on the thread that
created the HWND.


>On 2013/05/24 21:43:44, darin wrote:
>> Another solution might be to force the destruction of the 
>> MessagePump to its originating thread.  You can do that by
>> leveraging the second template argument to RefCountedThreadSafe.
>
>To make this work we need to make sure that the MessageLoop's thread
>is still alive, which might not be the case.

Perhaps we do have that guarantee.  Consumers of MessageLoop
already take care to synchronize destruction of the ML with
calls to PostTask.  See message_loop_proxy_impl.cc and
browser_thread_impl.cc.

I'm just a little worried about adding more locks to ScheduleWork
since it is a busy code path.

[darin (slow to review), 2013-05-25 21:17:24]

On 2013/05/25 21:15:21, darin wrote:
...
> >On 2013/05/24 21:43:44, darin wrote:
> >> Another solution might be to force the destruction of the 
> >> MessagePump to its originating thread.  You can do that by
> >> leveraging the second template argument to RefCountedThreadSafe.
> >
> >To make this work we need to make sure that the MessageLoop's thread
> >is still alive, which might not be the case.
> 
> Perhaps we do have that guarantee.  Consumers of MessageLoop
> already take care to synchronize destruction of the ML with
> calls to PostTask.  See message_loop_proxy_impl.cc and
> browser_thread_impl.cc.
> 
> I'm just a little worried about adding more locks to ScheduleWork
> since it is a busy code path.

Oh wait... [seconds after hitting send] it occurs to me that this
could lead to a dead-lock.  PostTask calling PostTask is no good.

[darin (slow to review), 2013-05-25 21:23:16]

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

https://chromiumcodereview.appspot.com/16020005/diff/2001/base/message_pump_w...
base/message_pump_win.cc:115: AutoLock lock(window_lock_);
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:51]

OK, here is another version that does not use an explicit lock.

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

https://chromiumcodereview.appspot.com/16020005/diff/2001/base/message_pump_w...
base/message_pump_win.cc:115: AutoLock lock(window_lock_);
On 2013/05/25 21:23:17, darin wrote:
> 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?

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

[alexeypa (please no reviews), 2013-05-30 20:29:20]

Darin, ping.

[alexeypa (please no reviews), 2013-06-06 19:29:26]

Ping!

[Wez, 2013-06-06 20:20:32]

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

https://chromiumcodereview.appspot.com/16020005/diff/27001/base/message_pump_...
base/message_pump_win.cc:30: VOID CALLBACK DummyApc(ULONG_PTR) {}
nit: Add a comment explaining what this dummy is used for.

https://chromiumcodereview.appspot.com/16020005/diff/27001/base/message_pump_...
base/message_pump_win.cc:155: // According to the UMA metrics posting an I/O
completing packet has very low
typo: completing -> completion

https://chromiumcodereview.appspot.com/16020005/diff/27001/base/message_pump_...
base/message_pump_win.cc:211: kPumpIdle) == kPumpHaveWork) {
Shouldn't this be != kPumpDisabled?

https://chromiumcodereview.appspot.com/16020005/diff/27001/base/message_pump_...
base/message_pump_win.cc:225: // At this point the pump is diabled and other
threads exited ScheduleWork().
typo: disabled

https://chromiumcodereview.appspot.com/16020005/diff/27001/base/message_pump_...
base/message_pump_win.cc:364: return;
nit: Add a comment explaining that we just use IO_COMPLETION to kick us out of
wait for work via APC, hence nothing to do here?

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

https://chromiumcodereview.appspot.com/16020005/diff/27001/base/message_pump_...
base/message_pump_win.h:74: // execution of tasks when a native message pump is
running.
It's also used to indicate whether the queue is "disabled", so best to mention
this in the comment, otherwise it's not clear why this is a "state" rather than
a bool.

[alexeypa (please no reviews), 2013-06-07 15:33:08]

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

https://chromiumcodereview.appspot.com/16020005/diff/27001/base/message_pump_...
base/message_pump_win.cc:30: VOID CALLBACK DummyApc(ULONG_PTR) {}
On 2013/06/06 20:20:33, Wez wrote:
> nit: Add a comment explaining what this dummy is used for.

Done.

https://chromiumcodereview.appspot.com/16020005/diff/27001/base/message_pump_...
base/message_pump_win.cc:155: // According to the UMA metrics posting an I/O
completing packet has very low
On 2013/06/06 20:20:33, Wez wrote:
> typo: completing -> completion

Done.

https://chromiumcodereview.appspot.com/16020005/diff/27001/base/message_pump_...
base/message_pump_win.cc:211: kPumpIdle) == kPumpHaveWork) {
On 2013/06/06 20:20:33, Wez wrote:
> Shouldn't this be != kPumpDisabled?

No. In most cases it will switch |pump_state_| from |kPumpIdle| directly to
|kPumpDisabled|. This will stop other threads from doying any work in
MessagePumpForUI::ScheduleWork().

https://chromiumcodereview.appspot.com/16020005/diff/27001/base/message_pump_...
base/message_pump_win.cc:225: // At this point the pump is diabled and other
threads exited ScheduleWork().
On 2013/06/06 20:20:33, Wez wrote:
> typo: disabled

Done.

https://chromiumcodereview.appspot.com/16020005/diff/27001/base/message_pump_...
base/message_pump_win.cc:364: return;
On 2013/06/06 20:20:33, Wez wrote:
> nit: Add a comment explaining that we just use IO_COMPLETION to kick us out of
> wait for work via APC, hence nothing to do here?

Done.

[alexeypa (please no reviews), 2013-06-10 18:47:55]

brettw@ could you please review this CL instead of darin@? Unfortunately I could
not get a hold of him for the last two weeks despite a few pings (here and over
email).

The brief history of this CL:

- It started as https://chromiumcodereview.appspot.com/15261005/, was landed and
reverted (see https://chromiumcodereview.appspot.com/16020005/#msg1). Patch #1
is the original CL.
- Darin was concerned about the lock, so Patch #3 removes it, using an APC to
synchronize two threads during message loop destruction. (see
https://chromiumcodereview.appspot.com/16020005/#msg9).

[darin (slow to review), 2013-06-10 18:53:36]

On 2013/06/10 18:47:55, alexeypa wrote:
> brettw@ could you please review this CL instead of darin@? Unfortunately I
could
> not get a hold of him for the last two weeks despite a few pings (here and
over
> email).
> 
> The brief history of this CL:
> 
> - It started as https://chromiumcodereview.appspot.com/15261005/, was landed
and
> reverted (see https://chromiumcodereview.appspot.com/16020005/#msg1). Patch #1
> is the original CL.
> - Darin was concerned about the lock, so Patch #3 removes it, using an APC to
> synchronize two threads during message loop destruction. (see
> https://chromiumcodereview.appspot.com/16020005/#msg9).

Sorry for the delays, I will review this today.  I saw the feedback from Wez
last
week and was waitign on your reply to his concerns.

[darin (slow to review), 2013-06-10 19:28:44]

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

https://chromiumcodereview.appspot.com/16020005/diff/45001/base/message_pump_...
base/message_pump_win.cc:150: InterlockedExchange(&pump_state_, kPumpIdle);
nit: Shouldn't this use InterlockedCompareExchange(&pump_state_, kPumpIdle,
kPumpHaveWork) so that you don't mistakenly clobber kPumpDisabled?

Are you sure using a tri-state variable here is best?  Maybe it would be cleaner
to have two variables, one for indicating work and the other for indicating
shutdown?

https://chromiumcodereview.appspot.com/16020005/diff/45001/base/message_pump_...
base/message_pump_win.cc:152: // Try to wake up the message loop thread by
posting an APC. This might not
I'm not sure I fully understand the conditions that lead to reaching this code. 
I also worry a little bit since sometimes other code runs nested message loops
on our thread not using our MessageLoop infrastructure.  For example, a library
function might implement its own nested message loop.  I'm not sure what it
means for our APC to run in that context.

https://chromiumcodereview.appspot.com/16020005/diff/45001/base/message_pump_...
base/message_pump_win.cc:344: MWMO_ALERTABLE | MWMO_INPUTAVAILABLE);
I'm a little worried that making this call alertable might result in wakeups
caused by other sources.  What does it mean if WaitForWork() returns early in
those cases (i.e., when we are not shutting down)?  It is probably safe to do
another pass through DoRunLoop().

[darin (slow to review), 2013-06-10 19:29:18]

+cpu for QueueUserAPC review

[alexeypa (please no reviews), 2013-06-10 20:16:11]

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

https://chromiumcodereview.appspot.com/16020005/diff/45001/base/message_pump_...
base/message_pump_win.cc:150: InterlockedExchange(&pump_state_, kPumpIdle);
On 2013/06/10 19:28:45, darin wrote:
> nit: Shouldn't this use InterlockedCompareExchange(&pump_state_, kPumpIdle,
> kPumpHaveWork) so that you don't mistakenly clobber kPumpDisabled?

No, at this point |pump_state_| is guaranteed to be |kPumpHaveWork| and the
current thread is the only thread that is allowed to modify |pump_state_|: 

- threads posting work and calling ScheduleWork() will bail out.
- the message loop thread will wait for |kPumpHaveWork| to be cleared.

> Are you sure using a tri-state variable here is best?  Maybe it would be
cleaner
> to have two variables, one for indicating work and the other for indicating
> shutdown?

Having two variables will increase the number of states to four and make
transitions between the states more complex and, therefore, - error prone.

https://chromiumcodereview.appspot.com/16020005/diff/45001/base/message_pump_...
base/message_pump_win.cc:152: // Try to wake up the message loop thread by
posting an APC. This might not
On 2013/06/10 19:28:45, darin wrote:
> I'm not sure I fully understand the conditions that lead to reaching this
code.

I don't have any data on hand but two reasons that come to my mind are:

1. The message queue is full. The receiving thread is spinning, blocked on
something or became unresponsive because any other reason.
2. The receiving thread has been terminated.

There could be other reasons.

> I also worry a little bit since sometimes other code runs nested message loops
> on our thread not using our MessageLoop infrastructure.  For example, a
library
> function might implement its own nested message loop.  I'm not sure what it
> means for our APC to run in that context.

APC will not be invoked unless the thread is in alertable state. Which means
either _our_ message pump is pumping events or 3rd party code doing an alertable
wait. In either case this CL either improve or does not change behavior:

1. If it is our pump, it will be waken up via APC, while before we would hope
that someone will post a message to us.
2. If it is not our code, than there is no change in behavior.

https://chromiumcodereview.appspot.com/16020005/diff/45001/base/message_pump_...
base/message_pump_win.cc:344: MWMO_ALERTABLE | MWMO_INPUTAVAILABLE);
On 2013/06/10 19:28:45, darin wrote:
> I'm a little worried that making this call alertable might result in wakeups
> caused by other sources.  What does it mean if WaitForWork() returns early in
> those cases (i.e., when we are not shutting down)?

It means that it will be possible to post APCs to the thread. Code in our code
base does not do it because it simply does not work currently. 3rd party code
that is doing that is currently broken anyway.

> It is probably safe to do
> another pass through DoRunLoop().

[cpu_(ooo_6.6-7.5), 2013-06-10 21:27:18]

only got cc now, sorry to be late to this party.

First off the message-only wrapper class is 99% the same motions you will have
to do for any window, apart from like 4 values that you could provide in a
constructor including HWND_MESSAGE.

Best here is to survey our window creation classes an unify this use into an
existing one or if the existing ones are not up par then create one. On a
different CL that is. I understand that you already landed this class once, but
as the owner of base\win it would be nice if additions there are at least fyi to
me.

There are also other strange things, but before we go into this let me propose a
path to get us from there to there faster and with less frustration.

I suggest the following, create a CL with the first and foremost thing you need
to fix which is the class name to the existing window. This should be a simple,
straightforward patch, which addresses the bug, we can get that done in 24 hours
or less.

In a second patch you then address the lifetime / threading issues of the
MessagePumpforUi, this time please include rvargas@.

https://codereview.chromium.org/16020005/diff/45001/base/win/message_window.cc
File base/win/message_window.cc (right):

https://codereview.chromium.org/16020005/diff/45001/base/win/message_window.c...
base/win/message_window.cc:23: instance_ =
base::GetModuleFromAddress(static_cast<WNDPROC>(
I don't see any value on keeping instance_, you can retrieve that value as shown
here in the two places needed.

btw, for this purpose &MessageWindow::WindowProc would be clearer.

https://codereview.chromium.org/16020005/diff/45001/base/win/message_window.c...
base/win/message_window.cc:32: window_ = NULL;
we don't use that practice of setting member vars to 0 in the dtor.

https://codereview.chromium.org/16020005/diff/45001/base/win/message_window.c...
base/win/message_window.cc:61: atom_ = RegisterClassEx(&window_class);
what is the value of keeping the class name and the atom as members?

[alexeypa (please no reviews), 2013-06-10 21:57:15]

On 2013/06/10 21:27:18, cpu wrote:
> I suggest the following, create a CL with the first and foremost thing you
need
> to fix which is the class name to the existing window. This should be a
simple,
> straightforward patch, which addresses the bug, we can get that done in 24
hours
> or less.
> 
> In a second patch you then address the lifetime / threading issues of the
> MessagePumpforUi, this time please include rvargas@.

OK, great. I'll follow up with a couple of CL.