ash: Do not use MessageLoopForUI when not needed.

ash: Do not use MessageLoopForUI when not needed.

Use ThreadTaskRunnerHandle to get thet task runner, and use that for
posting tasks from the ash code.

BUG=none

Committed: https://crrev.com/d3af96a7d69ccf2a6a0387f31074d9b5482f9a6f
Cr-Commit-Position: refs/heads/master@{#372096}

[sadrul, 2016-01-27 00:45:57]

sadrul@chromium.org changed reviewers:
+ oshima@chromium.org

[oshima, 2016-01-27 22:23:53]

change looks reasonable. Can you look into bot failures? (may not be related to
your CL, but want to double check).

One question below.

https://codereview.chromium.org/1640503002/diff/1/ash/accelerators/key_hold_d...
File ash/accelerators/key_hold_detector.cc (left):

https://codereview.chromium.org/1640503002/diff/1/ash/accelerators/key_hold_d...
ash/accelerators/key_hold_detector.cc:40:
base::MessageLoopForUI::current()->PostTask(
My understanding was that we used to use MessageLoopForUI::current() as a safe
guard to make sure that UI code runs on single UI thread.

I understand that it does not really have to be "UI thread" in new world, but
still should be single thread. Is there a way to make sure they runs on the same
single thread?

[sadrul, 2016-01-27 23:44:02]

https://codereview.chromium.org/1640503002/diff/1/ash/accelerators/key_hold_d...
File ash/accelerators/key_hold_detector.cc (left):

https://codereview.chromium.org/1640503002/diff/1/ash/accelerators/key_hold_d...
ash/accelerators/key_hold_detector.cc:40:
base::MessageLoopForUI::current()->PostTask(
On 2016/01/27 22:23:53, oshima wrote:
> My understanding was that we used to use MessageLoopForUI::current() as a safe
> guard to make sure that UI code runs on single UI thread.
> 
> I understand that it does not really have to be "UI thread" in new world, but
> still should be single thread. Is there a way to make sure they runs on the
same
> single thread?

ThreadTaskRunnerHandle::Get() returns the task-runner for the current thread,
and it's a SingleThreadTaskRunner instance. So we are guaranteed to run the task
(i.e. DispatchPressedEvent) in the same thread as this code (i.e.
PostPressedEvent).

Is that what you mean? We can also use base::ThreadChecker to verify that parts
of the code are running in the thread we expect, but it's not clear where we
would use that in this case.

[sadrul, 2016-01-27 23:49:37]

On 2016/01/27 22:23:53, oshima wrote:
> change looks reasonable. Can you look into bot failures? (may not be related
to
> your CL, but want to double check).

Looking at the bot failures.

[sadrul, 2016-01-28 00:19:35]

On 2016/01/27 23:49:37, sadrul wrote:
> On 2016/01/27 22:23:53, oshima wrote:
> > change looks reasonable. Can you look into bot failures? (may not be related
> to
> > your CL, but want to double check).
> 
> Looking at the bot failures.

Looks unrelated. Triggered another build on the failing bots, and they passed.

[oshima, 2016-01-28 00:38:32]

lgtm

On 2016/01/27 23:44:02, sadrul wrote:
>
https://codereview.chromium.org/1640503002/diff/1/ash/accelerators/key_hold_d...
> File ash/accelerators/key_hold_detector.cc (left):
> 
>
https://codereview.chromium.org/1640503002/diff/1/ash/accelerators/key_hold_d...
> ash/accelerators/key_hold_detector.cc:40:
> base::MessageLoopForUI::current()->PostTask(
> On 2016/01/27 22:23:53, oshima wrote:
> > My understanding was that we used to use MessageLoopForUI::current() as a
safe
> > guard to make sure that UI code runs on single UI thread.
> > 
> > I understand that it does not really have to be "UI thread" in new world,
but
> > still should be single thread. Is there a way to make sure they runs on the
> same
> > single thread?
> 
> ThreadTaskRunnerHandle::Get() returns the task-runner for the current thread,
> and it's a SingleThreadTaskRunner instance. So we are guaranteed to run the
task
> (i.e. DispatchPressedEvent) in the same thread as this code (i.e.
> PostPressedEvent).
> 
> Is that what you mean? We can also use base::ThreadChecker to verify that
parts
> of the code are running in the thread we expect, but it's not clear where we
> would use that in this case.

I was thinking of a scenario where ash client code call ash API from non UI
thread by accident.
I believe MessageLoopForUI::current() fails on that case, but
threadTaskRunnerHandle::Get() uses whatever
is associated with the current thread?

[sadrul, 2016-01-28 04:16:45]

On 2016/01/28 00:38:32, oshima wrote:
> lgtm
> 
> On 2016/01/27 23:44:02, sadrul wrote:
> >
>
https://codereview.chromium.org/1640503002/diff/1/ash/accelerators/key_hold_d...
> > File ash/accelerators/key_hold_detector.cc (left):
> > 
> >
>
https://codereview.chromium.org/1640503002/diff/1/ash/accelerators/key_hold_d...
> > ash/accelerators/key_hold_detector.cc:40:
> > base::MessageLoopForUI::current()->PostTask(
> > On 2016/01/27 22:23:53, oshima wrote:
> > > My understanding was that we used to use MessageLoopForUI::current() as a
> safe
> > > guard to make sure that UI code runs on single UI thread.
> > > 
> > > I understand that it does not really have to be "UI thread" in new world,
> but
> > > still should be single thread. Is there a way to make sure they runs on
the
> > same
> > > single thread?
> > 
> > ThreadTaskRunnerHandle::Get() returns the task-runner for the current
thread,
> > and it's a SingleThreadTaskRunner instance. So we are guaranteed to run the
> task
> > (i.e. DispatchPressedEvent) in the same thread as this code (i.e.
> > PostPressedEvent).
> > 
> > Is that what you mean? We can also use base::ThreadChecker to verify that
> parts
> > of the code are running in the thread we expect, but it's not clear where we
> > would use that in this case.
> 
> I was thinking of a scenario where ash client code call ash API from non UI
> thread by accident.
> I believe MessageLoopForUI::current() fails on that case, but
> threadTaskRunnerHandle::Get() uses whatever
> is associated with the current thread?

ThreadTaskRunnerHandle::Get() indeed uses whatever is associated with the
current thread, and you are right that if these specific functions are called on
a non-ui thread, then we would no longer catch these errors. But I think the ash
API surface is pretty large, and the best we can do is to use thread-checkers in
places that may be at a higher risk of being misused by multiple threads to
guard against this problem.

[oshima, 2016-01-28 04:33:33]

On 2016/01/28 04:16:45, sadrul wrote:
> On 2016/01/28 00:38:32, oshima wrote:
> > lgtm
> > 
> > On 2016/01/27 23:44:02, sadrul wrote:
> > >
> >
>
https://codereview.chromium.org/1640503002/diff/1/ash/accelerators/key_hold_d...
> > > File ash/accelerators/key_hold_detector.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/1640503002/diff/1/ash/accelerators/key_hold_d...
> > > ash/accelerators/key_hold_detector.cc:40:
> > > base::MessageLoopForUI::current()->PostTask(
> > > On 2016/01/27 22:23:53, oshima wrote:
> > > > My understanding was that we used to use MessageLoopForUI::current() as
a
> > safe
> > > > guard to make sure that UI code runs on single UI thread.
> > > > 
> > > > I understand that it does not really have to be "UI thread" in new
world,
> > but
> > > > still should be single thread. Is there a way to make sure they runs on
> the
> > > same
> > > > single thread?
> > > 
> > > ThreadTaskRunnerHandle::Get() returns the task-runner for the current
> thread,
> > > and it's a SingleThreadTaskRunner instance. So we are guaranteed to run
the
> > task
> > > (i.e. DispatchPressedEvent) in the same thread as this code (i.e.
> > > PostPressedEvent).
> > > 
> > > Is that what you mean? We can also use base::ThreadChecker to verify that
> > parts
> > > of the code are running in the thread we expect, but it's not clear where
we
> > > would use that in this case.
> > 
> > I was thinking of a scenario where ash client code call ash API from non UI
> > thread by accident.
> > I believe MessageLoopForUI::current() fails on that case, but
> > threadTaskRunnerHandle::Get() uses whatever
> > is associated with the current thread?
> 
> ThreadTaskRunnerHandle::Get() indeed uses whatever is associated with the
> current thread, and you are right that if these specific functions are called
on
> a non-ui thread, then we would no longer catch these errors. But I think the
ash
> API surface is pretty large, and the best we can do is to use thread-checkers
in
> places that may be at a higher risk of being misused by multiple threads to
> guard against this problem.

Maybe it's useful to have a task runner that works only for the same thread that
created it.

[sadrul, 2016-01-28 07:14:53]

On 2016/01/28 04:33:33, oshima wrote:
> On 2016/01/28 04:16:45, sadrul wrote:
> > On 2016/01/28 00:38:32, oshima wrote:
> > > lgtm
> > > 
> > > On 2016/01/27 23:44:02, sadrul wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1640503002/diff/1/ash/accelerators/key_hold_d...
> > > > File ash/accelerators/key_hold_detector.cc (left):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1640503002/diff/1/ash/accelerators/key_hold_d...
> > > > ash/accelerators/key_hold_detector.cc:40:
> > > > base::MessageLoopForUI::current()->PostTask(
> > > > On 2016/01/27 22:23:53, oshima wrote:
> > > > > My understanding was that we used to use MessageLoopForUI::current()
as
> a
> > > safe
> > > > > guard to make sure that UI code runs on single UI thread.
> > > > > 
> > > > > I understand that it does not really have to be "UI thread" in new
> world,
> > > but
> > > > > still should be single thread. Is there a way to make sure they runs
on
> > the
> > > > same
> > > > > single thread?
> > > > 
> > > > ThreadTaskRunnerHandle::Get() returns the task-runner for the current
> > thread,
> > > > and it's a SingleThreadTaskRunner instance. So we are guaranteed to run
> the
> > > task
> > > > (i.e. DispatchPressedEvent) in the same thread as this code (i.e.
> > > > PostPressedEvent).
> > > > 
> > > > Is that what you mean? We can also use base::ThreadChecker to verify
that
> > > parts
> > > > of the code are running in the thread we expect, but it's not clear
where
> we
> > > > would use that in this case.
> > > 
> > > I was thinking of a scenario where ash client code call ash API from non
UI
> > > thread by accident.
> > > I believe MessageLoopForUI::current() fails on that case, but
> > > threadTaskRunnerHandle::Get() uses whatever
> > > is associated with the current thread?
> > 
> > ThreadTaskRunnerHandle::Get() indeed uses whatever is associated with the
> > current thread, and you are right that if these specific functions are
called
> on
> > a non-ui thread, then we would no longer catch these errors. But I think the
> ash
> > API surface is pretty large, and the best we can do is to use
thread-checkers
> in
> > places that may be at a higher risk of being misused by multiple threads to
> > guard against this problem.
> 
> Maybe it's useful to have a task runner that works only for the same thread
that
> created it.

I believe that's the task-runner you get from ThreadTaskRunnerHandle::Get().

I think in this specific case, we want PostPressedEvent() to post the task on
the same thread it is run on, and the entry point in ash
(KeyHoldDetector::OnKeyEvent()) is expected to have been called in the right
thread. If we wanted to verify that it is being called on the right thread, then
we could add a ThreadChecker on KeyHoldDetector, to make sure OnKeyEvent() is
getting called on the same thread as it was created on. But there's a lot of
such handlers in ash, and I don't think it makes sense to add these checks
everywhere. What do you think?

Do you think we should have these checks in place before we get rid of these
MessageLoopForUI usage?

[oshima, 2016-01-28 11:01:33]

On 2016/01/28 07:14:53, sadrul wrote:
> On 2016/01/28 04:33:33, oshima wrote:
> > On 2016/01/28 04:16:45, sadrul wrote:
> > > On 2016/01/28 00:38:32, oshima wrote:
> > > > lgtm
> > > > 
> > > > On 2016/01/27 23:44:02, sadrul wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1640503002/diff/1/ash/accelerators/key_hold_d...
> > > > > File ash/accelerators/key_hold_detector.cc (left):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1640503002/diff/1/ash/accelerators/key_hold_d...
> > > > > ash/accelerators/key_hold_detector.cc:40:
> > > > > base::MessageLoopForUI::current()->PostTask(
> > > > > On 2016/01/27 22:23:53, oshima wrote:
> > > > > > My understanding was that we used to use MessageLoopForUI::current()
> as
> > a
> > > > safe
> > > > > > guard to make sure that UI code runs on single UI thread.
> > > > > > 
> > > > > > I understand that it does not really have to be "UI thread" in new
> > world,
> > > > but
> > > > > > still should be single thread. Is there a way to make sure they runs
> on
> > > the
> > > > > same
> > > > > > single thread?
> > > > > 
> > > > > ThreadTaskRunnerHandle::Get() returns the task-runner for the current
> > > thread,
> > > > > and it's a SingleThreadTaskRunner instance. So we are guaranteed to
run
> > the
> > > > task
> > > > > (i.e. DispatchPressedEvent) in the same thread as this code (i.e.
> > > > > PostPressedEvent).
> > > > > 
> > > > > Is that what you mean? We can also use base::ThreadChecker to verify
> that
> > > > parts
> > > > > of the code are running in the thread we expect, but it's not clear
> where
> > we
> > > > > would use that in this case.
> > > > 
> > > > I was thinking of a scenario where ash client code call ash API from non
> UI
> > > > thread by accident.
> > > > I believe MessageLoopForUI::current() fails on that case, but
> > > > threadTaskRunnerHandle::Get() uses whatever
> > > > is associated with the current thread?
> > > 
> > > ThreadTaskRunnerHandle::Get() indeed uses whatever is associated with the
> > > current thread, and you are right that if these specific functions are
> called
> > on
> > > a non-ui thread, then we would no longer catch these errors. But I think
the
> > ash
> > > API surface is pretty large, and the best we can do is to use
> thread-checkers
> > in
> > > places that may be at a higher risk of being misused by multiple threads
to
> > > guard against this problem.
> > 
> > Maybe it's useful to have a task runner that works only for the same thread
> that
> > created it.
> 
> I believe that's the task-runner you get from ThreadTaskRunnerHandle::Get().
> 
> I think in this specific case, we want PostPressedEvent() to post the task on
> the same thread it is run on, and the entry point in ash
> (KeyHoldDetector::OnKeyEvent()) is expected to have been called in the right
> thread. If we wanted to verify that it is being called on the right thread,
then
> we could add a ThreadChecker on KeyHoldDetector, to make sure OnKeyEvent() is
> getting called on the same thread as it was created on. But there's a lot of
> such handlers in ash, and I don't think it makes sense to add these checks
> everywhere. What do you think?

What I had in my mind was an object that will fail if I call "PostTask" from
wrong thread.
Ideally, it'd be nice if we can do

TaskRunner<UI>::Set(TaskRunner*)    // or "UI" can be Ash

in a init code, then

TaskRUnner<UI>::Get()->PostTask()   // Fails if you call from different thread.

That way, you don't need check everywhere.

In any case, that's a potential improvement we can consider in the future, so
please go ahead.

> 
> Do you think we should have these checks in place before we get rid of these
> MessageLoopForUI usage?

[sadrul, 2016-01-28 15:54:39]

On 2016/01/28 11:01:33, oshima wrote:
> On 2016/01/28 07:14:53, sadrul wrote:
> > On 2016/01/28 04:33:33, oshima wrote:
> > > On 2016/01/28 04:16:45, sadrul wrote:
> > > > On 2016/01/28 00:38:32, oshima wrote:
> > > > > lgtm
> > > > > 
> > > > > On 2016/01/27 23:44:02, sadrul wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1640503002/diff/1/ash/accelerators/key_hold_d...
> > > > > > File ash/accelerators/key_hold_detector.cc (left):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1640503002/diff/1/ash/accelerators/key_hold_d...
> > > > > > ash/accelerators/key_hold_detector.cc:40:
> > > > > > base::MessageLoopForUI::current()->PostTask(
> > > > > > On 2016/01/27 22:23:53, oshima wrote:
> > > > > > > My understanding was that we used to use
MessageLoopForUI::current()
> > as
> > > a
> > > > > safe
> > > > > > > guard to make sure that UI code runs on single UI thread.
> > > > > > > 
> > > > > > > I understand that it does not really have to be "UI thread" in new
> > > world,
> > > > > but
> > > > > > > still should be single thread. Is there a way to make sure they
runs
> > on
> > > > the
> > > > > > same
> > > > > > > single thread?
> > > > > > 
> > > > > > ThreadTaskRunnerHandle::Get() returns the task-runner for the
current
> > > > thread,
> > > > > > and it's a SingleThreadTaskRunner instance. So we are guaranteed to
> run
> > > the
> > > > > task
> > > > > > (i.e. DispatchPressedEvent) in the same thread as this code (i.e.
> > > > > > PostPressedEvent).
> > > > > > 
> > > > > > Is that what you mean? We can also use base::ThreadChecker to verify
> > that
> > > > > parts
> > > > > > of the code are running in the thread we expect, but it's not clear
> > where
> > > we
> > > > > > would use that in this case.
> > > > > 
> > > > > I was thinking of a scenario where ash client code call ash API from
non
> > UI
> > > > > thread by accident.
> > > > > I believe MessageLoopForUI::current() fails on that case, but
> > > > > threadTaskRunnerHandle::Get() uses whatever
> > > > > is associated with the current thread?
> > > > 
> > > > ThreadTaskRunnerHandle::Get() indeed uses whatever is associated with
the
> > > > current thread, and you are right that if these specific functions are
> > called
> > > on
> > > > a non-ui thread, then we would no longer catch these errors. But I think
> the
> > > ash
> > > > API surface is pretty large, and the best we can do is to use
> > thread-checkers
> > > in
> > > > places that may be at a higher risk of being misused by multiple threads
> to
> > > > guard against this problem.
> > > 
> > > Maybe it's useful to have a task runner that works only for the same
thread
> > that
> > > created it.
> > 
> > I believe that's the task-runner you get from ThreadTaskRunnerHandle::Get().
> > 
> > I think in this specific case, we want PostPressedEvent() to post the task
on
> > the same thread it is run on, and the entry point in ash
> > (KeyHoldDetector::OnKeyEvent()) is expected to have been called in the right
> > thread. If we wanted to verify that it is being called on the right thread,
> then
> > we could add a ThreadChecker on KeyHoldDetector, to make sure OnKeyEvent()
is
> > getting called on the same thread as it was created on. But there's a lot of
> > such handlers in ash, and I don't think it makes sense to add these checks
> > everywhere. What do you think?
> 
> What I had in my mind was an object that will fail if I call "PostTask" from
> wrong thread.
> Ideally, it'd be nice if we can do
> 
> TaskRunner<UI>::Set(TaskRunner*)    // or "UI" can be Ash
> 
> in a init code, then
> 
> TaskRUnner<UI>::Get()->PostTask()   // Fails if you call from different
thread.
> 
> That way, you don't need check everywhere.

Interesting, yeah. Sounds like it'd be equivalent to
DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI)) (except AshThread instead
of BrowserThread), before a post-task.

> 
> In any case, that's a potential improvement we can consider in the future, so
> please go ahead.

Cool. Thanks!

> 
> > 
> > Do you think we should have these checks in place before we get rid of these
> > MessageLoopForUI usage?

[sadrul, 2016-01-28 15:55:50]

The CQ bit was checked by sadrul@chromium.org

[commit-bot: I haz the power, 2016-01-28 15:56:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1640503002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1640503002/1

[commit-bot: I haz the power, 2016-01-28 16:16:31]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-01-28 16:17:46]

Description was changed from

==========
ash: Do not use MessageLoopForUI when not needed.

Use ThreadTaskRunnerHandle to get thet task runner, and use that for
posting tasks from the ash code.

BUG=none
==========

to

==========
ash: Do not use MessageLoopForUI when not needed.

Use ThreadTaskRunnerHandle to get thet task runner, and use that for
posting tasks from the ash code.

BUG=none

Committed: https://crrev.com/d3af96a7d69ccf2a6a0387f31074d9b5482f9a6f
Cr-Commit-Position: refs/heads/master@{#372096}
==========

[commit-bot: I haz the power, 2016-01-28 16:17:47]

Patchset 1 (id:??) landed as
https://crrev.com/d3af96a7d69ccf2a6a0387f31074d9b5482f9a6f
Cr-Commit-Position: refs/heads/master@{#372096}