Prepare CompositorWorker for per thread heap

Prepare CompositorWorker for per thread heap

Replace cross thread references with CrossThreadPersistents.

BUG=591606
Committed: https://crrev.com/c59f742ca99b5695cab3c7d118d23d2c0a800d8c
Cr-Commit-Position: refs/heads/master@{#418175}

[keishi, 2016-08-03 12:07:30]

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

[commit-bot: I haz the power, 2016-08-03 12:07:41]

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, 2016-08-03 12:25:20]

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

[commit-bot: I haz the power, 2016-08-03 12:25:21]

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

[keishi, 2016-08-03 12:58:49]

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

[commit-bot: I haz the power, 2016-08-03 12:59: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, 2016-08-03 13:56:02]

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

[commit-bot: I haz the power, 2016-08-03 13:56:03]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[keishi, 2016-08-18 05:28:20]

Description was changed from

==========
Enable per thread heap for CompositorWorker

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

to

==========
Enable per thread heap for CompositorWorker

BUG=591606
==========

[keishi, 2016-08-19 06:32:14]

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

[commit-bot: I haz the power, 2016-08-19 06:32:35]

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, 2016-08-19 06:34:38]

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

[commit-bot: I haz the power, 2016-08-19 06:34:39]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[keishi, 2016-08-19 12:03:47]

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

[commit-bot: I haz the power, 2016-08-19 12:04:15]

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

[keishi, 2016-08-22 07:07:27]

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

[commit-bot: I haz the power, 2016-08-22 07:07:42]

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

[keishi, 2016-08-22 07:11:16]

The CQ bit was unchecked by keishi@chromium.org

[keishi, 2016-08-22 07:12:44]

The CQ bit was checked by keishi@chromium.org

[keishi, 2016-08-22 07:12:44]

The CQ bit was unchecked by keishi@chromium.org

[keishi, 2016-08-22 07:12:50]

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

[commit-bot: I haz the power, 2016-08-22 07:13:04]

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, 2016-08-22 09:43:28]

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

[commit-bot: I haz the power, 2016-08-22 09:43:29]

Dry run: This issue passed the CQ dry run.

[keishi, 2016-08-22 09:47:36]

PTAL

https://codereview.chromium.org/2204183002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-disconnect-worker-terminate.html
(left):

