Made MessagePump a non-thread safe class.

Made MessagePump a non-thread safe class.

This CL makes MessagePump a non-thread safe class to make sure thread-bound resources (such as the UI window used for pumping messages on Windows) are freed on the correct thread.

Handling of incoming tasks and synchronization between different threads was moved out to a separate class - IncomingTaskQueue reducing the number of locks to be taken while posting a task to one. Posting tasks via both MessageLoop and MessageLoopProxyImpl is now routed via IncomingTaskQueue.

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

[alexeypa (please no reviews), 2013-06-25 23:55:52]

Patch #2 now passes most of the test (bots are not available at the moment, so I
cannot be sure that all redness is not mine).

Please take a look and tell me what you think.

There are two potential issues with this CL:
- FileDescriptorWatcher in message_pump_io_ios.h now keeps a weak pointer to the
pump. FileDescriptorWatcher in message_pump_libevent.h on the other hand uses a
raw pointer (assuming that all watchers will be deleted before the pump is
deleted?). I'm not sure whether both can use a raw pointer or both should use a
weak pointer.

- MessageLoop::AddToIncomingQueue now calls ScheduleWork() under the lock. I
think it should be safe because ScheduleWork() does not execute the tasks, -
only wakes up the pump. Also ~MessageLoop() is synchronized with
MessageLoop::AddToIncomingQueue(), so it should be OK to do that.

[alexeypa (please no reviews), 2013-06-27 17:46:22]

Ricardo, ping.

[rvargas (doing something else), 2013-06-28 03:00:51]

Sorry for the delay... it's a delicate CL

I guess it doesn't look too bad.

Interesting detail about the file watchers. There is a comment (and unit test)
on m_p_libevent.cc that hints that both should probably use WeakPtr, but someone
has to look carefully at both both files.

