base: Make it possible to replace the MessageLoop's task runner

base: Make it possible to replace the MessageLoop's task runner

This patch makes it possible to customize the task posting behavior
of a MessageLoop. More specifically, a client can change the value
returned by MessageLoop::task_runner() as well as
ThreadTaskRunnerHandle::Get() on the target thread. The original task
runner can still be used to post tasks to be run on the message loop.

The Blink/renderer scheduler will use this functionality to manage task
posting on the renderer main thread. This is needed to ensure consistent
ordering of tasks posted through the MessageLoop w.r.t. tasks posted to
the scheduler.

Design doc: https://docs.google.com/a/chromium.org/document/d/1qxdh2I61_aB_Uzh1QgNqvdWFBCL_E65G2smoSySw7KU/edit#

Alex Clarke <alexclarke@chromium.org>; also contributed to this patch (https://codereview.chromium.org/1206893003/).

BUG=465354
Committed: https://crrev.com/85f43a3185f38eb89c63ad108fd1b73000e0ac56
Cr-Commit-Position: refs/heads/master@{#338564}

[Sami, 2015-03-13 17:49:42]

skyostil@chromium.org changed reviewers:
+ jam@chromium.org

[Sami, 2015-03-13 17:51:04]

How does this approach look? I think I still want to eradicate ML::PostTask
before going ahead with this but I wanted to get your opinion first.

[jam, 2015-03-13 18:07:52]

On 2015/03/13 17:51:04, Sami wrote:
> How does this approach look? I think I still want to eradicate ML::PostTask
> before going ahead with this but I wanted to get your opinion first.

We shouldn't add more usage of MessageLoopProxy, that should go away in favor of
SingleThreadTaskRunner. ideally with the work to remove MessageLoop::PostTask,
we'd also remove MessageLoopProxy and then only have one way of posting tasks
using a ML (getting its task runner).

[Sami, 2015-03-13 18:13:15]

On 2015/03/13 18:07:52, jam wrote:
> On 2015/03/13 17:51:04, Sami wrote:
> > How does this approach look? I think I still want to eradicate ML::PostTask
> > before going ahead with this but I wanted to get your opinion first.
> 
> We shouldn't add more usage of MessageLoopProxy, that should go away in favor
of
> SingleThreadTaskRunner. ideally with the work to remove MessageLoop::PostTask,
> we'd also remove MessageLoopProxy and then only have one way of posting tasks
> using a ML (getting its task runner).

Fair enough. In that world, if you do an
s/MessageLoopProxy/SingleThreadTaskRunner/g to this patch, would it look
reasonable?

[jam, 2015-03-13 21:31:05]

On 2015/03/13 18:13:15, Sami wrote:
> On 2015/03/13 18:07:52, jam wrote:
> > On 2015/03/13 17:51:04, Sami wrote:
> > > How does this approach look? I think I still want to eradicate
ML::PostTask
> > > before going ahead with this but I wanted to get your opinion first.
> > 
> > We shouldn't add more usage of MessageLoopProxy, that should go away in
favor
> of
> > SingleThreadTaskRunner. ideally with the work to remove
MessageLoop::PostTask,
> > we'd also remove MessageLoopProxy and then only have one way of posting
tasks
> > using a ML (getting its task runner).
> 
> Fair enough. In that world, if you do an
> s/MessageLoopProxy/SingleThreadTaskRunner/g to this patch, would it look
> reasonable?

sure. i would probably avoid a new constructor and add a setter for the task
runner since it seems simpler to keep ML construction the same.

[Sami, 2015-06-19 20:55:22]

skyostil@chromium.org changed reviewers:
+ kinuko@chromium.org

[Sami, 2015-06-19 20:56:21]

skyostil@chromium.org changed reviewers:
+ danakj@chromium.org
- kinuko@chromium.org

[Sami, 2015-06-19 21:06:00]

*pops stack*

I've now updated this to a post-MLP world. I'm now adding a setter function for
the task runner instead of going through the constructor. I don't have a huge
preference either way, other than this requires fewer contortions and is in
theory more composable since you can chain several task runners together.

Kinuko, I noticed CreateUnbound wasn't being used anywhere. I made it static so
I could use it in the tests, but we could also remove it isn't needed anymore.

[Sami, 2015-06-19 21:17:35]

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

https://codereview.chromium.org/998063002/diff/60001/base/message_loop/messag...
base/message_loop/message_loop.h:300: // once the internal type matches what is
being returned (crbug.com/465354).
N.B. This can't return a BindableSingleThreadTaskRunner since it breaks these
types of constructs:

  scoped_refptr<SingleThreadTaskRunner> foo = bar ? message_loop.task_runner() :
something_else.task_runner();

The problem seems to be that the types become ambiguous, but I'm not sure why it
doesn't just default to the common base class:

error: conditional expression is ambiguous; 'const
scoped_refptr<base::SingleThreadTaskRunner>' can be converted to
'scoped_refptr<base::BindableSingleThreadTaskRunner>' and vice versa

[alexclarke, 2015-06-24 10:27:03]

alexclarke@google.com changed reviewers:
+ alexclarke@google.com

[alexclarke, 2015-06-24 10:27:04]

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

https://codereview.chromium.org/998063002/diff/60001/base/message_loop/messag...
base/message_loop/message_loop.h:300: // once the internal type matches what is
being returned (crbug.com/465354).
On 2015/06/19 21:17:35, Sami (OoO) wrote:
> N.B. This can't return a BindableSingleThreadTaskRunner since it breaks these
> types of constructs:
> 
>   scoped_refptr<SingleThreadTaskRunner> foo = bar ? message_loop.task_runner()
:
> something_else.task_runner();
> 
> The problem seems to be that the types become ambiguous, but I'm not sure why
it
> doesn't just default to the common base class:
> 
> error: conditional expression is ambiguous; 'const
> scoped_refptr<base::SingleThreadTaskRunner>' can be converted to
> 'scoped_refptr<base::BindableSingleThreadTaskRunner>' and vice versa

This is kind of an annoying feature of C++.  Automatic upcasts to a base class
are fine, but an automatic downcast seems a bit iffy.  Anyway the usual solution
to this is to add an explicit cast in to coerce the
BindableSingleThreadTaskRunner to a SingleThreadTaskRunner.

Looks like there is only one such cast needed, so I think it's OK.

[kinuko, 2015-06-25 05:58:08]

kinuko@chromium.org changed reviewers:
+ kinuko@chromium.org

[kinuko, 2015-06-25 05:58:09]

Looks a lot nicer (than the original one) to me =D

Made some minor comments.

https://codereview.chromium.org/998063002/diff/60001/base/message_loop/messag...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/998063002/diff/60001/base/message_loop/messag...
base/message_loop/message_loop.cc:419: thread_task_runner_handle_.reset(new
ThreadTaskRunnerHandle(task_runner));
nit: no need to call reset() twice, line 418 is not necessary?

https://codereview.chromium.org/998063002/diff/60001/base/message_loop/messag...
base/message_loop/message_loop.cc:421:
scoped_refptr<BindableSingleThreadTaskRunner> previous_runner = task_runner_;
nit: task_runner_.Pass() might be slightly better

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

https://codereview.chromium.org/998063002/diff/60001/base/message_loop/messag...
base/message_loop/message_loop.h:429: MessagePumpFactoryCallback pump_factory);
(Aha, how I could miss this's unused)

[alexclarke, 2015-06-25 09:36:51]

Thanks for the comments Kinuko.  Sami is away on vacation so I'll be taking on
this patch for him, here:

https://codereview.chromium.org/1206893003

https://codereview.chromium.org/998063002/diff/60001/base/message_loop/messag...
File base/message_loop/message_loop.cc (right):

https://codereview.chromium.org/998063002/diff/60001/base/message_loop/messag...
base/message_loop/message_loop.cc:419: thread_task_runner_handle_.reset(new
ThreadTaskRunnerHandle(task_runner));
On 2015/06/25 05:58:08, kinuko wrote:
> nit: no need to call reset() twice, line 418 is not necessary?

Done.

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

https://codereview.chromium.org/998063002/diff/60001/base/message_loop/messag...
base/message_loop/message_loop.h:429: MessagePumpFactoryCallback pump_factory);
On 2015/06/25 05:58:08, kinuko wrote:
> (Aha, how I could miss this's unused)

Are you intending to use this, or would you like us to remove it?

[kinuko, 2015-06-25 14:31:12]

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

https://codereview.chromium.org/998063002/diff/60001/base/message_loop/messag...
base/message_loop/message_loop.h:429: MessagePumpFactoryCallback pump_factory);
On 2015/06/25 09:36:51, alexclarke wrote:
> On 2015/06/25 05:58:08, kinuko wrote:
> > (Aha, how I could miss this's unused)
> 
> Are you intending to use this, or would you like us to remove it?

I was intending to use this at least when I added, since I thought it'd make the
intention clearer.  I'll create a follow-up CL to use this in base::Thread and
ask reviewer's opinion, so would like you to keep it as is in this CL.

[Sami, 2015-07-07 13:21:33]

Thanks for all the comments in https://codereview.chromium.org/1206893003/! I'll
respond to the relevant ones here:

>On 2015/06/29 19:07:23, danakj OOO back june 28 wrote:
>> Why do we want to support both:
>> 
>> 1. SwapTaskRunner
>> 2. BindToCurrentThread
>> 
>> and
>> 
>> 1. BindToCurrentThread
>> 2. SwapTaskRunner
>> 
>> ?
>
> I don't know, and Sami is currently OOO so I can't ask him.

I originally wanted to support both for completeness, but since Kinuko pointed
out that creating an unbound message loop is private API, the unbound case isn't
really that relevant. I've now rewritten the patch accordingly and it is a lot
simpler now.

>> On 2015/06/29 19:07:23, danakj OOO back june 28 wrote:
>> > What happened to the TODO?
>> 
>> Should the fact that this is Bindable really be public? Should this instead
>> return a STTR?
>>
>>I agree the fact it's Bindable feels like an implementation detail that
ideally
>>shouldn't be public.
>>
>I think we have the following options:
>
>1. Return const scoped_refptr<BindableSingleThreadTaskRunner>&
>2. Add a base_task_runner_ as well and return const
>scoped_refptr<SingleThreadTaskRunner>& here
>3. Regard returning a const reference as micro-optimization and leave it as is,
>and remove the todo.
>
>I think I prefer #1 or #3, WDYT?

Since we don't have to expose the internal task runner type anymore, we can just
return a plain ol' SingleThreadTaskRunner here.

>On 2015/06/30 02:26:55, kinuko wrote:
>> Would we want to call this before the message loop's bound to a thread (other
>> than in test)?  
>
>Apparently yes, see SchedulerMessageLoopDelegate::SetDefaultTaskRunner in
>https://codereview.chromium.org/987193002/

Actually that only needs to run after the message loop has been bound, so we can
get away with a much simpler change here.

[danakj, 2015-07-07 21:11:13]

LGTM

[kinuko, 2015-07-08 02:22:33]

This looks neat, lgtm/2!

[Sami, 2015-07-08 10:00:49]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2015-07-08 10:01:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/998063002/120001

[commit-bot: I haz the power, 2015-07-08 10:47:12]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-07-08 10:48:06]

