Make sure that the UI window created by base::MessagePumpForUI is destoyed on the same thread (Wind…

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
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207278

[alexeypa (please no reviews), 2013-06-10 23:51:34]

Ricardo,

I'm splitting https://codereview.chromium.org/16020005/ into pieces so that it
could get thought the review. This is the second part. 

PTAL.

cc-ing: cpu@ and darin@ since there were on the original CL.

Please, let me know if you cannot review it for any reason, so I could find
another reviewer. Thanks!

[rvargas (doing something else), 2013-06-12 00:22:22]

https://codereview.chromium.org/15709015/diff/1/base/message_pump.h
File base/message_pump.h (right):

https://codereview.chromium.org/15709015/diff/1/base/message_pump.h#newcode122
base/message_pump.h:122: // Called directly by MessageLoop::~MessageLoop to
clean up resources that
nit: it's better to document what the method does instead of who calls it... and
it would also be better if the method is named accordingly (note that this file
does not mention MesssageLoop so far).

https://codereview.chromium.org/15709015/diff/1/base/message_pump.h#newcode124
base/message_pump.h:124: virtual void WillDestroyCurrentMessageLoop() {}
This class is a pure interface... let's keep all methods pure virtual.

https://codereview.chromium.org/15709015/diff/1/base/message_pump_win.cc
File base/message_pump_win.cc (right):

https://codereview.chromium.org/15709015/diff/1/base/message_pump_win.cc#newc...
base/message_pump_win.cc:29: static const LONG kPumpIdle = 0;
nit: don't need static

https://codereview.chromium.org/15709015/diff/1/base/message_pump_win.cc#newc...
base/message_pump_win.cc:33: // Used to wake up the message loop thread in the
case if it is waiting for
nit: remove "in the case"

https://codereview.chromium.org/15709015/diff/1/base/message_pump_win.cc#newc...
base/message_pump_win.cc:121: FALSE,    // no not inherit this handle
fix comment

https://codereview.chromium.org/15709015/diff/1/base/message_pump_win.cc#newc...
base/message_pump_win.cc:492: // then resets the pump_state_ flag (allowing a
replacement kMsgHaveWork to
nit: not a flag to be reset anymore.

https://codereview.chromium.org/15709015/diff/3001/base/message_pump_win.cc
File base/message_pump_win.cc (right):

https://codereview.chromium.org/15709015/diff/3001/base/message_pump_win.cc#n...
base/message_pump_win.cc:217: while (InterlockedCompareExchange(&pump_state_,
kPumpDisabled,
To be honest I prefer the version with a lock here.

ScheduleWork can use Try instead of Acquire (either someone else is starting the
pump or the loop is going away) so a regular critical section should be very
close to an interlocked operation.

We should only roll our own locks (like this implementation) after finding that
a regular lock is hurting performance.

[alexeypa (please no reviews), 2013-06-12 18:05:22]

https://codereview.chromium.org/15709015/diff/1/base/message_pump.h
File base/message_pump.h (right):

https://codereview.chromium.org/15709015/diff/1/base/message_pump.h#newcode122
base/message_pump.h:122: // Called directly by MessageLoop::~MessageLoop to
clean up resources that
On 2013/06/12 00:22:22, rvargas wrote:
> nit: it's better to document what the method does instead of who calls it...
and
> it would also be better if the method is named accordingly (note that this
file
> does not mention MesssageLoop so far).

Done.

https://codereview.chromium.org/15709015/diff/1/base/message_pump.h#newcode124
base/message_pump.h:124: virtual void WillDestroyCurrentMessageLoop() {}
On 2013/06/12 00:22:22, rvargas wrote:
> This class is a pure interface... let's keep all methods pure virtual.

Done.

https://codereview.chromium.org/15709015/diff/1/base/message_pump_win.cc
File base/message_pump_win.cc (right):

https://codereview.chromium.org/15709015/diff/1/base/message_pump_win.cc#newc...
base/message_pump_win.cc:29: static const LONG kPumpIdle = 0;
On 2013/06/12 00:22:22, rvargas wrote:
> nit: don't need static

Done.

https://codereview.chromium.org/15709015/diff/1/base/message_pump_win.cc#newc...
base/message_pump_win.cc:33: // Used to wake up the message loop thread in the
case if it is waiting for
On 2013/06/12 00:22:22, rvargas wrote:
> nit: remove "in the case"

Done.

https://codereview.chromium.org/15709015/diff/1/base/message_pump_win.cc#newc...
base/message_pump_win.cc:121: FALSE,    // no not inherit this handle
On 2013/06/12 00:22:22, rvargas wrote:
> fix comment

Done.

https://codereview.chromium.org/15709015/diff/1/base/message_pump_win.cc#newc...
base/message_pump_win.cc:492: // then resets the pump_state_ flag (allowing a
replacement kMsgHaveWork to
On 2013/06/12 00:22:22, rvargas wrote:
> nit: not a flag to be reset anymore.

Done.

https://codereview.chromium.org/15709015/diff/3001/base/message_pump_win.cc
File base/message_pump_win.cc (right):

https://codereview.chromium.org/15709015/diff/3001/base/message_pump_win.cc#n...
base/message_pump_win.cc:217: while (InterlockedCompareExchange(&pump_state_,
kPumpDisabled,
On 2013/06/12 00:22:23, rvargas wrote:
> To be honest I prefer the version with a lock here.

Well, Darin's reaction was exactly the opposite. He was concerned about
performance penalty the lock adds. See https://codereview.chromium.org/16020005.

> ScheduleWork can use Try instead of Acquire (either someone else is starting
the
> pump or the loop is going away) so a regular critical section should be very
> close to an interlocked operation.

Try vs Acquire should not make much difference. The blocking can occur only when
MessageLoop is being destroyed. So when ScheduleWork() enters the critical
section it is always free meaning EnterCriticalSection() never enters the kernel
nor it spins.

> We should only roll our own locks (like this implementation) after finding
that
> a regular lock is hurting performance.

Check out my comments at https://codereview.chromium.org/16020005/#msg9.

Another reason to keep the interlocked logic that that it wasn't introduced in
this CL. It existed before, I just added the3rd state to it.

[rvargas (doing something else), 2013-06-12 18:24:24]

https://codereview.chromium.org/15709015/diff/3001/base/message_pump_win.cc
File base/message_pump_win.cc (right):

https://codereview.chromium.org/15709015/diff/3001/base/message_pump_win.cc#n...
base/message_pump_win.cc:217: while (InterlockedCompareExchange(&pump_state_,
kPumpDisabled,
On 2013/06/12 18:05:22, alexeypa wrote:
> On 2013/06/12 00:22:23, rvargas wrote:
> > To be honest I prefer the version with a lock here.
> 
> Well, Darin's reaction was exactly the opposite. He was concerned about
> performance penalty the lock adds. See
https://codereview.chromium.org/16020005.
> 
> > ScheduleWork can use Try instead of Acquire (either someone else is starting
> the
> > pump or the loop is going away) so a regular critical section should be very
> > close to an interlocked operation.
> 
> Try vs Acquire should not make much difference. The blocking can occur only
when
> MessageLoop is being destroyed. So when ScheduleWork() enters the critical
> section it is always free meaning EnterCriticalSection() never enters the
kernel
> nor it spins.
> 
> > We should only roll our own locks (like this implementation) after finding
> that
> > a regular lock is hurting performance.
> 
> Check out my comments at https://codereview.chromium.org/16020005/#msg9.
> 
> Another reason to keep the interlocked logic that that it wasn't introduced in
> this CL. It existed before, I just added the3rd state to it.

Yes, I read all the comments on the original CL before sending my comments
out...

I agree that Try vs Acquire should be close to the same (you _can_ have multiple
threads calling ScheduleWork at the same time so there is a small difference).
But the fact that we don't care about possible contention during the normal
lifetime of the pump is lost on your original version with a lock.

... and that is part of my argument against writing our own lock. The current
version has a very simple "lock" (a boolean value accessed with an interlocked
operation). This cl is extending that lock to a multi-state interlocked variable
aided with posting a task and and APC. All that logic is a complex
synchronization "primitive" and I don't see the justification for it.

Yes, the new behavior requires more synchronization to avoid the shutdown race
of using a closed handle. But a standard lock provides basically the same
performance of the current implementation (one interlocked access), and it is
way simpler.

[alexeypa (please no reviews), 2013-06-12 18:57:25]

https://codereview.chromium.org/15709015/diff/3001/base/message_pump_win.cc
File base/message_pump_win.cc (right):

https://codereview.chromium.org/15709015/diff/3001/base/message_pump_win.cc#n...
base/message_pump_win.cc:217: while (InterlockedCompareExchange(&pump_state_,
kPumpDisabled,
On 2013/06/12 18:24:24, rvargas wrote:
> ... synchronization "primitive" and I don't see the justification for it.

OK, fair enough. I resurrected the version with the lock. PTAL.

[rvargas (doing something else), 2013-06-12 23:03:22]

LGTM after the nits.

https://codereview.chromium.org/15709015/diff/22001/base/message_pump.h
File base/message_pump.h (right):

https://codereview.chromium.org/15709015/diff/22001/base/message_pump.h#newco...
base/message_pump.h:123: // that called Run(). This is the last method called on
that thread.
I know what you want to say, but I don't know how to read the last sentence. It
should say "this must be the last", but that is not correct. "should be" is
closer, but also not true (as in nothing really happens if, say, ScheduleWork()
is called on that thread after Stop... it just doesn't do anything.

How about "Stops the pump and frees any resources that must to be freed on the
thread that called Run(). This method should be called on the same thread that
called Run()"... I still don't like how that reads, but it is explicit about
thread expectations, but lacks the detail about the pump doing nothing after
this point; maybe that's clear enough?

https://codereview.chromium.org/15709015/diff/22001/base/message_pump_win.cc
File base/message_pump_win.cc (right):

https://codereview.chromium.org/15709015/diff/22001/base/message_pump_win.cc#...
base/message_pump_win.cc:119: base::AutoLock lock(message_hwnd_lock_);
doing:
if (!message_hwnd_lock_.Try())
  return;

Makes it very explicit that this lock should never be contended.

https://codereview.chromium.org/15709015/diff/22001/base/message_pump_win.cc#...
base/message_pump_win.cc:121: // Do nothing if the window has been destoryed
already. There is no thread
nit: remove the last sentence... there may be a thread, there is just no window.

[alexeypa (please no reviews), 2013-06-13 00:13:39]

https://chromiumcodereview.appspot.com/15709015/diff/22001/base/message_pump.h
File base/message_pump.h (right):

https://chromiumcodereview.appspot.com/15709015/diff/22001/base/message_pump....
base/message_pump.h:123: // that called Run(). This is the last method called on
that thread.
On 2013/06/12 23:03:22, rvargas wrote:
> I know what you want to say, but I don't know how to read the last sentence.
It
> should say "this must be the last", but that is not correct. "should be" is
> closer, but also not true (as in nothing really happens if, say,
ScheduleWork()
> is called on that thread after Stop... it just doesn't do anything.
> 
> How about "Stops the pump and frees any resources that must to be freed on the
> thread that called Run(). This method should be called on the same thread that
> called Run()"... I still don't like how that reads, but it is explicit about
> thread expectations, but lacks the detail about the pump doing nothing after
> this point; maybe that's clear enough?

Done.

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

https://chromiumcodereview.appspot.com/15709015/diff/22001/base/message_pump_...
base/message_pump_win.cc:119: base::AutoLock lock(message_hwnd_lock_);
On 2013/06/12 23:03:22, rvargas wrote:
> doing:
> if (!message_hwnd_lock_.Try())
>   return;
> 
> Makes it very explicit that this lock should never be contended.

Done.

https://chromiumcodereview.appspot.com/15709015/diff/22001/base/message_pump_...
base/message_pump_win.cc:121: // Do nothing if the window has been destoryed
already. There is no thread
On 2013/06/12 23:03:22, rvargas wrote:
> nit: remove the last sentence... there may be a thread, there is just no
window.

Done.

[commit-bot: I haz the power, 2013-06-13 00:13:58]

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

[commit-bot: I haz the power, 2013-06-13 02:28:25]

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

[commit-bot: I haz the power, 2013-06-13 07:08:15]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-06-13 13:32:21]

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

[commit-bot: I haz the power, 2013-06-13 13:40:48]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-06-14 04:11:31]

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

[commit-bot: I haz the power, 2013-06-14 04:11:36]

Failed to apply patch for base/message_loop.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  can't find file to patch at input line 6
  Perhaps you used the wrong -p or --strip option?
  The text leading up to this was:
  --------------------------
  |Index: base/message_loop.cc
  |diff --git a/base/message_loop.cc b/base/message_loop.cc
  |index
0d8a8ef0d769875f386e244b69932779e9ed2271..c8b2ecb49261d6159a271a8fdcb8aa43d54f9467
100644
  |--- a/base/message_loop.cc
  |+++ b/base/message_loop.cc
  --------------------------
  No file to patch.  Skipping patch.
  1 out of 1 hunk ignored

Patch:       base/message_loop.cc
Index: base/message_loop.cc
diff --git a/base/message_loop.cc b/base/message_loop.cc
index
0d8a8ef0d769875f386e244b69932779e9ed2271..c8b2ecb49261d6159a271a8fdcb8aa43d54f9467
100644
--- a/base/message_loop.cc
+++ b/base/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_->Stop();
+
   // OK, now make it so that no one can find us.
   lazy_tls_ptr.Pointer()->Set(NULL);

[alexeypa (please no reviews), 2013-06-14 15:02:06]

Adding darin@ for OWNERS approval.

[alexeypa (please no reviews), 2013-06-17 18:10:35]

Darin, ping.

[alexeypa (please no reviews), 2013-06-18 18:19:32]

Darin, ping^2.

[darin (slow to review), 2013-06-18 18:28:41]

LGTM w/ the following changes:

https://chromiumcodereview.appspot.com/15709015/diff/31001/base/message_pump.h
File base/message_pump.h (right):

https://chromiumcodereview.appspot.com/15709015/diff/31001/base/message_pump....
base/message_pump.h:125: virtual void Stop() = 0;
It is perhaps confusing to have a method named Quit and a method named Stop on
the same interface.  They sound like synonyms for the same thing.

"Shutdown" might be a better name since it better conveys a notion of finality.

Also, the comment says that this should be called on the same thread that called
Run.  However, from reading MessagePumpWin, it looks like InitMessageWnd is
called from the constructor, so maybe you should say that Shutdown must be
called on the same thread that allocated the pump.

I think the important thing to document here is that this is potentially the
last chance for the MessagePump to do work on the thread where it was
constructed.

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

https://chromiumcodereview.appspot.com/15709015/diff/31001/base/message_pump_...
base/message_pump_win.cc:118: if (!message_hwnd_lock_.Try())
Please add a comment explaining the design of this code.  The use of .Try() is a
bit subtle.  You should explain what it means for .Try() to fail (i.e., that it
could only fail if we were in the process of shutting down).

[alexeypa (please no reviews), 2013-06-18 19:44:23]

FYI

https://chromiumcodereview.appspot.com/15709015/diff/31001/base/message_pump.h
File base/message_pump.h (right):

https://chromiumcodereview.appspot.com/15709015/diff/31001/base/message_pump....
base/message_pump.h:125: virtual void Stop() = 0;
On 2013/06/18 18:28:42, darin wrote:
> "Shutdown" might be a better name since it better conveys a notion of
finality.

Done.

> Also, the comment says that this should be called on the same thread that
called
> Run. 

It is already in the comment.

> However, from reading MessagePumpWin, it looks like InitMessageWnd is
> called from the constructor, so maybe you should say that Shutdown must be
> called on the same thread that allocated the pump.

What is important is that Shutdown() is called on the same thread as Run() was
called. The fact that the pump is also created on this thread is not part of
MessagePump interface and can be changed in the future.

> I think the important thing to document here is that this is potentially the
> last chance for the MessagePump to do work on the thread where it was
> constructed.

Done.

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

https://chromiumcodereview.appspot.com/15709015/diff/31001/base/message_pump_...
base/message_pump_win.cc:118: if (!message_hwnd_lock_.Try())
On 2013/06/18 18:28:42, darin wrote:
> Please add a comment explaining the design of this code.  The use of .Try() is
a
> bit subtle.  You should explain what it means for .Try() to fail (i.e., that
it
> could only fail if we were in the process of shutting down).

Done.

[commit-bot: I haz the power, 2013-06-18 19:47:42]

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

[darin (slow to review), 2013-06-18 20:31:15]

https://chromiumcodereview.appspot.com/15709015/diff/68001/base/message_pump.h
File base/message_pump.h (right):

https://chromiumcodereview.appspot.com/15709015/diff/68001/base/message_pump....
base/message_pump.h:128: // does) run on arbitraty thread.
nit: s/arbitraty/arbitrary/

This comment section seems to re-state the same thing a few times.  I bet you
could tighten it up a bit.

[alexeypa (please no reviews), 2013-06-18 20:48:52]

https://chromiumcodereview.appspot.com/15709015/diff/68001/base/message_pump.h
File base/message_pump.h (right):

https://chromiumcodereview.appspot.com/15709015/diff/68001/base/message_pump....
base/message_pump.h:128: // does) run on arbitraty thread.
On 2013/06/18 20:31:16, darin wrote:
> nit: s/arbitraty/arbitrary/
> 
> This comment section seems to re-state the same thing a few times.  I bet you
> could tighten it up a bit.

Done.

[darin (slow to review), 2013-06-18 20:51:42]

LGTM

[commit-bot: I haz the power, 2013-06-18 21:01:31]

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

[commit-bot: I haz the power, 2013-06-19 06:57:20]

Retried try job too often on win_rel for step(s) chrome_frame_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-06-19 13:53:36]

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

[commit-bot: I haz the power, 2013-06-20 02:38:14]

Change committed as 207278