Prefer returning a SingleThreadTaskRunner in SequencedTaskRunnerHandle::Get().

Prefer returning a SingleThreadTaskRunner in SequencedTaskRunnerHandle::Get().

Currently:
- When a thread has a SingleThreadTaskRunner but not
  SequencedTaskRunner, SequencedTaskRunnerHandle::Get() and
  ThreadTaskRunnerHandle::Get() are equivalent.
- ThreadTaskRunnerHandle::Get() can only be called from a thread that
  has a SingleThreadTaskRunner.
- Having both a SequencedTaskRunner and a SingleThreadTaskRunner on the
  same thread is prevented by a DCHECK.
That means that replacing ThreadTaskRunnerHandle::Get() with
SequencedTaskRunnerHandle::Get() shouldn't change anything.

Unfortunately, RLZ sets a SingleThreadTaskRunner on a thread that
already has a SequencedTaskRunner.

To make the replacement of ThreadTaskRunnerHandle::Get() with
SequencedTaskRunnerHandle::Get() a true no-op under the assumption
that a SingleThreadTaskRunner can be registered on a thread that
already has a SequencedTaskRunner, this CL gives priority to the
SingleThreadTaskRunner in SequencedTaskRunnerHandle::Get().

This change will be reverted once RLZ stops setting a
SingleThreadTaskRunner on a thread that already has a
SequencedTaskRunner.

