Move CompositorPendingAnimations to Oilpan's heap

Move CompositorPendingAnimations to Oilpan's heap

CompositorPendingAnimations::m_timer can be fired in Oilpan's lazy sweeping phase.
This is problematic because at that point things accessed in the timer function
might have been already destructed.

We don't hit this problem if CompositorPendingAnimations is a GarbageCollected object
because we have logic to suppress firing timers of unreachable GarbageCollected objects.
So this CL moves CompositorPendingAnimations to the heap.

BUG=592845
Committed: https://crrev.com/a8f340ef9d09b2692552bbc463572af84e3e1fbb
Cr-Commit-Position: refs/heads/master@{#383248}

[haraken, 2016-03-24 08:46:55]

haraken@chromium.org changed reviewers:
+ oilpan-reviews@chromium.org, sigbjornf@opera.com

[haraken, 2016-03-24 08:46:56]

PTAL

(It's not nice that a DISALLOW_NEW object causes this problem. It would be great
if we could improve timer's infrastructure or add some verification to avoid the
scenario...)

[sof, 2016-03-24 15:02:10]

lgtm

https://codereview.chromium.org/1832773002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/1832773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.h:1375:
PersistentWillBeMember<CompositorPendingAnimations>
m_compositorPendingAnimations;
Can we forward declare CompositorPendingAnimations rather than have Document.h
include it? Doesn't have to be done in this CL, but the shorter the include list
from Document, the better.

[sof, 2016-03-24 15:11:10]

On 2016/03/24 08:46:56, haraken wrote:
> PTAL
> 
> (It's not nice that a DISALLOW_NEW object causes this problem. It would be
great
> if we could improve timer's infrastructure or add some verification to avoid
the
> scenario...)

We can add some static_assert()s to EAGERLY_FINALIZE() and USING_PREFINALIZER()
to restrict their uses to classes that derive from a gc base. Another thing we
can perhaps do is to disallow timers for part objects if they have a trace
method, once oilpan is always on. But I suspect there might be details
preventing that from working out..

[sof, 2016-03-24 17:36:06]

https://codereview.chromium.org/1832773002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/Document.h (right):

https://codereview.chromium.org/1832773002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/Document.h:974: CompositorPendingAnimations&
compositorPendingAnimations() { return *m_compositorPendingAnimations; }
Wait, you need to create this heap object somewhere.

[haraken, 2016-03-25 02:32:13]

On 2016/03/24 17:36:06, sof wrote:
>
https://codereview.chromium.org/1832773002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/Document.h (right):
> 
>
https://codereview.chromium.org/1832773002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/Document.h:974:
CompositorPendingAnimations&
> compositorPendingAnimations() { return *m_compositorPendingAnimations; }
> Wait, you need to create this heap object somewhere.

Done.

[haraken, 2016-03-25 02:32:55]

>
https://codereview.chromium.org/1832773002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/dom/Document.h (right):
> 
>
https://codereview.chromium.org/1832773002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/dom/Document.h:1375:
> PersistentWillBeMember<CompositorPendingAnimations>
> m_compositorPendingAnimations;
> Can we forward declare CompositorPendingAnimations rather than have Document.h
> include it? Doesn't have to be done in this CL, but the shorter the include
list
> from Document, the better.

I noticed that this requires a lot of changes, so I'll defer it to later.

[haraken, 2016-03-25 02:34:01]

On 2016/03/24 15:11:10, sof wrote:
> On 2016/03/24 08:46:56, haraken wrote:
> > PTAL
> > 
> > (It's not nice that a DISALLOW_NEW object causes this problem. It would be
> great
> > if we could improve timer's infrastructure or add some verification to avoid
> the
> > scenario...)
> 
> We can add some static_assert()s to EAGERLY_FINALIZE() and
USING_PREFINALIZER()
> to restrict their uses to classes that derive from a gc base. Another thing we
> can perhaps do is to disallow timers for part objects if they have a trace
> method, once oilpan is always on. But I suspect there might be details
> preventing that from working out..

Yeah, it would be hard to correctly detect timers in part objects.
static_asserts in EAGERLY_FINALIZE() and USING_PREFINALIZER() sounds like a good
idea :)

[haraken, 2016-03-25 02:34:05]

The CQ bit was checked by haraken@chromium.org

[haraken, 2016-03-25 02:34:05]

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

[commit-bot: I haz the power, 2016-03-25 02:34:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1832773002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1832773002/20001

[commit-bot: I haz the power, 2016-03-25 04:52:36]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-25 04:54:11]

Description was changed from

==========
Move CompositorPendingAnimations to Oilpan's heap

CompositorPendingAnimations::m_timer can be fired in Oilpan's lazy sweeping
phase.
This is problematic because at that point things accessed in the timer function
might have been already destructed.

We don't hit this problem if CompositorPendingAnimations is a GarbageCollected
object
because we have logic to suppress firing timers of unreachable GarbageCollected
objects.
So this CL moves CompositorPendingAnimations to the heap.

BUG=592845
==========

to

==========
Move CompositorPendingAnimations to Oilpan's heap

CompositorPendingAnimations::m_timer can be fired in Oilpan's lazy sweeping
phase.
This is problematic because at that point things accessed in the timer function
might have been already destructed.

We don't hit this problem if CompositorPendingAnimations is a GarbageCollected
object
because we have logic to suppress firing timers of unreachable GarbageCollected
objects.
So this CL moves CompositorPendingAnimations to the heap.

BUG=592845

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

[commit-bot: I haz the power, 2016-03-25 04:54:11]

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