https://codereview.chromium.org/2204183002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/virtual/threaded/fast/compositorworker/compositor-proxy-disconnect-worker-terminate.html:20:
waitUntilWorkerThreadsExit(function() {
CompositorWorker termination != compositor thread termination. We don't have a
way to force a gc on compositor thread right now.

[keishi, 2016-08-22 09:48:07]

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

[keishi, 2016-08-22 09:48:07]

PTAL

[haraken, 2016-08-22 14:24:44]

https://codereview.chromium.org/2204183002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h
(right):

https://codereview.chromium.org/2204183002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h:111:
CrossThreadPersistent<WorkerGlobalScope> m_workerGlobalScope;

I want to make sure that these CrossThreadPersistents won't leak
WorkerGlobalScopes. Would you check in with hajimehoshi@ that WorkerGlobalScope
is supported in the leak detector?

https://codereview.chromium.org/2204183002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp (right):

https://codereview.chromium.org/2204183002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/WorkerBackingThread.cpp:58: :
m_backingThread(WebThreadSupportingGC::createForThread(thread, true))

I'd prefer making this change in a separate CL so that we can revert the
per-thread heap if necessary.

BTW, maybe do we want to send a PSA to blink-dev@?

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

https://codereview.chromium.org/2204183002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorMutatorImpl.cpp:80: for (auto& it :
m_proxyClients) {

Would it make more sense to provide a helper class like this?

  CrossThreadPersistentScope scope(m_proxyClients);  // This strongifies handles
in the map.
  ...;

https://codereview.chromium.org/2204183002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorMutatorImpl.cpp:81:
CrossThreadPersistent<CompositorProxyClientImpl> protect(it);

I'm just curious but how did you find that you need a protecting pointer here?
If you miss the protecting pointer, will you hit the DCHECK in the runtime
verification?

[haraken, 2016-09-01 19:01:58]

What's the status of this CL? Is there anything blocking you from enabling the
per-thread heap on the compositor worker?

[keishi, 2016-09-02 06:41:55]

Details are in the comments below but, in order to do leak detection correctly,
collectAllGarbage needs to run GC for all ThreadHeaps.

Right now I was able to get it working locally with something like
https://codereview.chromium.org/2304073002/ . Do you have any idea how to do
this more cleanly?

https://codereview.chromium.org/2204183002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h
(right):

https://codereview.chromium.org/2204183002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h:111:
CrossThreadPersistent<WorkerGlobalScope> m_workerGlobalScope;
On 2016/08/22 14:24:43, haraken wrote:
> 
> I want to make sure that these CrossThreadPersistents won't leak
> WorkerGlobalScopes. Would you check in with hajimehoshi@ that
WorkerGlobalScope
> is supported in the leak detector?

- Added the WorkerGlobalScope leak detector. r414978
- CompositorWorker doesn't own its thread so termination GC does not run and
collectAllGarbage only runs a GC in the main ThreadHeap. So leak detector
reports leaks. We need to run GC on all ThreadHeaps. CL at
https://codereview.chromium.org/2304073002/
- StackFrameDepth was static causing assertion failures when gcs run
simultaneously. CL at https://codereview.chromium.org/2304023003/
- CompositorProxyClientImpl::m_globalScope was causing a leak because
CompositorProxyClientImpl supplements WorkerClients which is referenced from
WorkerGlobalScope. Updated this CL to fix the reference cycle.

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

https://codereview.chromium.org/2204183002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorMutatorImpl.cpp:80: for (auto& it :
m_proxyClients) {
On 2016/08/22 14:24:44, haraken wrote:
> 
> Would it make more sense to provide a helper class like this?
> 
>   CrossThreadPersistentScope scope(m_proxyClients);  // This strongifies
handles
> in the map.
>   ...;

Strongifying all items at once is hard, and this pattern is only used here for
now so I would like to leave this as is.

https://codereview.chromium.org/2204183002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorMutatorImpl.cpp:81:
CrossThreadPersistent<CompositorProxyClientImpl> protect(it);
On 2016/08/22 14:24:44, haraken wrote:
> 
> I'm just curious but how did you find that you need a protecting pointer here?
> If you miss the protecting pointer, will you hit the DCHECK in the runtime
> verification?

I think the only safe way to use CrossThreadWeakPersistent (in a non-owner
thread), is by protecting it with a CrossThreadPersistent first. If you don't
protect it the object might disappear while using it. Also I think we need to
use the special CrossThreadPersistent::protectWeak() which locks the
crossThreadPersistentRegion before assigning, so the object isn't destroyed
while assigning. (I was trying to use CrossThreadPersistent constructor to
trigger the locking but I couldn't trigger the right constructor because
CrossThreadWeakPersistent could be implicitly cast to a raw pointer)

I added DCHECK to CrossThreadWeakPersistent accessor methods to avoid this
mistake.

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

https://codereview.chromium.org/2204183002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientImpl.h:46:
CrossThreadWeakPersistent<CompositorWorkerGlobalScope> m_globalScope;
CompositorProxyClientImpl::m_globalScope was causing a leak because
CompositorProxyClientImpl supplements WorkerClients which is referenced from
WorkerGlobalScope. WorkerGlobalScope should outlive this object so I think its
safe to make weak.

[haraken, 2016-09-02 09:39:18]

https://codereview.chromium.org/2204183002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h
(right):

https://codereview.chromium.org/2204183002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/workers/InProcessWorkerObjectProxy.h:111:
CrossThreadPersistent<WorkerGlobalScope> m_workerGlobalScope;
On 2016/09/02 06:41:55, keishi wrote:
> On 2016/08/22 14:24:43, haraken wrote:
> > 
> > I want to make sure that these CrossThreadPersistents won't leak
> > WorkerGlobalScopes. Would you check in with hajimehoshi@ that
> WorkerGlobalScope
> > is supported in the leak detector?
> 
> - Added the WorkerGlobalScope leak detector. r414978
> - CompositorWorker doesn't own its thread so termination GC does not run and
> collectAllGarbage only runs a GC in the main ThreadHeap. So leak detector
> reports leaks. We need to run GC on all ThreadHeaps. CL at
> https://codereview.chromium.org/2304073002/
> - StackFrameDepth was static causing assertion failures when gcs run
> simultaneously. CL at https://codereview.chromium.org/2304023003/
> - CompositorProxyClientImpl::m_globalScope was causing a leak because
> CompositorProxyClientImpl supplements WorkerClients which is referenced from
> WorkerGlobalScope. Updated this CL to fix the reference cycle.

Nice detective work! Posted some ideas on
https://codereview.chromium.org/2304073002/.

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

https://codereview.chromium.org/2204183002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorMutatorImpl.cpp:81:
CrossThreadPersistent<CompositorProxyClientImpl> protect(it);
On 2016/09/02 06:41:55, keishi wrote:
> On 2016/08/22 14:24:44, haraken wrote:
> > 
> > I'm just curious but how did you find that you need a protecting pointer
here?
> > If you miss the protecting pointer, will you hit the DCHECK in the runtime
> > verification?
> 
> I think the only safe way to use CrossThreadWeakPersistent (in a non-owner
> thread), is by protecting it with a CrossThreadPersistent first. If you don't
> protect it the object might disappear while using it. Also I think we need to
> use the special CrossThreadPersistent::protectWeak() which locks the
> crossThreadPersistentRegion before assigning, so the object isn't destroyed
> while assigning. (I was trying to use CrossThreadPersistent constructor to
> trigger the locking but I couldn't trigger the right constructor because
> CrossThreadWeakPersistent could be implicitly cast to a raw pointer)
> 
> I added DCHECK to CrossThreadWeakPersistent accessor methods to avoid this
> mistake.

Ah, the DCHECK looks clever!

However, maybe would it be better to make CrossThreadWeakPersistent::operator*
unconditionally return CrossThreadPersistent? I think it's *always* unsafe to
return a raw pointer from CrossThreadWeakPersistent::operator* because nothing
protects the raw pointer on the stack.

https://codereview.chromium.org/2204183002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Persistent.h (right):

https://codereview.chromium.org/2204183002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Persistent.h:534: DCHECK(!Parent::get()
|| !ThreadState::current() || !ThreadState::current()->perThreadHeapEnabled() ||
&ThreadState::fromObject(Parent::get())->heap() ==
&ThreadState::current()->heap());

Should we drop the '|| !ThreadState::current()->perThreadHeapEnabled()' part?

It's unsafe to let a thread use a CrossThreadWeakPersistent even if the
per-thread heap is not enabled on *this* thread (i.e., it's unsafe if the
per-thread heap is enabled on a GCing thread).

[keishi, 2016-09-07 10:14:42]

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

https://codereview.chromium.org/2204183002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorMutatorImpl.cpp:81:
CrossThreadPersistent<CompositorProxyClientImpl> protect(it);
On 2016/09/02 09:39:17, haraken wrote:
> On 2016/09/02 06:41:55, keishi wrote:
> > On 2016/08/22 14:24:44, haraken wrote:
> > > 
> > > I'm just curious but how did you find that you need a protecting pointer
> here?
> > > If you miss the protecting pointer, will you hit the DCHECK in the runtime
> > > verification?
> > 
> > I think the only safe way to use CrossThreadWeakPersistent (in a non-owner
> > thread), is by protecting it with a CrossThreadPersistent first. If you
don't
> > protect it the object might disappear while using it. Also I think we need
to
> > use the special CrossThreadPersistent::protectWeak() which locks the
> > crossThreadPersistentRegion before assigning, so the object isn't destroyed
> > while assigning. (I was trying to use CrossThreadPersistent constructor to
> > trigger the locking but I couldn't trigger the right constructor because
> > CrossThreadWeakPersistent could be implicitly cast to a raw pointer)
> > 
> > I added DCHECK to CrossThreadWeakPersistent accessor methods to avoid this
> > mistake.
> 
> Ah, the DCHECK looks clever!
> 
> However, maybe would it be better to make CrossThreadWeakPersistent::operator*
> unconditionally return CrossThreadPersistent? I think it's *always* unsafe to
> return a raw pointer from CrossThreadWeakPersistent::operator* because nothing
> protects the raw pointer on the stack.
> 

I was worried about this so I would love to do this, 
but we can't change the return type for CrossThreadWeakPersistent::operator* and
base::Bind uses that method so I can't remove it.

https://codereview.chromium.org/2204183002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Persistent.h (right):

https://codereview.chromium.org/2204183002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Persistent.h:534: DCHECK(!Parent::get()
|| !ThreadState::current() || !ThreadState::current()->perThreadHeapEnabled() ||
&ThreadState::fromObject(Parent::get())->heap() ==
&ThreadState::current()->heap());
On 2016/09/02 09:39:18, haraken wrote:
> 
> Should we drop the '|| !ThreadState::current()->perThreadHeapEnabled()' part?
> 
> It's unsafe to let a thread use a CrossThreadWeakPersistent even if the
> per-thread heap is not enabled on *this* thread (i.e., it's unsafe if the
> per-thread heap is enabled on a GCing thread).