BUG=693711, 618530
Review-Url: https://codereview.chromium.org/2714813005
Cr-Commit-Position: refs/heads/master@{#452980}
Committed: https://chromium.googlesource.com/chromium/src/+/533c31e78be276dcaa843dc66efd4fee697d9573

[fdoray, 2017-02-24 14:11:38]

fdoray@chromium.org changed reviewers:
+ gab@chromium.org, pkl@chromium.org

[fdoray, 2017-02-24 14:11:39]

PTAL

[gab, 2017-02-24 16:34:42]

https://codereview.chromium.org/2714813005/diff/1/base/threading/sequenced_ta...
File base/threading/sequenced_task_runner_handle.cc (left):

https://codereview.chromium.org/2714813005/diff/1/base/threading/sequenced_ta...
base/threading/sequenced_task_runner_handle.cc:46: return sequenced_task_runner;
Interesting: just realized this didn't DCHECK !TTRH::IsSet(): probably explains
why RLZ hits this more recently (i.e. it's hit behind task_scheduler redirection
only).

https://codereview.chromium.org/2714813005/diff/1/base/threading/sequenced_ta...
File base/threading/sequenced_task_runner_handle.cc (right):

https://codereview.chromium.org/2714813005/diff/1/base/threading/sequenced_ta...
base/threading/sequenced_task_runner_handle.cc:32: // https://crbug.com/618530
https://crbug.com/618530#c14 (to point directly at relevant context)

https://codereview.chromium.org/2714813005/diff/1/base/threading/sequenced_ta...
base/threading/sequenced_task_runner_handle.cc:40:
DCHECK(!ThreadTaskRunnerHandle::IsSet());
That will prevent this DCHECK from being hit (i.e. allows more problems to crop
up)...

DCHECK(!lazy_tls_ptr.Pointer()->Get() &&
!SequencedWorkerPool::GetWorkerPoolForCurrentThread()) in early return above?
Will keep checking in RLZ but I don't know that we want to remove that check...

[fdoray, 2017-02-24 17:14:00]

PTAnL

https://codereview.chromium.org/2714813005/diff/1/base/threading/sequenced_ta...
File base/threading/sequenced_task_runner_handle.cc (left):

https://codereview.chromium.org/2714813005/diff/1/base/threading/sequenced_ta...
base/threading/sequenced_task_runner_handle.cc:46: return sequenced_task_runner;
On 2017/02/24 16:34:41, gab wrote:
> Interesting: just realized this didn't DCHECK !TTRH::IsSet(): probably
explains
> why RLZ hits this more recently (i.e. it's hit behind task_scheduler
redirection
> only).

No need to add a DCHECK now as it would never be hit.

https://codereview.chromium.org/2714813005/diff/1/base/threading/sequenced_ta...
File base/threading/sequenced_task_runner_handle.cc (right):

https://codereview.chromium.org/2714813005/diff/1/base/threading/sequenced_ta...
base/threading/sequenced_task_runner_handle.cc:32: // https://crbug.com/618530
On 2017/02/24 16:34:41, gab wrote:
> https://crbug.com/618530#c14 (to point directly at relevant context)

Done.

https://codereview.chromium.org/2714813005/diff/1/base/threading/sequenced_ta...
base/threading/sequenced_task_runner_handle.cc:40:
DCHECK(!ThreadTaskRunnerHandle::IsSet());
On 2017/02/24 16:34:41, gab wrote:
> That will prevent this DCHECK from being hit (i.e. allows more problems to
crop
> up)...
> 
> DCHECK(!lazy_tls_ptr.Pointer()->Get() &&
> !SequencedWorkerPool::GetWorkerPoolForCurrentThread()) in early return above?
> Will keep checking in RLZ but I don't know that we want to remove that
check...

Done.

[gab, 2017-02-24 17:44:37]

Sad lgtm :( -- but thanks for the prompt fix for M57! Can't wait for RLZ to be
fixed.

Also link CL to issue 618530.

https://codereview.chromium.org/2714813005/diff/1/base/threading/sequenced_ta...
File base/threading/sequenced_task_runner_handle.cc (left):

https://codereview.chromium.org/2714813005/diff/1/base/threading/sequenced_ta...
base/threading/sequenced_task_runner_handle.cc:46: return sequenced_task_runner;
On 2017/02/24 17:14:00, fdoray wrote:
> On 2017/02/24 16:34:41, gab wrote:
> > Interesting: just realized this didn't DCHECK !TTRH::IsSet(): probably
> explains
> > why RLZ hits this more recently (i.e. it's hit behind task_scheduler
> redirection
> > only).
> 
> No need to add a DCHECK now as it would never be hit.

Agreed, just an observation.

[fdoray, 2017-02-24 19:07:19]

Description was changed from

==========
Prefer returning a SingleThreadTaskRunner in SequencedTaskRunnerHandle::Get().

Currently:
- When a thread has a SingleThreadTaskRunner but not
  SequencedTaskRunner, SequencedTaskRunnerHandle::Get() and
  ThreadTaskRunnerHandle::Get() are equivalent.
- ThreadTaskRunnerHandle::Get() can only be called from a thread that
  has a SingleThreadTaskRunner.
- Having both a SequencedTaskRunner and a SingleThreadTaskRunner on the
  same thread is prevented by a DCHECK.
That means that replacing ThreadTaskRunnerHandle::Get() with
SequencedTaskRunnerHandle::Get() shouldn't change anything.

Unfortunately, RLZ sets a SingleThreadTaskRunner on a thread that
already has a SequencedTaskRunner.

To make the replacement of ThreadTaskRunnerHandle::Get() with
SequencedTaskRunnerHandle::Get() a true no-op under the assumption
that a SingleThreadTaskRunner can be registered on a thread that
already has a SequencedTaskRunner, this CL gives priority to the
SingleThreadTaskRunner in SequencedTaskRunnerHandle::Get().

This change will be reverted once RLZ stops setting a
SingleThreadTaskRunner on a thread that already has a
SequencedTaskRunner.

BUG=693711
==========

to

==========
Prefer returning a SingleThreadTaskRunner in SequencedTaskRunnerHandle::Get().

Currently:
- When a thread has a SingleThreadTaskRunner but not
  SequencedTaskRunner, SequencedTaskRunnerHandle::Get() and
  ThreadTaskRunnerHandle::Get() are equivalent.
- ThreadTaskRunnerHandle::Get() can only be called from a thread that
  has a SingleThreadTaskRunner.
- Having both a SequencedTaskRunner and a SingleThreadTaskRunner on the
  same thread is prevented by a DCHECK.
That means that replacing ThreadTaskRunnerHandle::Get() with
SequencedTaskRunnerHandle::Get() shouldn't change anything.

Unfortunately, RLZ sets a SingleThreadTaskRunner on a thread that
already has a SequencedTaskRunner.

To make the replacement of ThreadTaskRunnerHandle::Get() with
SequencedTaskRunnerHandle::Get() a true no-op under the assumption
that a SingleThreadTaskRunner can be registered on a thread that
already has a SequencedTaskRunner, this CL gives priority to the
SingleThreadTaskRunner in SequencedTaskRunnerHandle::Get().

This change will be reverted once RLZ stops setting a
SingleThreadTaskRunner on a thread that already has a
SequencedTaskRunner.

BUG=693711, 618530
==========

[fdoray, 2017-02-24 19:07:24]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-02-24 19:07:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pkl (ping after 24h if needed), 2017-02-24 19:16:15]

LGTM. Thank you for this!

[commit-bot: I haz the power, 2017-02-24 19:29:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-24 19:29:18]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[fdoray, 2017-02-24 21:43:04]

The CQ bit was checked by fdoray@chromium.org

[commit-bot: I haz the power, 2017-02-24 21:44:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-24 23:07:45]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487972584926000,
"parent_rev": "12a3f61803fefb0aeb06f3f4093ea8b06a29fbb3", "commit_rev":
"533c31e78be276dcaa843dc66efd4fee697d9573"}

