Assert RendererScheduler thread usage

Assert RendererScheduler thread usage

Use DCHECK to validate the ThreadChecker results in RendererScheduler.

Also add a WARN_UNUSED_RESULT modifier to
ThreadChecker::CalledOnValidThread.

Committed: https://crrev.com/8e64d3644b80a0f50b13fd7fcb534b6802df62fd
Cr-Commit-Position: refs/heads/master@{#314927}

[jdduke (slow), 2015-02-05 17:49:04]

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

[jdduke (slow), 2015-02-05 17:49:04]

Sami, looks like this flew under the radar. I think there may be some bad test
assumptions that I'm still tracking down, wonder if we just need to detach the
ThreadChecker in the constructor.

[Sami, 2015-02-05 17:51:49]

Facepalm, lgtm.

What did you mean by detaching this in the constructor?

[jdduke (slow), 2015-02-05 17:54:56]

On 2015/02/05 17:51:49, Sami wrote:
> Facepalm, lgtm.
> 
> What did you mean by detaching this in the constructor?

I'm just wondering if any of the tests create the RendererSchedulerImpl on, say,
the test thread, but it's used on another thread (in which case we'd want to
detach the ThreadChecker in the RendererSchedulerImpl constructor, and it will
be "attached" to the thread on the next validation).

[jdduke (slow), 2015-02-05 18:02:19]

New patchsets have been uploaded after l-g-t-m from skyostil@chromium.org

[Sami, 2015-02-05 18:16:45]

On 2015/02/05 17:54:56, jdduke wrote:
> On 2015/02/05 17:51:49, Sami wrote:
> > Facepalm, lgtm.
> > 
> > What did you mean by detaching this in the constructor?
> 
> I'm just wondering if any of the tests create the RendererSchedulerImpl on,
say,
> the test thread, but it's used on another thread (in which case we'd want to
> detach the ThreadChecker in the RendererSchedulerImpl constructor, and it will
> be "attached" to the thread on the next validation).

I don't think that's a pattern we want to support anyway. The task queue manager
also latches on to the thread you create it on so migrating to another thread
would be tricky.

[Sami, 2015-02-05 18:16:54]

https://codereview.chromium.org/904593002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.cc (left):

https://codereview.chromium.org/904593002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:296:
thread_checker_.CalledOnValidThread();
Ross, could you double check this is what we want to do?

[jdduke (slow), 2015-02-05 18:18:16]

On 2015/02/05 18:16:45, Sami wrote:
> On 2015/02/05 17:54:56, jdduke wrote:
> > On 2015/02/05 17:51:49, Sami wrote:
> > > Facepalm, lgtm.
> > > 
> > > What did you mean by detaching this in the constructor?
> > 
> > I'm just wondering if any of the tests create the RendererSchedulerImpl on,
> say,
> > the test thread, but it's used on another thread (in which case we'd want to
> > detach the ThreadChecker in the RendererSchedulerImpl constructor, and it
will
> > be "attached" to the thread on the next validation).
> 
> I don't think that's a pattern we want to support anyway. The task queue
manager
> also latches on to the thread you create it on so migrating to another thread
> would be tricky.

I was able to repro locally.

We don't need to detach, the problem is that WebSchedulerImpl::postLoadingTask
is getting called from a worker thread (which in turn calls
renderer_scheduler_->LoadingTaskRunner()).

[jdduke (slow), 2015-02-05 18:19:18]

https://codereview.chromium.org/904593002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.cc (left):

https://codereview.chromium.org/904593002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:296:
thread_checker_.CalledOnValidThread();
On 2015/02/05 18:16:54, Sami wrote:
> Ross, could you double check this is what we want to do?

This seemed unnecessary, as the class is internal only and |IsSet()| is already
guarded by a ThreadChecker.

[Sami, 2015-02-05 18:23:07]

On 2015/02/05 18:18:16, jdduke wrote:
> On 2015/02/05 18:16:45, Sami wrote:
> > On 2015/02/05 17:54:56, jdduke wrote:
> > > On 2015/02/05 17:51:49, Sami wrote:
> > > > Facepalm, lgtm.
> > > > 
> > > > What did you mean by detaching this in the constructor?
> > > 
> > > I'm just wondering if any of the tests create the RendererSchedulerImpl
on,
> > say,
> > > the test thread, but it's used on another thread (in which case we'd want
to
> > > detach the ThreadChecker in the RendererSchedulerImpl constructor, and it
> will
> > > be "attached" to the thread on the next validation).
> > 
> > I don't think that's a pattern we want to support anyway. The task queue
> manager
> > also latches on to the thread you create it on so migrating to another
thread
> > would be tricky.
> 
> I was able to repro locally.
> 
> We don't need to detach, the problem is that WebSchedulerImpl::postLoadingTask
> is getting called from a worker thread (which in turn calls
> renderer_scheduler_->LoadingTaskRunner()).