Done.

[haraken, 2016-09-07 15:10:31]

https://codereview.chromium.org/2204183002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Persistent.h (right):

https://codereview.chromium.org/2204183002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Persistent.h:534: DCHECK(!Parent::get()
|| !ThreadState::current() || &ThreadState::fromObject(Parent::get())->heap() ==
&ThreadState::current()->heap());

I don't think we should add the '|| !ThreadState::current()' part. If the
CrossThreadWeakPersistent is used on a thread not attached to Oilpan, that's
already dangerous.

https://codereview.chromium.org/2204183002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorMutatorImpl.cpp (right):

https://codereview.chromium.org/2204183002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorMutatorImpl.cpp:93:
m_proxyClients.append(client);

Hmm, this looks very complicated and I'm not sure if this code is really
thread-safe...

Maybe can we use HeapHashSet<UntracedMember<CompositorProxyClientImpl>> and
introduce CompositorMutatorImpl::unregisterProxyClient, which removes the
UntracedMember at some explicit point?

If that is not easy, let's chat offline.

(I begin to think that CrossThreadWeakPersistent is a dangerous thing in
general, and maybe it should be just forbidden...)

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

https://codereview.chromium.org/2204183002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientImpl.h:46:
CrossThreadWeakPersistent<CompositorWorkerGlobalScope> m_globalScope;

