base: Let ThreadTaskRunnerHandle::Get() return NULL

base: Let ThreadTaskRunnerHandle::Get() return NULL

We are migrating clients of MessageLoopProxy to ThreadTaskRunnerHandle
as the former class is deprecated. One difference between the classes is
that MessageLoopProxy::current() can return NULL if the current thread
has no active message loop, whereas ThreadTaskRunnerHandle::Get() will
fail a DCHECK if it is called without an active task runner. There are
at least four ways to work around this during the migration:

1. Vet all callers of MessageLoopProxy::current() to see if they expect
   to run in a context without a message loop. If so, modify them to
   check for the presence of a task runner with
   ThreadTaskRunnerHandle::IsSet() before calling
   ThreadTaskRunnerHandle::Get(). This is infeasible due to the number
   of call sites.

2. Modify all call sites to call IsSet() before Get(). Not great
   because adds lots of visual noise and boilerplate.

3. Add some new getter to ThreadTaskRunnerHandle which is allowed to
   return NULL. Unsure what to call this without making calling code
   overly verbose.

4. Change to contract of ThreadTaskRunnerHandle::Get() so that it is
   allowed to return NULL. This allows automatic migration of
   MessageLoopProxy callers to ThreadTaskRunnerHandle and does not
   affect any current users of ThreadTaskRunnerHandle.

This patch implements option #4. As a followup we might want to remove
IsSet() since it is somewhat redundant (although it does read better at
the call site).

BUG=465354

[Sami, 2015-04-10 11:04:09]

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

[Sami, 2015-04-10 11:05:15]

WDYT?

[danakj, 2015-04-10 17:52:53]

danakj@chromium.org changed reviewers:
+ rsleevi@chromium.org

[danakj, 2015-04-10 17:52:59]

+rsleevi for thoughts

[danakj, 2015-04-10 18:02:12]

This seems cool to me, but I have no grasp of what could go wrong here.. would
love to hear from jam/ryan.

[Sami, 2015-04-10 18:09:46]

Thanks Dana. I also wasn't able to find out why these two functions have
different semantics to begin with so any historical tidbits are appreciated :)

[Ryan Sleevi, 2015-04-10 21:13:24]

On 2015/04/10 18:09:46, Sami wrote:
> Thanks Dana. I also wasn't able to find out why these two functions have
> different semantics to begin with so any historical tidbits are appreciated :)

In every context I looked at, code which let MessageLoopProxy::current() return
NULL was broken in some way, or was done so because the author's didn't want to
fix tests.

I made an attempt to clean up //base, and got LGTMs, but then got pulled into
multiple security incidents and pre-emptions that didn't give me time to go back
and finally land. You can see it in https://codereview.chromium.org/369703003/

TL;DR: is that there were only two places I could find where it was actually,
trully legit for MLP to return NULL. The rest was bugged.

I'm not a fan of this, in part because I think it hides real design issues and
broken tests.

[danakj, 2015-04-10 21:16:34]

On Fri, Apr 10, 2015 at 2:13 PM, <rsleevi@chromium.org> wrote:

> On 2015/04/10 18:09:46, Sami wrote:
>
>> Thanks Dana. I also wasn't able to find out why these two functions have
>> different semantics to begin with so any historical tidbits are
>> appreciated :)
>>
>
> In every context I looked at, code which let MessageLoopProxy::current()
> return
> NULL was broken in some way, or was done so because the author's didn't
> want to
> fix tests.
>
> I made an attempt to clean up //base, and got LGTMs, but then got pulled
> into
> multiple security incidents and pre-emptions that didn't give me time to
> go back
> and finally land. You can see it in https://codereview.chromium.
> org/369703003/
>
> TL;DR: is that there were only two places I could find where it was
> actually,
> trully legit for MLP to return NULL. The rest was bugged.
>
> I'm not a fan of this, in part because I think it hides real design issues
> and
> broken tests.
>

When I reviewed the cc/ patch based on this, I also felt like... if it's
null we should just pass nullptr in the tests. So I'm glad to see a similar
opinion here.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2015-04-13 14:28:27]

I agree with Ryan that it's best to not have NULL returns for these getters; we
are adding extra complexity to production code just for tests. There are easy
way to instantiate MLs for tests already through TestBrowserThreadBundle

[Sami, 2015-04-13 16:19:17]

On 2015/04/13 14:28:27, jam wrote:
> I agree with Ryan that it's best to not have NULL returns for these getters;
we
> are adding extra complexity to production code just for tests. There are easy
> way to instantiate MLs for tests already through TestBrowserThreadBundle

Ah, I didn't realize that would mainly happen in tests. If that's the case then
I can see how much effort it is to fix the affected tests. However if there are
cases where we hit this in production code that isn't tested, then those code
paths will trigger a crash crash (not just a DCHECK failure). Luckily the crash
dump should be fairly obvious to decode.