Ah, that sounds like an overzealous thread check. Let me qualify my earlier
statement that it's still safe to use the thread safe parts
RendererSchedulerImpl from other threads :)

[jdduke (slow), 2015-02-05 18:29:54]

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

[jdduke (slow), 2015-02-05 18:29:55]

+jam@ for base/threading review.

[rmcilroy, 2015-02-05 18:49:59]

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

[rmcilroy, 2015-02-05 18:50:00]

https://codereview.chromium.org/904593002/diff/20001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.cc (left):

https://codereview.chromium.org/904593002/diff/20001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:296:
thread_checker_.CalledOnValidThread();
On 2015/02/05 18:19:18, jdduke wrote:
> On 2015/02/05 18:16:54, Sami wrote:
> > Ross, could you double check this is what we want to do?
> 
> This seemed unnecessary, as the class is internal only and |IsSet()| is
already
> guarded by a ThreadChecker.

Yes I think you are right, this is probably unnecessary.

https://codereview.chromium.org/904593002/diff/60001/content/renderer/schedul...
File content/renderer/scheduler/renderer_scheduler_impl.cc (left):

https://codereview.chromium.org/904593002/diff/60001/content/renderer/schedul...
content/renderer/scheduler/renderer_scheduler_impl.cc:73:
main_thread_checker_.CalledOnValidThread();
I'm not sure we can remove these checks - this is the comment from sievers@ on
the original review which caused me to add these in the first place:

  You can't access |this| from another thread because |this| might 
  get deleted while you are calling into it which will crash.
  Everybody needs to acquire a reference to the task runners from
  the main thread and then hold on to it.

[rmcilroy, 2015-02-05 18:53:06]

On 2015/02/05 18:50:00, rmcilroy wrote:
>
https://codereview.chromium.org/904593002/diff/20001/content/renderer/schedul...
> File content/renderer/scheduler/renderer_scheduler_impl.cc (left):
> 
>
https://codereview.chromium.org/904593002/diff/20001/content/renderer/schedul...
> content/renderer/scheduler/renderer_scheduler_impl.cc:296:
> thread_checker_.CalledOnValidThread();
> On 2015/02/05 18:19:18, jdduke wrote:
> > On 2015/02/05 18:16:54, Sami wrote:
> > > Ross, could you double check this is what we want to do?
> > 
> > This seemed unnecessary, as the class is internal only and |IsSet()| is
> already
> > guarded by a ThreadChecker.
> 
> Yes I think you are right, this is probably unnecessary.
> 
>
https://codereview.chromium.org/904593002/diff/60001/content/renderer/schedul...
> File content/renderer/scheduler/renderer_scheduler_impl.cc (left):
> 
>
https://codereview.chromium.org/904593002/diff/60001/content/renderer/schedul...
> content/renderer/scheduler/renderer_scheduler_impl.cc:73:
> main_thread_checker_.CalledOnValidThread();
> I'm not sure we can remove these checks - this is the comment from sievers@ on
> the original review which caused me to add these in the first place:
> 
>   You can't access |this| from another thread because |this| might 
>   get deleted while you are calling into it which will crash.
>   Everybody needs to acquire a reference to the task runners from
>   the main thread and then hold on to it.

The DidReceiveInputEventOnCompositorThread being called from a different thread
is allowed because we verified that the RenderWidgetCompositor is always deleted
before the RendererScheduler is, however it was much more difficult to verify
that this would be the case for the TaskRunner getters.

[jdduke (slow), 2015-02-05 19:07:10]