[commit-bot: I haz the power, 2017-02-24 23:08:26]

Description was changed from

==========
Prefer returning a SingleThreadTaskRunner in SequencedTaskRunnerHandle::Get().

Currently:
- When a thread has a SingleThreadTaskRunner but not
  SequencedTaskRunner, SequencedTaskRunnerHandle::Get() and
  ThreadTaskRunnerHandle::Get() are equivalent.
- ThreadTaskRunnerHandle::Get() can only be called from a thread that
  has a SingleThreadTaskRunner.
- Having both a SequencedTaskRunner and a SingleThreadTaskRunner on the
  same thread is prevented by a DCHECK.
That means that replacing ThreadTaskRunnerHandle::Get() with
SequencedTaskRunnerHandle::Get() shouldn't change anything.

Unfortunately, RLZ sets a SingleThreadTaskRunner on a thread that
already has a SequencedTaskRunner.

To make the replacement of ThreadTaskRunnerHandle::Get() with
SequencedTaskRunnerHandle::Get() a true no-op under the assumption
that a SingleThreadTaskRunner can be registered on a thread that
already has a SequencedTaskRunner, this CL gives priority to the
SingleThreadTaskRunner in SequencedTaskRunnerHandle::Get().

This change will be reverted once RLZ stops setting a
SingleThreadTaskRunner on a thread that already has a
SequencedTaskRunner.

BUG=693711, 618530
==========

to

==========
Prefer returning a SingleThreadTaskRunner in SequencedTaskRunnerHandle::Get().

Currently:
- When a thread has a SingleThreadTaskRunner but not
  SequencedTaskRunner, SequencedTaskRunnerHandle::Get() and
  ThreadTaskRunnerHandle::Get() are equivalent.
- ThreadTaskRunnerHandle::Get() can only be called from a thread that
  has a SingleThreadTaskRunner.
- Having both a SequencedTaskRunner and a SingleThreadTaskRunner on the
  same thread is prevented by a DCHECK.
That means that replacing ThreadTaskRunnerHandle::Get() with
SequencedTaskRunnerHandle::Get() shouldn't change anything.

Unfortunately, RLZ sets a SingleThreadTaskRunner on a thread that
already has a SequencedTaskRunner.

To make the replacement of ThreadTaskRunnerHandle::Get() with
SequencedTaskRunnerHandle::Get() a true no-op under the assumption
that a SingleThreadTaskRunner can be registered on a thread that
already has a SequencedTaskRunner, this CL gives priority to the
SingleThreadTaskRunner in SequencedTaskRunnerHandle::Get().

This change will be reverted once RLZ stops setting a
SingleThreadTaskRunner on a thread that already has a
SequencedTaskRunner.

BUG=693711, 618530

Review-Url: https://codereview.chromium.org/2714813005
Cr-Commit-Position: refs/heads/master@{#452980}
Committed:
https://chromium.googlesource.com/chromium/src/+/533c31e78be276dcaa843dc66efd...
==========

[commit-bot: I haz the power, 2017-02-24 23:08:27]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/533c31e78be276dcaa843dc66efd...