CompositorWorkerProxyClientImpl to CompositorProxy cross thread reference is using WeakMember

CompositorWorkerProxyClientImpl to CompositorProxy cross thread reference is using WeakMember

Because of a bug in Oilpan causing DCHECK to be skipped, we failed to detect that CompositorWorkerProxyClientImpl to CompositorProxy reference isn't using CrossThreadWeakPersistent.

The DCHECK bug will be fixed in https://codereview.chromium.org/2773013002/ .

This CL splits out the CompositorProxy registration to a new class so we don't need to put CrossThreadWeakPersistents in a HashMap.

BUG=705872
Review-Url: https://codereview.chromium.org/2774223002
Cr-Commit-Position: refs/heads/master@{#461972}
Committed: https://chromium.googlesource.com/chromium/src/+/0af77d69187ee8a529072cb3995bd111aaed5864

[keishi, 2017-03-28 19:34:40]

Description was changed from

==========
Fix CompositorWorker

BUG=
==========

to

==========
CompositorWorkerProxyClientImpl to CompositorProxy cross thread reference is
using WeakMember

Because of a bug in Oilpan causing DCHECK to be skipped, we failed to detect
that CompositorWorkerProxyClientImpl to CompositorProxy reference isn't using
CrossThreadWeakPersistent.

The DCHECK bug will be fixed in https://codereview.chromium.org/2773013002/ .

This CL splits out the CompositorProxy registration to a new class so we don't
need to put CrossThreadWeakPersistents in a HashMap.

BUG=705872
==========

[keishi, 2017-03-28 19:34:40]

keishi@chromium.org changed reviewers:
+ haraken@chromium.org, majidvp@chromium.org

[keishi, 2017-03-28 19:35:13]

keishi@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org

[keishi, 2017-03-28 19:35:13]

PTAL. AnimationWorkletProxyClientImpl::m_proxies isn't being used yet but so I
wasn't sure but I assumed that it works the same way as
CompositorWorkerProxyClientImpl.

[keishi, 2017-03-29 05:29:16]

FYI

CompositorProxy can be constructed in the main thread and compositor worker
thread. However CompositorWorkerProxyClientImpl::m_proxies only ever contains
ones constructed in the compositor worker thread.

CompositorWorkerProxyClientImpl inherits Supplement<WorkerClients> which means
it is constructed on the main thread and passed to the compositor worker thread.

CompositorMutatorImpl can be constructed in the main thread and compositor
thread.

[keishi, 2017-03-29 05:44:38]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 05:45:08]

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

[sof, 2017-03-29 05:51:04]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2017-03-29 05:51:05]

https://codereview.chromium.org/2774223002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/AnimationWorkletProxyClient.h (right):

https://codereview.chromium.org/2774223002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/AnimationWorkletProxyClient.h:14: class
CORE_EXPORT AnimationWorkletProxyClient
What role does this abstraction play? i.e., it's now a synonym for GCFinalized.

[commit-bot: I haz the power, 2017-03-29 06:19:09]

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

[commit-bot: I haz the power, 2017-03-29 06:19:09]

Dry run: Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[keishi, 2017-03-29 06:30:47]

The CQ bit was checked by keishi@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 06:31:05]

Dry run: 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-03-29 07:08:24]

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

[commit-bot: I haz the power, 2017-03-29 07:08:25]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[haraken, 2017-03-29 07:45:07]

Looks good % Sigbjorn's comment addressed.

[sof, 2017-03-29 10:24:08]

#5 is a great help in gaining quicker understanding of thread ownership & usage
(wrt current), so the more comments of that nature that can be added to the
classes that are touched&added here, the better.  With so many, it's hard to
navigate the assumptions.

https://codereview.chromium.org/2774223002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorProxyClientImpl.h (right):

https://codereview.chromium.org/2774223002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientImpl.h:12: class
CompositorProxyClientImpl
Could you add some documentation describing where this implementation object
resides, and overall purpose?

[keishi, 2017-03-30 08:34:14]

Moved all information in comment #5 into code comments.

https://codereview.chromium.org/2774223002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/AnimationWorkletProxyClient.h (right):

https://codereview.chromium.org/2774223002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/AnimationWorkletProxyClient.h:14: class
CORE_EXPORT AnimationWorkletProxyClient
On 2017/03/29 05:51:05, sof wrote:
> What role does this abstraction play? i.e., it's now a synonym for
GCFinalized.

I think it's suppose to be an interface so it can be accessed from core/ or
modules/. AnimationWorkletProxyClientImpl is in web/ and
AnimationWorkletProxyClient is used in modules/. I don't think I can delete this
class without upsetting DEPS.

Changed to GarbageCollectedMixin.

https://codereview.chromium.org/2774223002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorProxyClientImpl.h (right):

https://codereview.chromium.org/2774223002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientImpl.h:12: class
CompositorProxyClientImpl
On 2017/03/29 10:24:08, sof wrote:
> Could you add some documentation describing where this implementation object
> resides, and overall purpose?

Done.

[keishi, 2017-03-30 08:36:13]

majidvp could you take a look? Thanks.

[sof, 2017-03-30 13:57:38]

lgtm

[majidvp, 2017-03-30 16:20:07]

majidvp@chromium.org changed reviewers:
+ flackr@chromium.org
- majidvp@chromium.org

[majidvp, 2017-03-30 16:20:37]

On 2017/03/30 13:57:38, sof wrote:
> lgtm

I am OOO but flackr@ can review instead of me.

[keishi, 2017-04-03 00:34:06]

flackr, could you take a look? Thanks!

[flackr, 2017-04-04 02:17:08]