Patchset 7 (id:??) landed as
https://crrev.com/698e77997894bf823c45ad1d6e0764fa93e2f3c1
Cr-Commit-Position: refs/heads/master@{#337799}

[Alexander Potapenko, 2015-07-13 09:10:21]

A revert of this CL (patchset #7 id:120001) has been created in
https://codereview.chromium.org/1223193004/ by glider@chromium.org.

The reason for reverting is: Reverting this CL because of
https://crbug.com/508789
Please fix and test using the linux_chromium_tsan_rel_ng trybot..

[Sami, 2015-07-13 18:42:09]

Relanding with a fix for the data race (reviewed in
https://codereview.chromium.org/1232573002/). I've tested locally that it fixes
the tsan problem.

[Sami, 2015-07-13 18:42:45]

The CQ bit was checked by skyostil@chromium.org

[Sami, 2015-07-13 18:42:46]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/998063002/#ps140001
(title: "Fix data race (see https://codereview.chromium.org/1232573002/)")

[commit-bot: I haz the power, 2015-07-13 18:44:15]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/998063002/140001

[commit-bot: I haz the power, 2015-07-13 21:41:25]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-07-13 21:42:29]

Patchset 8 (id:??) landed as
https://crrev.com/85f43a3185f38eb89c63ad108fd1b73000e0ac56
Cr-Commit-Position: refs/heads/master@{#338564}