On 2015/02/05 18:53:06, rmcilroy wrote:
> On 2015/02/05 18:50:00, rmcilroy wrote:
> >
>
https://codereview.chromium.org/904593002/diff/20001/content/renderer/schedul...
> > File content/renderer/scheduler/renderer_scheduler_impl.cc (left):
> > 
> >
>
https://codereview.chromium.org/904593002/diff/20001/content/renderer/schedul...
> > content/renderer/scheduler/renderer_scheduler_impl.cc:296:
> > thread_checker_.CalledOnValidThread();
> > On 2015/02/05 18:19:18, jdduke wrote:
> > > On 2015/02/05 18:16:54, Sami wrote:
> > > > Ross, could you double check this is what we want to do?
> > > 
> > > This seemed unnecessary, as the class is internal only and |IsSet()| is
> > already
> > > guarded by a ThreadChecker.
> > 
> > Yes I think you are right, this is probably unnecessary.
> > 
> >
>
https://codereview.chromium.org/904593002/diff/60001/content/renderer/schedul...
> > File content/renderer/scheduler/renderer_scheduler_impl.cc (left):
> > 
> >
>
https://codereview.chromium.org/904593002/diff/60001/content/renderer/schedul...
> > content/renderer/scheduler/renderer_scheduler_impl.cc:73:
> > main_thread_checker_.CalledOnValidThread();
> > I'm not sure we can remove these checks - this is the comment from sievers@
on
> > the original review which caused me to add these in the first place:
> > 
> >   You can't access |this| from another thread because |this| might 
> >   get deleted while you are calling into it which will crash.
> >   Everybody needs to acquire a reference to the task runners from
> >   the main thread and then hold on to it.
> 
> The DidReceiveInputEventOnCompositorThread being called from a different
thread
> is allowed because we verified that the RenderWidgetCompositor is always
deleted
> before the RendererScheduler is, however it was much more difficult to verify
> that this would be the case for the TaskRunner getters.

I see, hopefully this latest version addresses all our concerns here,

[rmcilroy, 2015-02-05 19:28:10]

On 2015/02/05 19:07:10, jdduke wrote:
> On 2015/02/05 18:53:06, rmcilroy wrote:
> > On 2015/02/05 18:50:00, rmcilroy wrote:
> > >
> >
>
https://codereview.chromium.org/904593002/diff/20001/content/renderer/schedul...
> > > File content/renderer/scheduler/renderer_scheduler_impl.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/904593002/diff/20001/content/renderer/schedul...
> > > content/renderer/scheduler/renderer_scheduler_impl.cc:296:
> > > thread_checker_.CalledOnValidThread();
> > > On 2015/02/05 18:19:18, jdduke wrote:
> > > > On 2015/02/05 18:16:54, Sami wrote:
> > > > > Ross, could you double check this is what we want to do?
> > > > 
> > > > This seemed unnecessary, as the class is internal only and |IsSet()| is
> > > already
> > > > guarded by a ThreadChecker.
> > > 
> > > Yes I think you are right, this is probably unnecessary.
> > > 
> > >
> >
>
https://codereview.chromium.org/904593002/diff/60001/content/renderer/schedul...
> > > File content/renderer/scheduler/renderer_scheduler_impl.cc (left):
> > > 
> > >
> >
>
https://codereview.chromium.org/904593002/diff/60001/content/renderer/schedul...
> > > content/renderer/scheduler/renderer_scheduler_impl.cc:73:
> > > main_thread_checker_.CalledOnValidThread();
> > > I'm not sure we can remove these checks - this is the comment from
sievers@
> on
> > > the original review which caused me to add these in the first place:
> > > 
> > >   You can't access |this| from another thread because |this| might 
> > >   get deleted while you are calling into it which will crash.
> > >   Everybody needs to acquire a reference to the task runners from
> > >   the main thread and then hold on to it.
> > 
> > The DidReceiveInputEventOnCompositorThread being called from a different
> thread
> > is allowed because we verified that the RenderWidgetCompositor is always
> deleted
> > before the RendererScheduler is, however it was much more difficult to
verify
> > that this would be the case for the TaskRunner getters.
> 
> I see, hopefully this latest version addresses all our concerns here,

Lgtm, thanks!

[jam, 2015-02-05 21:20:49]

base lgtm

[jdduke (slow), 2015-02-05 21:39:17]

New patchsets have been uploaded after l-g-t-m from
rmcilroy@chromium.org,jam@chromium.org

[jdduke (slow), 2015-02-05 23:32:39]

The CQ bit was checked by jdduke@chromium.org

[commit-bot: I haz the power, 2015-02-05 23:33:47]

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

[commit-bot: I haz the power, 2015-02-06 00:50:40]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-02-06 00:52:00]

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