LGTM with nits

https://codereview.chromium.org/2774223002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorMutatorImpl.h (right):

https://codereview.chromium.org/2774223002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorMutatorImpl.h:26: // Owned by the main
thread or control thread.
Unless I'm missing something, this should only be owned by the main thread if we
don't have a compositor thread, see:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/Compositor...

https://codereview.chromium.org/2774223002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorProxyClientImpl.h (right):

https://codereview.chromium.org/2774223002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientImpl.h:12: // Registery for
CompositorProxies on the control thread.
nit: s/Registery/Registry

[keishi, 2017-04-04 05:18:32]

https://codereview.chromium.org/2774223002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorMutatorImpl.h (right):

https://codereview.chromium.org/2774223002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorMutatorImpl.h:26: // Owned by the main
thread or control thread.
On 2017/04/04 02:17:07, flackr wrote:
> Unless I'm missing something, this should only be owned by the main thread if
we
> don't have a compositor thread, see:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/Compositor...

I'm unclear about when the compositor thread will be missing.

Should the comment here be "Owned by the control thread" because the only time
it is owned by the main thread is when we run with the
"--disable-compositor-thread" flag?

The only test where CompositorMutatorImpl is allocated on the main thread seems
to be "global-interface-listing-compositor-worker.html". Why doesn't it have a
compositor thread?

[keishi, 2017-04-04 05:25:54]

On 2017/04/04 05:18:32, keishi wrote:
>
https://codereview.chromium.org/2774223002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/web/CompositorMutatorImpl.h (right):
> 
>
https://codereview.chromium.org/2774223002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/web/CompositorMutatorImpl.h:26: // Owned by the main
> thread or control thread.
> On 2017/04/04 02:17:07, flackr wrote:
> > Unless I'm missing something, this should only be owned by the main thread
if
> we
> > don't have a compositor thread, see:
> >
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/Compositor...
> 
> I'm unclear about when the compositor thread will be missing.
> 
> Should the comment here be "Owned by the control thread" because the only time
> it is owned by the main thread is when we run with the
> "--disable-compositor-thread" flag?
> 
> The only test where CompositorMutatorImpl is allocated on the main thread
seems
> to be "global-interface-listing-compositor-worker.html". Why doesn't it have a
> compositor thread?

s/--disable-compositor-thread/--disable-threaded-compositing/

[keishi, 2017-04-04 05:50:22]

Looks like threaded compositor is disabled by default for content_shell and
tests that use it need to enable it manually using VirtualTestSuites.

For normal Chrome, threaded compositing is enabled, so I'll write that
CompositorMutatorImpl is "owned by the compositor thread (unless threaded
compositing is disabled)".

[keishi, 2017-04-04 05:54:43]

https://codereview.chromium.org/2774223002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorMutatorImpl.h (right):

https://codereview.chromium.org/2774223002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorMutatorImpl.h:26: // Owned by the main
thread or control thread.
On 2017/04/04 02:17:07, flackr wrote:
> Unless I'm missing something, this should only be owned by the main thread if
we
> don't have a compositor thread, see:
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/Compositor...

Done.

https://codereview.chromium.org/2774223002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorProxyClientImpl.h (right):

https://codereview.chromium.org/2774223002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientImpl.h:12: // Registery for
CompositorProxies on the control thread.
On 2017/04/04 02:17:07, flackr wrote:
> nit: s/Registery/Registry

Done.

[keishi, 2017-04-05 01:46:03]

The CQ bit was checked by keishi@chromium.org

[keishi, 2017-04-05 01:46:04]

The patchset sent to the CQ was uploaded after l-g-t-m from flackr@chromium.org,
sigbjornf@opera.com
Link to the patchset: https://codereview.chromium.org/2774223002/#ps160001
(title: "fix")

[commit-bot: I haz the power, 2017-04-05 01:46:51]

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-04-05 03:42:47]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1491356763781320,
"parent_rev": "d9427ef54d34482a544c97f6f5393c92c3ed7765", "commit_rev":
"0af77d69187ee8a529072cb3995bd111aaed5864"}

[commit-bot: I haz the power, 2017-04-05 03:44:02]

Description was changed from

==========
CompositorWorkerProxyClientImpl to CompositorProxy cross thread reference is
using WeakMember

Because of a bug in Oilpan causing DCHECK to be skipped, we failed to detect
that CompositorWorkerProxyClientImpl to CompositorProxy reference isn't using
CrossThreadWeakPersistent.

The DCHECK bug will be fixed in https://codereview.chromium.org/2773013002/ .

This CL splits out the CompositorProxy registration to a new class so we don't
need to put CrossThreadWeakPersistents in a HashMap.

BUG=705872
==========

to

==========
CompositorWorkerProxyClientImpl to CompositorProxy cross thread reference is
using WeakMember

Because of a bug in Oilpan causing DCHECK to be skipped, we failed to detect
that CompositorWorkerProxyClientImpl to CompositorProxy reference isn't using
CrossThreadWeakPersistent.

The DCHECK bug will be fixed in https://codereview.chromium.org/2773013002/ .

This CL splits out the CompositorProxy registration to a new class so we don't
need to put CrossThreadWeakPersistents in a HashMap.

BUG=705872

Review-Url: https://codereview.chromium.org/2774223002
Cr-Commit-Position: refs/heads/master@{#461972}
Committed:
https://chromium.googlesource.com/chromium/src/+/0af77d69187ee8a529072cb3995b...
==========

[commit-bot: I haz the power, 2017-04-05 03:44:04]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/0af77d69187ee8a529072cb3995b...