Maybe can we use an UntracedMember?

[keishi, 2016-09-12 09:00:48]

https://codereview.chromium.org/2204183002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Persistent.h (right):

https://codereview.chromium.org/2204183002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Persistent.h:534: DCHECK(!Parent::get()
|| !ThreadState::current() || &ThreadState::fromObject(Parent::get())->heap() ==
&ThreadState::current()->heap());
On 2016/09/07 15:10:30, haraken wrote:
> 
> I don't think we should add the '|| !ThreadState::current()' part. If the
> CrossThreadWeakPersistent is used on a thread not attached to Oilpan, that's
> already dangerous.
> 

Done.

https://codereview.chromium.org/2204183002/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorMutatorImpl.cpp (right):

https://codereview.chromium.org/2204183002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorMutatorImpl.cpp:93:
m_proxyClients.append(client);
On 2016/09/07 15:10:30, haraken wrote:
> 
> Hmm, this looks very complicated and I'm not sure if this code is really
> thread-safe...
> 
> Maybe can we use HeapHashSet<UntracedMember<CompositorProxyClientImpl>> and
> introduce CompositorMutatorImpl::unregisterProxyClient, which removes the
> UntracedMember at some explicit point?
> 
> If that is not easy, let's chat offline.
> 
> (I begin to think that CrossThreadWeakPersistent is a dangerous thing in
> general, and maybe it should be just forbidden...)

I used WorkerGlobalScope::dispose for explicit removal instead of relying on
weak. dispose is called when the worker is terminated so it should work.

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

https://codereview.chromium.org/2204183002/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientImpl.h:46:
CrossThreadWeakPersistent<CompositorWorkerGlobalScope> m_globalScope;
On 2016/09/07 15:10:30, haraken wrote:
> 
> Maybe can we use an UntracedMember?

Done.

[haraken, 2016-09-12 11:45:09]

Mostly LG

https://codereview.chromium.org/2204183002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Persistent.h (right):

https://codereview.chromium.org/2204183002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Persistent.h:477: static
CrossThreadPersistent<U> protectWeak(const PersistentBase<U,
WeakPersistentConfiguration, CrossThreadPersistentConfiguration>& other)

Can we drop the changes in this file?

https://codereview.chromium.org/2204183002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Persistent.h:532: T& operator*() const

As discussed offline, we should try to remove CrossThreadWeakPersistent instead
of making it more powerful.

https://codereview.chromium.org/2204183002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorMutatorImpl.cpp (right):