https://codereview.chromium.org/17567007/diff/32001/base/message_loop/message...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/17567007/diff/32001/base/message_loop/message...
base/message_loop/message_loop.cc:350: scoped_refptr<MessageLoopProxy>
MessageLoop::message_loop_proxy() {
why de-inline this?

https://codereview.chromium.org/17567007/diff/32001/base/message_loop/message...
base/message_loop/message_loop.cc:382: void MessageLoop::AssertIdle() const {
This should probably be a ForTest method.

https://codereview.chromium.org/17567007/diff/32001/base/message_loop/message...
base/message_loop/message_loop.cc:384: AutoLock
lock(message_loop_proxy_->message_loop_lock_);
we should do something to avoid grabbing this lock directly.

https://codereview.chromium.org/17567007/diff/32001/base/message_loop/message...
base/message_loop/message_loop.cc:517: AutoLock
lock(message_loop_proxy_->message_loop_lock_);
Maybe

message_loop_proxy_->LoadPendingTasks();

MessageLoopProxy::LoadPendingTasks() {
  grab lock
  loop_->LoadPendingTasks();
}

MessageLoop::LoadPendingTasks() {
  DCHECK(work_queue_.empty());
  ...
}
(again, documenting that access to LoadPendingTasks should be serialized by the
caller)

Or at the very least using a public accessor to the lock, but using a lock of
another class doesn't look clean.

https://codereview.chromium.org/17567007/diff/32001/base/message_loop/message...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/17567007/diff/32001/base/message_loop/message...
base/message_loop/message_loop.h:509: //
|message_loop_proxy_->message_loop_lock_|.
I'm not really comfortable with this pattern of protecting a member with a lock
that belongs to another class.

I think it is better to keep interactions at the method call level: stating that
the caller of AddToIncomingQueue() is responsible for the synchronization is
fine.

[alexeypa (please no reviews), 2013-06-28 17:00:57]

OK, I moved |incoming_queue_| to MessageLoopProxyImpl. PTAL.

https://codereview.chromium.org/17567007/diff/32001/base/message_loop/message...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/17567007/diff/32001/base/message_loop/message...
base/message_loop/message_loop.cc:350: scoped_refptr<MessageLoopProxy>
MessageLoop::message_loop_proxy() {
On 2013/06/28 03:00:51, rvargas wrote:
> why de-inline this?

"base/message_loop/message_loop_proxy_impl.h" cannot be included to
message_loop.h because it will create a circular dependency. Without it the
compiler does not know that MessageLoopProxyImpl can be casted to
MessageLoopProxy.

https://codereview.chromium.org/17567007/diff/32001/base/message_loop/message...
base/message_loop/message_loop.cc:382: void MessageLoop::AssertIdle() const {
On 2013/06/28 03:00:51, rvargas wrote:
> This should probably be a ForTest method.

Done.

https://codereview.chromium.org/17567007/diff/32001/base/message_loop/message...
base/message_loop/message_loop.cc:384: AutoLock
lock(message_loop_proxy_->message_loop_lock_);
On 2013/06/28 03:00:51, rvargas wrote:
> we should do something to avoid grabbing this lock directly.

Done.

https://codereview.chromium.org/17567007/diff/32001/base/message_loop/message...
base/message_loop/message_loop.cc:517: AutoLock
lock(message_loop_proxy_->message_loop_lock_);
On 2013/06/28 03:00:51, rvargas wrote:
> Maybe
> 
> message_loop_proxy_->LoadPendingTasks();
> 
> MessageLoopProxy::LoadPendingTasks() {
>   grab lock
>   loop_->LoadPendingTasks();
> }
> 
> MessageLoop::LoadPendingTasks() {
>   DCHECK(work_queue_.empty());
>   ...
> }
> (again, documenting that access to LoadPendingTasks should be serialized by
the
> caller)
> 
> Or at the very least using a public accessor to the lock, but using a lock of
> another class doesn't look clean.

Done.

https://codereview.chromium.org/17567007/diff/32001/base/message_loop/message...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/17567007/diff/32001/base/message_loop/message...
base/message_loop/message_loop.h:509: //
|message_loop_proxy_->message_loop_lock_|.
On 2013/06/28 03:00:51, rvargas wrote:
> I'm not really comfortable with this pattern of protecting a member with a
lock
> that belongs to another class.
> 
> I think it is better to keep interactions at the method call level: stating
that
> the caller of AddToIncomingQueue() is responsible for the synchronization is
> fine.

Done.

[rvargas (doing something else), 2013-06-28 22:59:44]

https://codereview.chromium.org/17567007/diff/46001/base/message_loop/message...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/17567007/diff/46001/base/message_loop/message...
base/message_loop/message_loop.h:440: // resonsible for synchronizing
StartPump() calls.
typo: responsible

https://codereview.chromium.org/17567007/diff/46001/base/message_loop/message...
base/message_loop/message_loop.h:441: void StartPump();
nit: Maybe WakeUpPump()? RestartPump()? StartPump is kind of close to Start()
and it doesn't convey the idea that it should be called again  and again.

https://codereview.chromium.org/17567007/diff/46001/base/message_loop/message...
base/message_loop/message_loop.h:494: bool os_modal_loop_;
Now that you're moving variables do you mind keeping all bools together?

https://codereview.chromium.org/17567007/diff/46001/base/message_loop/message...
File base/message_loop/message_loop_proxy_impl.cc (right):

https://codereview.chromium.org/17567007/diff/46001/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.cc:67: if (message_loop_ &&
(MessageLoop::current() != message_loop_)) {
This should be always false. (we should only reach refcount 0 after
WillDestroyCurrentML). DCHECK?

https://codereview.chromium.org/17567007/diff/46001/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.cc:109:
high_resolution_timer_expiration_ = TimeTicks::Now() +
This looks like racing to me (same thing on the old code). Not that it matters,
but tools may start complaining about it. Actually, ActivateHighResolutionTimer
seem to have bigger issues when racing. It may be a good idea to push the whole
thing behind the lock.

https://codereview.chromium.org/17567007/diff/46001/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.cc:183: return !!message_loop_;
early return before adding the task to the queue.

https://codereview.chromium.org/17567007/diff/46001/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.cc:191: if (!work_queue->empty())
nit: we should probably do this check at the caller side.

https://codereview.chromium.org/17567007/diff/52001/base/message_loop/message...
File base/message_loop/message_loop_proxy_impl.h (right):

https://codereview.chromium.org/17567007/diff/52001/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.h:76: bool TryAddToIncomingQueue(const
tracked_objects::Location& from_here,
it would be nice to have some sort of indication of what methods are supposed to
be used by the message_loop. The fact that it is a friend and all methods are
private makes that difficult.

[alexeypa (please no reviews), 2013-07-03 18:56:48]

https://chromiumcodereview.appspot.com/17567007/diff/46001/base/message_loop/...
File base/message_loop/message_loop.h (right):

https://chromiumcodereview.appspot.com/17567007/diff/46001/base/message_loop/...
base/message_loop/message_loop.h:440: // resonsible for synchronizing
StartPump() calls.
On 2013/06/28 22:59:44, rvargas wrote:
> typo: responsible

Done.

https://chromiumcodereview.appspot.com/17567007/diff/46001/base/message_loop/...
base/message_loop/message_loop.h:441: void StartPump();
On 2013/06/28 22:59:44, rvargas wrote:
> nit: Maybe WakeUpPump()? RestartPump()? StartPump is kind of close to Start()
> and it doesn't convey the idea that it should be called again  and again.

Done.

https://chromiumcodereview.appspot.com/17567007/diff/46001/base/message_loop/...
base/message_loop/message_loop.h:494: bool os_modal_loop_;
On 2013/06/28 22:59:44, rvargas wrote:
> Now that you're moving variables do you mind keeping all bools together?

Done.

https://chromiumcodereview.appspot.com/17567007/diff/46001/base/message_loop/...
File base/message_loop/message_loop_proxy_impl.cc (right):

https://chromiumcodereview.appspot.com/17567007/diff/46001/base/message_loop/...
base/message_loop/message_loop_proxy_impl.cc:67: if (message_loop_ &&
(MessageLoop::current() != message_loop_)) {
On 2013/06/28 22:59:44, rvargas wrote:
> This should be always false. (we should only reach refcount 0 after
> WillDestroyCurrentML). DCHECK?

Done. I deleted OnDestruct and added DCHECK to the destructor.

https://chromiumcodereview.appspot.com/17567007/diff/46001/base/message_loop/...
base/message_loop/message_loop_proxy_impl.cc:109:
high_resolution_timer_expiration_ = TimeTicks::Now() +
On 2013/06/28 22:59:44, rvargas wrote:
> This looks like racing to me (same thing on the old code). Not that it
matters,
> but tools may start complaining about it. Actually,
ActivateHighResolutionTimer
> seem to have bigger issues when racing. It may be a good idea to push the
whole
> thing behind the lock.

Done.

https://chromiumcodereview.appspot.com/17567007/diff/46001/base/message_loop/...
base/message_loop/message_loop_proxy_impl.cc:183: return !!message_loop_;
On 2013/06/28 22:59:44, rvargas wrote:
> early return before adding the task to the queue.

Done.

https://chromiumcodereview.appspot.com/17567007/diff/46001/base/message_loop/...
base/message_loop/message_loop_proxy_impl.cc:191: if (!work_queue->empty())
On 2013/06/28 22:59:44, rvargas wrote:
> nit: we should probably do this check at the caller side.

Done.

[alexeypa (please no reviews), 2013-07-09 19:14:05]

ping.

[rvargas (doing something else), 2013-07-09 21:46:09]

We are missing the message_pump_win files. Looks basically good.

https://codereview.chromium.org/17567007/diff/64036/base/message_loop/message...
File base/message_loop/message_loop_proxy_impl.cc (right):

https://codereview.chromium.org/17567007/diff/64036/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.cc:175: // Make sure now tasks are
lost.
nit: I don't understand the comment.

https://codereview.chromium.org/17567007/diff/64036/base/message_loop/message...
File base/message_loop/message_loop_proxy_impl.h (right):

https://codereview.chromium.org/17567007/diff/64036/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.h:53: // called method.
nit: Given the number of private methods we should clearly state which methods
are supposed to be used by the message loop and which ones are truly private.

https://codereview.chromium.org/17567007/diff/64036/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.h:76: // The caller is responsible for
synchronizing calls to this method.
nit: This is a private method not used by anyone else so documenting here what
the caller is supposed to do looks out of place. (the part about
synchronization)

https://codereview.chromium.org/17567007/diff/64036/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.h:86: // arranges synchronization for
|*work_queue|.
nit: the last sentence is confusing. I understand that the point of the comment
is to state that this method doesn't do anything to synchronize work_queue, but
it may be easier to state that this method should be called from the thread that
is running the loop.

https://codereview.chromium.org/17567007/diff/64036/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.h:95: mutable base::Lock
incoming_queue_lock_;
nit: it would be nice to have a better name here (given that the lock protects
more than the queue) but I'm short of suggestions.

[alexeypa (please no reviews), 2013-07-09 23:37:40]

> We are missing the message_pump_win files.

No changes to message_pump_win are required as a part of this CL.

Once you are OK with it, I'll start adding OWNERS for all modified files.

https://codereview.chromium.org/17567007/diff/64036/base/message_loop/message...
File base/message_loop/message_loop_proxy_impl.cc (right):

https://codereview.chromium.org/17567007/diff/64036/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.cc:175: // Make sure now tasks are
lost.
On 2013/07/09 21:46:09, rvargas wrote:
> nit: I don't understand the comment.

Done.

https://codereview.chromium.org/17567007/diff/64036/base/message_loop/message...
File base/message_loop/message_loop_proxy_impl.h (right):

https://codereview.chromium.org/17567007/diff/64036/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.h:53: // called method.
On 2013/07/09 21:46:09, rvargas wrote:
> nit: Given the number of private methods we should clearly state which methods
> are supposed to be used by the message loop and which ones are truly private.

I converted MessageLoopProxyImpl into a private nested class declared in
MessageLoop. This way it remains an implementation detail of MessageLoop, but
MessageLoop can now call it public methods. |friend class MessageLoop| is not
needed any more.

https://codereview.chromium.org/17567007/diff/64036/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.h:76: // The caller is responsible for
synchronizing calls to this method.
On 2013/07/09 21:46:09, rvargas wrote:
> nit: This is a private method not used by anyone else so documenting here what
> the caller is supposed to do looks out of place. (the part about
> synchronization)

Done.

https://codereview.chromium.org/17567007/diff/64036/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.h:86: // arranges synchronization for
|*work_queue|.
On 2013/07/09 21:46:09, rvargas wrote:
> nit: the last sentence is confusing. I understand that the point of the
comment
> is to state that this method doesn't do anything to synchronize work_queue,
but
> it may be easier to state that this method should be called from the thread
that
> is running the loop.

Done.

https://codereview.chromium.org/17567007/diff/64036/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.h:95: mutable base::Lock
incoming_queue_lock_;
On 2013/07/09 21:46:09, rvargas wrote:
> nit: it would be nice to have a better name here (given that the lock protects
> more than the queue) but I'm short of suggestions.

I prefer to keep it |incoming_queue_lock_| unless someone comes up with a better
name. It is mainly all about managing the input queue even if it protects
something else too.

[rvargas (doing something else), 2013-07-10 00:24:42]

he, I see the public destructor on MessagePumpWin :(

LGTM

https://codereview.chromium.org/17567007/diff/91001/base/message_loop/message...
File base/message_loop/message_loop_proxy_impl.h (right):

https://codereview.chromium.org/17567007/diff/91001/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.h:64: // Diconnects |this| from the
parent message loop.
type: disconnects

https://codereview.chromium.org/17567007/diff/91001/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.h:68: virtual ~ProxyImpl();
nit: send it to the private section?

[alexeypa (please no reviews), 2013-07-10 16:14:26]

https://codereview.chromium.org/17567007/diff/91001/base/message_loop/message...
File base/message_loop/message_loop_proxy_impl.h (right):

https://codereview.chromium.org/17567007/diff/91001/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.h:64: // Diconnects |this| from the
parent message loop.
On 2013/07/10 00:24:42, rvargas wrote:
> type: disconnects

Done.

https://codereview.chromium.org/17567007/diff/91001/base/message_loop/message...
base/message_loop/message_loop_proxy_impl.h:68: virtual ~ProxyImpl();
On 2013/07/10 00:24:42, rvargas wrote:
> nit: send it to the private section?

Done.

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

Requesting OWNERS approval:

base/* - darin@
chrome/browser/ui/cocoa/* - thakis@
media/base/* - dalecurtis@

[Nico, 2013-07-10 16:52:49]

c/b/u/cocoa lgtm

But since this is a potentially subtle change, getting base/OWNERS approval
before asking for the rest might've made sense.

nit: wrap cl description at 80 cols

https://codereview.chromium.org/17567007/diff/102001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm (right):

https://codereview.chromium.org/17567007/diff/102001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm:11: #include
"base/memory/ref_counted.h"
still needed?

[DaleCurtis, 2013-07-10 17:31:08]

media/ lgtm, but some nits on base/

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
File base/message_loop/message_loop.h (left):

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
base/message_loop/message_loop.h:285: scoped_refptr<MessageLoopProxy>
message_loop_proxy() {
OOC, why? This is a simple getter. Per style guide this should be renamed if
you're going to stop inlining it.

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
base/message_loop/message_loop.h:380: bool
IsHishResolutionTimersEnabledForTest();
s/Hish/High/ s/Timers/Timer/ ?

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
File base/message_loop/message_loop_proxy_impl.h (right):

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
base/message_loop/message_loop_proxy_impl.h:49: bool
IsHishResolutionTimersEnabledForTest();
DItto.

[alexeypa (please no reviews), 2013-07-10 19:39:58]

Thanks!

Darin, now is your turn. :-)

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
File base/message_loop/message_loop.h (left):

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
base/message_loop/message_loop.h:285: scoped_refptr<MessageLoopProxy>
message_loop_proxy() {
On 2013/07/10 17:31:08, DaleCurtis wrote:
> OOC, why? This is a simple getter. Per style guide this should be renamed if
> you're going to stop inlining it.

Including message_loop_proxy_impl.h to message_loop.h creates a circular
dependency. Casting of |message_loop_proxy_| to MessageLoopProxy does not work
without MessageLoop::ProxyImpl's definition available.

MessageLoop::message_loop_proxy() is used in way too many files to change it.

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
File base/message_loop/message_loop.h (right):

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
base/message_loop/message_loop.h:380: bool
IsHishResolutionTimersEnabledForTest();
On 2013/07/10 17:31:08, DaleCurtis wrote:
> s/Hish/High/ s/Timers/Timer/ ?

Done.

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
File base/message_loop/message_loop_proxy_impl.h (right):

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
base/message_loop/message_loop_proxy_impl.h:49: bool
IsHishResolutionTimersEnabledForTest();
On 2013/07/10 17:31:08, DaleCurtis wrote:
> DItto.

Done.

https://codereview.chromium.org/17567007/diff/102001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm (right):

https://codereview.chromium.org/17567007/diff/102001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm:11: #include
"base/memory/ref_counted.h"
On 2013/07/10 16:52:49, Nico wrote:
> still needed?

Done.

[DaleCurtis, 2013-07-10 19:52:43]

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
File base/message_loop/message_loop.h (left):

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
base/message_loop/message_loop.h:285: scoped_refptr<MessageLoopProxy>
message_loop_proxy() {
On 2013/07/10 19:39:59, alexeypa wrote:
> On 2013/07/10 17:31:08, DaleCurtis wrote:
> > OOC, why? This is a simple getter. Per style guide this should be renamed if
> > you're going to stop inlining it.
> 
> Including message_loop_proxy_impl.h to message_loop.h creates a circular
> dependency. Casting of |message_loop_proxy_| to MessageLoopProxy does not work
> without MessageLoop::ProxyImpl's definition available.

This is because you've defined ProxyImpl as a nested class in MessageLoop. Is it
necessary to do that?

> 
> MessageLoop::message_loop_proxy() is used in way too many files to change it.

[alexeypa (please no reviews), 2013-07-10 20:05:26]

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
File base/message_loop/message_loop.h (left):

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
base/message_loop/message_loop.h:285: scoped_refptr<MessageLoopProxy>
message_loop_proxy() {
On 2013/07/10 19:52:43, DaleCurtis wrote:
> This is because you've defined ProxyImpl as a nested class in MessageLoop. Is
it
> necessary to do that?

No, it is because ProxyImpl (and MessageLoopProxyImpl before it was renamed) is
defined in a separate header file. Technically, I can move its declaration to
message_loop.h but will it make the code better?

[alexeypa (please no reviews), 2013-07-10 21:01:12]

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
File base/message_loop/message_loop.h (left):

https://codereview.chromium.org/17567007/diff/102001/base/message_loop/messag...
base/message_loop/message_loop.h:285: scoped_refptr<MessageLoopProxy>
message_loop_proxy() {
On 2013/07/10 20:05:27, alexeypa wrote:
> No, it is because ProxyImpl (and MessageLoopProxyImpl before it was renamed)
is
> defined in a separate header file. Technically, I can move its declaration to
> message_loop.h but will it make the code better?

We discussed this issue offline and it seems that moving MessageLoopProxyImpl to
base::internal is the optimal solution. PTAL patch #11.

[DaleCurtis, 2013-07-10 21:04:48]

lgtm. thanks! :)

[Robert Sesek, 2013-07-10 22:13:32]

https://codereview.chromium.org/17567007/diff/127001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm (right):

https://codereview.chromium.org/17567007/diff/127001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm:69:
quit_closure.Run();
Why can't Quit be called directly here?

[alexeypa (please no reviews), 2013-07-10 22:15:26]

https://codereview.chromium.org/17567007/diff/127001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm (right):

https://codereview.chromium.org/17567007/diff/127001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm:69:
quit_closure.Run();
On 2013/07/10 22:13:32, rsesek wrote:
> Why can't Quit be called directly here?

Because |message_pump_| is a scoped_ptr now and cannot be captured when
|quit_loop| is constructed. |base::Closure| is copyable so this does the trick.

[Robert Sesek, 2013-07-10 22:19:14]

https://codereview.chromium.org/17567007/diff/127001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm (right):

https://codereview.chromium.org/17567007/diff/127001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm:69:
quit_closure.Run();
On 2013/07/10 22:15:26, alexeypa wrote:
> On 2013/07/10 22:13:32, rsesek wrote:
> > Why can't Quit be called directly here?
> 
> Because |message_pump_| is a scoped_ptr now and cannot be captured when
> |quit_loop| is constructed. |base::Closure| is copyable so this does the
trick.

Ah, right. I think there's a slight preference to extracting the pointer into a
non-scoped local:

base::MessagePumpNSRunLoop* weak_message_pump = messge_pump.get();

[alexeypa (please no reviews), 2013-07-10 22:27:23]

https://codereview.chromium.org/17567007/diff/127001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm (right):

https://codereview.chromium.org/17567007/diff/127001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/history_overlay_controller_unittest.mm:69:
quit_closure.Run();
On 2013/07/10 22:19:15, rsesek wrote:
> On 2013/07/10 22:15:26, alexeypa wrote:
> > On 2013/07/10 22:13:32, rsesek wrote:
> > > Why can't Quit be called directly here?
> > 
> > Because |message_pump_| is a scoped_ptr now and cannot be captured when
> > |quit_loop| is constructed. |base::Closure| is copyable so this does the
> trick.
> 
> Ah, right. I think there's a slight preference to extracting the pointer into
a
> non-scoped local:
> 
> base::MessagePumpNSRunLoop* weak_message_pump = messge_pump.get();

Done.

[Robert Sesek, 2013-07-10 23:08:40]

lgtm, thanks

[alexeypa (please no reviews), 2013-07-11 20:17:19]

Darin, ping^2.

[alexeypa (please no reviews), 2013-07-15 15:42:39]

On 2013/07/11 20:17:19, alexeypa wrote:
> Darin, ping^2.

ping^3

[darin (slow to review), 2013-07-15 16:13:29]

On 2013/07/15 15:42:39, alexeypa wrote:
> On 2013/07/11 20:17:19, alexeypa wrote:
> > Darin, ping^2.
> 
> ping^3

pong... back from vacation now.  this is on my queue of CLs to review.

[darin (slow to review), 2013-07-16 16:26:45]

On 2013/07/15 16:13:29, darin wrote:
> On 2013/07/15 15:42:39, alexeypa wrote:
> > On 2013/07/11 20:17:19, alexeypa wrote:
> > > Darin, ping^2.
> > 
> > ping^3
> 
> pong... back from vacation now.  this is on my queue of CLs to review.

Uhm, this CL completely changes the relationship between MessageLoop and
MessageLoopProxy.  MLP was intended as a wrapper around ML.  MLP was not
intended as part of the ML implementation.

Note: content::BrowserThread provides an alternative implementation of MLP.
See BrowserThreadMessageLoopProxy.

With this CL, using BrowserThread's MLP implementation now involves calling
into the base::MLPImpl as well, which is not at all desired.

I think it would help to understand why you felt the need to change MLP.
Are you just trying to take advantage of it being reference counted?

[alexeypa (please no reviews), 2013-07-16 16:55:42]

On 2013/07/16 16:26:45, darin wrote:
> Uhm, this CL completely changes the relationship between MessageLoop and
> MessageLoopProxy.  MLP was intended as a wrapper around ML.  MLP was not
> intended as part of the ML implementation.

Not exactly. The CL changes the relationship between MessageLoop and
MessageLoopProxyImpl. It does not change any aspect of MessageLoopProxy
interface.  

> Note: content::BrowserThread provides an alternative implementation of MLP.
> See BrowserThreadMessageLoopProxy.
> 
> With this CL, using BrowserThread's MLP implementation now involves calling
> into the base::MLPImpl as well, which is not at all desired.

Why is it a bad thing? The fact that MLPImpl is used is an implementation detail
of MessageLoop. The caller still calls MessageLoop directly. It sounds like a
case of leaking abstraction if BrowserThreadMessageLoopProxy needs to know that
MLPImpl is not involved during the call.   

> I think it would help to understand why you felt the need to change MLP.
> Are you just trying to take advantage of it being reference counted?

It is an attempt to address issues uncovered while working on
https://chromiumcodereview.appspot.com/17517002/. There are a few issues:

1. It does not look like MessagePump should be ref-counted. It is naturally
bound to the thread running the loop. It can also use resources (the
message-only window on Windows) that are bound to the thread. Synchronizing
destruction of those resources across multiple threads is messy.
2. MessageLoop should not be used from multiple threads. It is not ref-counted
meaning that it is harder to synchronize its shutdown across threads.
3. MessageLoopProxy (TaskRunner) on the other hand is designed to be called from
a different thread. It seems to be natural to put any synchronization between
threads to MLP implementation.
4. Two locks are takes when a task is posted from a different thread. It is
unnecessary.     

This CL addresses all 1, 3 and 4. #2 is partially addressed - there are still
many callers using MessageLoop from a different thread, but at least it is very
clearly stated now that ML is bound to the thread.

[darin (slow to review), 2013-07-16 17:20:18]

On 2013/07/16 16:55:42, alexeypa wrote:
> On 2013/07/16 16:26:45, darin wrote:
> > Uhm, this CL completely changes the relationship between MessageLoop and
> > MessageLoopProxy.  MLP was intended as a wrapper around ML.  MLP was not
> > intended as part of the ML implementation.
> 
> Not exactly. The CL changes the relationship between MessageLoop and
> MessageLoopProxyImpl. It does not change any aspect of MessageLoopProxy
> interface.

True

> 
> > Note: content::BrowserThread provides an alternative implementation of MLP.
> > See BrowserThreadMessageLoopProxy.
> > 
> > With this CL, using BrowserThread's MLP implementation now involves calling
> > into the base::MLPImpl as well, which is not at all desired.
> 
> Why is it a bad thing? The fact that MLPImpl is used is an implementation
detail
> of MessageLoop. The caller still calls MessageLoop directly. It sounds like a
> case of leaking abstraction if BrowserThreadMessageLoopProxy needs to know
that
> MLPImpl is not involved during the call.   

BrowserThread provides an alternate implementation of MLP because it can do
so more efficiently.  It understands the lifetime of the browser threads,
so it can avoid some locking.

With this CL, we should probably just get rid of the BrowserThread
implementation
of MLP since it really adds no value.


> > I think it would help to understand why you felt the need to change MLP.
> > Are you just trying to take advantage of it being reference counted?
> 
> It is an attempt to address issues uncovered while working on
> https://chromiumcodereview.appspot.com/17517002/. There are a few issues:
> 
> 1. It does not look like MessagePump should be ref-counted. It is naturally
> bound to the thread running the loop. It can also use resources (the
> message-only window on Windows) that are bound to the thread. Synchronizing
> destruction of those resources across multiple threads is messy.
> 2. MessageLoop should not be used from multiple threads. It is not ref-counted
> meaning that it is harder to synchronize its shutdown across threads.
> 3. MessageLoopProxy (TaskRunner) on the other hand is designed to be called
from
> a different thread. It seems to be natural to put any synchronization between
> threads to MLP implementation.
> 4. Two locks are takes when a task is posted from a different thread. It is
> unnecessary.     
> 
> This CL addresses all 1, 3 and 4. #2 is partially addressed - there are still
> many callers using MessageLoop from a different thread, but at least it is
very
> clearly stated now that ML is bound to the thread.

I see where you are going with this.

I think we have the goal to eliminate direct MessageLoop usage over time,
replacing
it with SingleThreadTaskRunner / ThreadTaskRunnerHandle (for posting tasks) and
RunLoop (for running and quitting a message loop).

Perhaps this CL is a good step in that direction.  I need to think about this
more.

[darin (slow to review), 2013-07-17 16:46:47]

OK, here's what bugs me about this CL:

MessageLoopProxy is a "proxy interface" to a MessageLoop.  It is a wrapper for
ML.  MLPImpl is the implementation of the proxy interface around ML.

These names imply certain code structure and relationships.  To invert the
relationship like this CL does is inconsistent with the notion of MLP being a
proxy around ML.

However, it seems like the implementation of MLP is certainly improved by this
change.  Perhaps this suggests that we should be inventing a new intermediate
class instead of re-purposing the MLPImpl.  Perhaps there is a way to do that
while maintaining the optimization of MLP that this CL presents.

Possible approaches:
1- Give the MLPImpl in this CL a better name.
2- Break up the MLPImpl in this CL into two classes: MLIncomingQueue (?) and
MLPImpl.

#2 is conceptually cleanest, but #1 might be worth it from a performance point
of view.

[alexeypa (please no reviews), 2013-07-18 00:07:15]

On 2013/07/17 16:46:47, darin wrote:
> #2 is conceptually cleanest, but #1 might be worth it from a performance point
> of view.

OK, I moved all queue management out of MLPImpl into IncomingTaskQueue. PTAL.

[darin (slow to review), 2013-07-18 04:37:28]

Yeah, this is looking good.  More detailed review shortly.

[alexeypa (please no reviews), 2013-07-18 04:47:25]

On 2013/07/18 04:37:28, darin wrote:
> Yeah, this is looking good.  More detailed review shortly.

Thanks. I'll be waiting for it. One thing I'm a bit concerned is that both
MLPImpl and IncomingTaskQueue have to be ref-counted classed. I don't see a good
way to make IncomingTaskQueue non ref-counted. One way is to make it explicitly
owned by both ML and MLPIMpl. It does not look cleaner than simply making it a
ref-counted class.

[darin (slow to review), 2013-07-18 04:53:22]

I wouldn't worry too much about that overhead at this point.  Down the
road, once we get callers off of MessageLoop and onto
{SingleThread}TaskRunner, we can look into streamlining some of this code
further.


On Wed, Jul 17, 2013 at 9:47 PM, <alexeypa@chromium.org> wrote:

> On 2013/07/18 04:37:28, darin wrote:
>
>> Yeah, this is looking good.  More detailed review shortly.
>>
>
> Thanks. I'll be waiting for it. One thing I'm a bit concerned is that both
> MLPImpl and IncomingTaskQueue have to be ref-counted classed. I don't see
> a good
> way to make IncomingTaskQueue non ref-counted. One way is to make it
> explicitly
> owned by both ML and MLPIMpl. It does not look cleaner than simply making
> it a
> ref-counted class.
>
>
https://chromiumcodereview.**appspot.com/17567007/<https://chromiumcodereview...
>

[darin (slow to review), 2013-07-21 04:09:41]

LGTM w/ nits fixed

https://chromiumcodereview.appspot.com/17567007/diff/165001/base/message_loop...
File base/message_loop/incoming_task_queue.cc (right):

https://chromiumcodereview.appspot.com/17567007/diff/165001/base/message_loop...
base/message_loop/incoming_task_queue.cc:16: :
message_loop_(MessageLoop::current()),
minor-nit: Perhaps IncomingTaskQueue() should be parameterized with a
MessageLoop* instead?  I realize this is only done once when a ML is constructed
(so overhead of TLS is not an issue), but it may be a bit fragile to assume that
the TLS is set at this stage of ML construction.

https://chromiumcodereview.appspot.com/17567007/diff/165001/base/message_loop...
base/message_loop/incoming_task_queue.cc:164: message_loop_->WakeUpPump();
nit: The corresponding method on MessagePump is called ScheduleWork.  Perhaps
this method should be named similarly?

https://chromiumcodereview.appspot.com/17567007/diff/165001/base/message_loop...
File base/message_loop/incoming_task_queue.h (right):

https://chromiumcodereview.appspot.com/17567007/diff/165001/base/message_loop...
base/message_loop/incoming_task_queue.h:5: #ifndef BASE_INCOMING_TASK_QUEUE_H_
nit: BASE_MESSAGE_LOOP_INCOMING_TASK_QUEUE_H_

https://chromiumcodereview.appspot.com/17567007/diff/165001/base/message_loop...
base/message_loop/incoming_task_queue.h:29: // Acquires |incoming_queue_lock_|
and appends a task to |incoming_queue_|.
nit: I don't think it is useful to spell out the implementation here.  This
first sentence can be deleted.

https://chromiumcodereview.appspot.com/17567007/diff/165001/base/message_loop...
base/message_loop/incoming_task_queue.h:44: bool
IsHighResolutionTimerEnabledForTest();
micro-nit: I thought our convention was to use a ForTesting suffix rather than a
ForTest suffix.  Maybe I'm mistaken.

https://chromiumcodereview.appspot.com/17567007/diff/165001/base/message_loop...
base/message_loop/incoming_task_queue.h:61: bool TryAddToIncomingQueue(const
tracked_objects::Location& from_here,
nit: It feels like this should be listed after AddToIncomingQueue since they are
so related.

[alexeypa (please no reviews), 2013-07-21 06:52:25]

https://chromiumcodereview.appspot.com/17567007/diff/165001/base/message_loop...
File base/message_loop/incoming_task_queue.cc (right):

https://chromiumcodereview.appspot.com/17567007/diff/165001/base/message_loop...
base/message_loop/incoming_task_queue.cc:16: :
message_loop_(MessageLoop::current()),
On 2013/07/21 04:09:42, darin wrote:
> minor-nit: Perhaps IncomingTaskQueue() should be parameterized with a
> MessageLoop* instead?  I realize this is only done once when a ML is
constructed
> (so overhead of TLS is not an issue), but it may be a bit fragile to assume
that
> the TLS is set at this stage of ML construction.

Done.

https://chromiumcodereview.appspot.com/17567007/diff/165001/base/message_loop...
base/message_loop/incoming_task_queue.cc:164: message_loop_->WakeUpPump();
On 2013/07/21 04:09:42, darin wrote:
> nit: The corresponding method on MessagePump is called ScheduleWork.  Perhaps
> this method should be named similarly?

Done.

https://chromiumcodereview.appspot.com/17567007/diff/165001/base/message_loop...
File base/message_loop/incoming_task_queue.h (right):

https://chromiumcodereview.appspot.com/17567007/diff/165001/base/message_loop...
base/message_loop/incoming_task_queue.h:5: #ifndef BASE_INCOMING_TASK_QUEUE_H_
On 2013/07/21 04:09:42, darin wrote:
> nit: BASE_MESSAGE_LOOP_INCOMING_TASK_QUEUE_H_

Done.

https://chromiumcodereview.appspot.com/17567007/diff/165001/base/message_loop...
base/message_loop/incoming_task_queue.h:29: // Acquires |incoming_queue_lock_|
and appends a task to |incoming_queue_|.
On 2013/07/21 04:09:42, darin wrote:
> nit: I don't think it is useful to spell out the implementation here.  This
> first sentence can be deleted.

Done.

https://chromiumcodereview.appspot.com/17567007/diff/165001/base/message_loop...
base/message_loop/incoming_task_queue.h:44: bool
IsHighResolutionTimerEnabledForTest();
On 2013/07/21 04:09:42, darin wrote:
> micro-nit: I thought our convention was to use a ForTesting suffix rather than
a
> ForTest suffix.  Maybe I'm mistaken.

Done.

https://chromiumcodereview.appspot.com/17567007/diff/165001/base/message_loop...
base/message_loop/incoming_task_queue.h:61: bool TryAddToIncomingQueue(const
tracked_objects::Location& from_here,
On 2013/07/21 04:09:42, darin wrote:
> nit: It feels like this should be listed after AddToIncomingQueue since they
are
> so related.

Done.

[commit-bot: I haz the power, 2013-07-22 15:54:13]

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

[commit-bot: I haz the power, 2013-07-22 19:52:16]

Change committed as 212948

[jar1, 2013-07-29 23:30:49]

Some of this makes me pretty nervous about potential performance impact.  The
big thing is that the incomming_queue_lock_ needs to be held as briefly as
possible, as it is a primary source of inter-thread contention.  This is
generally significant enough that the performance impact can be very measurable.

https://chromiumcodereview.appspot.com/17567007/diff/188001/base/message_loop...
File base/message_loop/incoming_task_queue.cc (right):

https://chromiumcodereview.appspot.com/17567007/diff/188001/base/message_loop...
base/message_loop/incoming_task_queue.cc:25: AutoLock
locked(incoming_queue_lock_);
Calling to acquire the lock here is IMO much too early, and should instigate
contention on this lock (which should have a negative impact on perf).

This lock is *only* intended to protect incomming_queue_, so we should only be
manipulating that queue while holding the lock... and to the extent possible, do
nothing more.  Calls that might do allocations, or operating system calls,
should probably be pulled outside if at all possible.

https://chromiumcodereview.appspot.com/17567007/diff/188001/base/message_loop...
base/message_loop/incoming_task_queue.cc:27: from_here, task,
CalculateDelayedRuntime(delay), nestable);
The rule with locks is to try to protect data, and not to protect code.  In this
case, I *think* the lock was held to try to make the call to
CalculateDelayedRuntime() safe.  I suspect the intent was to avoid the races by
the posting entity in the update of the member variables that involve the
HighResTimer.  This was historically a problem, but I don't think this approach
is the right way to deal with it.

JamesR has some suggestions for how to move around how/when we do the adjust the
high res timer... and I think we can get away without having to do the locking
here.

https://chromiumcodereview.appspot.com/17567007/diff/188001/base/message_loop...
base/message_loop/incoming_task_queue.cc:28: return
PostPendingTask(&pending_task);
Here again we're calling to do work while holding a lock... which is very
hazardous.

Often a good pattern (if you ever needed to do this) is to postfix the name with
the word "Locked" to  make sure all callers know that you are holding a lock. 
That said... I think it would be much better to not hold the lock... as I think
you have troubles in the function (see below).

https://chromiumcodereview.appspot.com/17567007/diff/188001/base/message_loop...
base/message_loop/incoming_task_queue.cc:143:
incoming_queue_lock_.AssertAcquired();
It is good that you had this.... but if this stays, the code would be much more
readable if the method had the postfix "Locked."

https://chromiumcodereview.appspot.com/17567007/diff/188001/base/message_loop...
base/message_loop/incoming_task_queue.cc:163:
message_loop_->ScheduleWork(was_empty);
This is where things really get dicey.  The lock was really only intended to
protect the queue.... but now it is being held while makeing a call to
ScheduleWork()... which does not have a postfix... and hence it is not clear to
the reviewer that a lock is held, and they work done should be very quickly
finished.

https://chromiumcodereview.appspot.com/17567007/diff/188001/base/message_loop...
File base/message_loop/message_loop.cc (left):

https://chromiumcodereview.appspot.com/17567007/diff/188001/base/message_loop...
base/message_loop/message_loop.cc:628: AutoLock locked(incoming_queue_lock_,
AutoLock::AlreadyAcquired());
It is very important when using locks to hold them for the minimal length of
time, and also to try to avoid calling any other functions that may "take time"
or worse "grab other locks."  

Specifically in this case, care was taken to release the lock on line 646 before
calling ScheduleWork().

In addition, this code was careful to do as much work outside the lock as
possible, such as the construction of the task, which was done in the caller.

https://chromiumcodereview.appspot.com/17567007/diff/188001/base/message_loop...
File base/message_loop/message_loop.cc (right):

https://chromiumcodereview.appspot.com/17567007/diff/188001/base/message_loop...
base/message_loop/message_loop.cc:550: void MessageLoop::ScheduleWork(bool
was_empty) {
This is (I think) where you forward the call while holding the lock on the
queue.

[alexeypa (please no reviews), 2013-07-30 00:31:13]

https://chromiumcodereview.appspot.com/17567007/diff/188001/base/message_loop...
File base/message_loop/incoming_task_queue.cc (right):

https://chromiumcodereview.appspot.com/17567007/diff/188001/base/message_loop...
base/message_loop/incoming_task_queue.cc:25: AutoLock
locked(incoming_queue_lock_);
On 2013/07/29 23:30:50, jar1 wrote:
> Calling to acquire the lock here is IMO much too early, and should instigate
> contention on this lock (which should have a negative impact on perf).

Before I made this change, there were two locks. One was taken while posting
messages via MessageLoop. The duration of the lock was the same as
|incoming_queue_lock_| except that CalculateDelayedRuntime() and
pump_->ShceduleWork() were not under the lock.

Both locks were taken if a task was posted via MessageLoopProxy. The outer lock
was taken before CalculateDelayedRuntime() was called and released after
pump_->ShceduleWork().
 
> This lock is *only* intended to protect incomming_queue_, so we should only be
> manipulating that queue while holding the lock... and to the extent possible,
do
> nothing more. Calls that might do allocations, or operating system calls,
> should probably be pulled outside if at all possible.

I agree, it is a great point. We should be careful about lifetimes of
MessageLoop and MessagePump though. It makes sense to use a separate lock to
protect ScheduleWork(). The problem is where to put it. MessageLoop can be
destroyed at the same time as another thread is trying to wake up the message
pump.

https://chromiumcodereview.appspot.com/17567007/diff/188001/base/message_loop...
base/message_loop/incoming_task_queue.cc:163:
message_loop_->ScheduleWork(was_empty);
On 2013/07/29 23:30:50, jar1 wrote:
> The lock was really only intended to
> protect the queue....

This is new code, so not really. The lock was intended to protect ScheduleWork()
and message pump's lifetime as well. Now I agree that is not very good from
performance standpoint. 

> and they work done should be very quickly
> finished.

This is documented next to MessagePump::ScheduleWork() declaration. 

I think it should be possible to have a separate lock protecting MessageLoop's
registration with IncomingTaskQueue. It will synchronize ScheduleWork() calls
and message pump destruction as well.

https://chromiumcodereview.appspot.com/17567007/diff/188001/base/message_loop...
File base/message_loop/message_loop.cc (left):

https://chromiumcodereview.appspot.com/17567007/diff/188001/base/message_loop...
base/message_loop/message_loop.cc:628: AutoLock locked(incoming_queue_lock_,
AutoLock::AlreadyAcquired());
On 2013/07/29 23:30:50, jar1 wrote:
> Specifically in this case, care was taken to release the lock on line 646
before
> calling ScheduleWork().

Yes, I know. Unfortunately that caused a race down in Win32 code when one thread
was destroying the message loop, while the other was executing ScheduleWork().