https://codereview.chromium.org/2204183002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorMutatorImpl.cpp:78:
m_proxyClients.add(client);

Add DCHECK(!m_proxyClients.contains(client)).

https://codereview.chromium.org/2204183002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorMutatorImpl.cpp:84:
m_proxyClients.remove(client);

Add DCHECK(m_proxyClients.contains(client)).

https://codereview.chromium.org/2204183002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp (right):

https://codereview.chromium.org/2204183002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp:61: void
CompositorProxyClientImpl::dispose()

Add a comment about the lifetime of relevant objects.

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

https://codereview.chromium.org/2204183002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientImpl.h:47:
UntracedMember<CompositorWorkerGlobalScope> m_globalScope;

Can this be a CrossThreadPersistent?

[keishi, 2016-09-13 03:11:41]

Description was changed from

==========
Enable per thread heap for CompositorWorker

BUG=591606
==========

to

==========
Prepare CompositorWorker for per thread heap

BUG=591606
==========

[keishi, 2016-09-13 03:12:28]

Description was changed from

==========
Prepare CompositorWorker for per thread heap

BUG=591606
==========

to

==========
Prepare CompositorWorker for per thread heap

Replace cross thread references with CrossThreadPersistents.

BUG=591606
==========

[keishi, 2016-09-13 03:12:43]

https://codereview.chromium.org/2204183002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/heap/Persistent.h (right):

https://codereview.chromium.org/2204183002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/heap/Persistent.h:532: T& operator*() const
On 2016/09/12 11:45:09, haraken wrote:
> 
> As discussed offline, we should try to remove CrossThreadWeakPersistent
instead
> of making it more powerful.
> 

Sorry. Done.

https://codereview.chromium.org/2204183002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorMutatorImpl.cpp (right):

https://codereview.chromium.org/2204183002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorMutatorImpl.cpp:78:
m_proxyClients.add(client);
On 2016/09/12 11:45:09, haraken wrote:
> 
> Add DCHECK(!m_proxyClients.contains(client)).

Done.

https://codereview.chromium.org/2204183002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorMutatorImpl.cpp:84:
m_proxyClients.remove(client);
On 2016/09/12 11:45:09, haraken wrote:
> 
> Add DCHECK(m_proxyClients.contains(client)).

Done.

https://codereview.chromium.org/2204183002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp (right):

https://codereview.chromium.org/2204183002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientImpl.cpp:61: void
CompositorProxyClientImpl::dispose()
On 2016/09/12 11:45:09, haraken wrote:
> 
> Add a comment about the lifetime of relevant objects.

Done.

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

https://codereview.chromium.org/2204183002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/CompositorProxyClientImpl.h:47:
UntracedMember<CompositorWorkerGlobalScope> m_globalScope;
On 2016/09/12 11:45:09, haraken wrote:
> 
> Can this be a CrossThreadPersistent?

Done.

[keishi, 2016-09-13 03:12:49]

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

[commit-bot: I haz the power, 2016-09-13 03:13:02]

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, 2016-09-13 04:38:34]

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

[commit-bot: I haz the power, 2016-09-13 04:38:37]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-09-13 05:09:48]

LGTM

[keishi, 2016-09-13 05:13:24]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2016-09-13 05:13:41]

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, 2016-09-13 05:18:04]

Description was changed from

==========
Prepare CompositorWorker for per thread heap

Replace cross thread references with CrossThreadPersistents.

BUG=591606
==========

to

==========
Prepare CompositorWorker for per thread heap

Replace cross thread references with CrossThreadPersistents.

BUG=591606
==========

[commit-bot: I haz the power, 2016-09-13 05:18:05]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2016-09-13 05:21:14]

Description was changed from

==========
Prepare CompositorWorker for per thread heap

Replace cross thread references with CrossThreadPersistents.

BUG=591606
==========

to

==========
Prepare CompositorWorker for per thread heap

Replace cross thread references with CrossThreadPersistents.

BUG=591606

Committed: https://crrev.com/c59f742ca99b5695cab3c7d118d23d2c0a800d8c
Cr-Commit-Position: refs/heads/master@{#418175}
==========

[commit-bot: I haz the power, 2016-09-13 05:21:16]

Patchset 20 (id:??) landed as
https://crrev.com/c59f742ca99b5695cab3c7d118d23d2c0a800d8c
Cr-Commit-Position: refs/heads/master@{#418175}