Oilpan: Completely move core/animations/ to oilpan's heap

Source/core/animation/TimedItem.h

Index: Source/core/animation/TimedItem.h
diff --git a/Source/core/animation/TimedItem.h b/Source/core/animation/TimedItem.h
index 69e82a01c3c2a8ce1f4e73caa5a4da8bc269061e..773369b680320cbf06fd50841619b7bdc71caaa6 100644
--- a/Source/core/animation/TimedItem.h
+++ b/Source/core/animation/TimedItem.h
@@ -32,6 +32,7 @@
 #define TimedItem_h
 
 #include "core/animation/Timing.h"
+#include "platform/heap/Handle.h"
 #include "wtf/OwnPtr.h"
 #include "wtf/PassOwnPtr.h"
 #include "wtf/RefCounted.h"
@@ -52,7 +53,7 @@ static inline double nullValue()
     return std::numeric_limits<double>::quiet_NaN();
 }
 
-class TimedItem : public RefCounted<TimedItem> {
+class TimedItem : public RefCountedWillBeGarbageCollectedFinalized<TimedItem> {
     friend class AnimationPlayer; // Calls attach/detach, updateInheritedTime.
 public:
     // Note that logic in CSSAnimations depends on the order of these values.
@@ -98,15 +99,18 @@ public:
     AnimationPlayer* player() { return m_player; }
     AnimationPlayer* player(bool& isNull) { isNull = !m_player; return m_player; }
     const Timing& specifiedTiming() const { return m_timing; }
-    PassRefPtr<TimedItemTiming> timing();
+    PassRefPtrWillBeRawPtr<TimedItemTiming> timing();
     void updateSpecifiedTiming(const Timing&);
 
     // This method returns time in ms as it is unused except via the API.
     double localTime(bool& isNull) const { isNull = !m_player; return ensureCalculated().localTime * 1000; }
     double currentIteration(bool& isNull) const { isNull = !ensureCalculated().isInEffect; return ensureCalculated().currentIteration; }
 
+    virtual void trace(Visitor*);
+    void processWeakMembers(Visitor*);
+
 protected:
-    TimedItem(const Timing&, PassOwnPtr<EventDelegate> = nullptr);
+    explicit TimedItem(const Timing&, PassOwnPtr<EventDelegate> = nullptr);
 
     // When TimedItem receives a new inherited time via updateInheritedTime
     // it will (if necessary) recalculate timings and (if necessary) call
@@ -116,34 +120,28 @@ protected:
     bool hasEvents() const { return m_eventDelegate; }
     void clearEventDelegate() { m_eventDelegate = nullptr; }
 
-private:
+    virtual void attach(AnimationPlayer* player)
+    {
+        m_player = player;
+    }
+
+    virtual void detach()
+    {
+        ASSERT(m_player);
+        m_player = nullptr;
+    }
 
     double repeatedDuration() const;
 
     virtual void updateChildrenAndEffects() const = 0;
     virtual double intrinsicIterationDuration() const { return 0; };
     virtual double calculateTimeToEffectChange(bool forwards, double localTime, double timeToNextIteration) const = 0;
-    virtual void didAttach() { };
-    virtual void willDetach() { };
-    virtual void specifiedTimingChanged() { };
-
-    void attach(AnimationPlayer* player)
-    {
-        m_player = player;
-        didAttach();
-    };
-
-    void detach()
-    {
-        ASSERT(m_player);
-        willDetach();
-        m_player = 0;
-    };
+    virtual void specifiedTimingChanged() { }
 
     // FIXME: m_parent and m_startTime are placeholders, they depend on timing groups.
-    TimedItem* const m_parent;
+    RawPtrWillBeMember<TimedItem> m_parent;
     const double m_startTime;
-    AnimationPlayer* m_player;
+    RawPtrWillBeWeakMember<AnimationPlayer> m_player;

[haraken, 2014/04/29 04:47:35]

Actually I don't fully understand this pointer.

- In non-oilpan builds, this is implemented as a weak pointer. However, shanes@
and timothy@ are saying that this should be a strong pointer. Probably the
situation is that (1) in non-oilpan builds we are using a weak pointer and
accepting wrong behavior because it is hard to make it a strong pointer without
oilpan; (2) in oilpan builds we want to make it a strong pointer.

- However, I cannot find an easy way to make it a strong pointer in oilpan
builds as well. The issue is that we need to invoke TimedItem::detach() when the
AnimationPlayer object dies. Since TimedItem::detach() touches on-heap objects,
we cannot invoke it in ~AnimationPlayer(). Thus we need to invoke
TimedItem::detach() in the weak processing of the AnimationPlayer object. This
means that someone needs to retain a weak pointer to the AnimationPlayer, but
I'm not sure who should retain the weak pointer.

- Given the above, at the moment, I'm using a weak pointer in this CL. This
won't fix existing wrong behavior but won't change existing behavior.

Any advice is welcome.

[Mads Ager (chromium), 2014/04/29 10:29:41]

I don't understand the lifetime of these objects well enough. Maybe the authors
of core/animation can create a drawing of the lifetime connections that they
would like to have. Which objects live and die together. Which objects are
allowed to die independently of the other objects?

[dstockwell, 2014/05/01 14:06:54]

This is correct. We accept wrong behavior currently as the cases where the
correct lifetimes matter are not shipping yet.
 
I think executing this logic in detach() should not be necessary in oilpan.

Its purpose in the destructor should only be to clean up references to the
now-destructing object. But in oilpan, these objects should die together.

Example: V8 -> TimedItem <-> Player <- DocumentTimeline
- Eventually the animation will complete and DocumentTimeline will drop its
reference to Player.
- At this point, Player is no longer referenced (in broken, non-oilpan TimedItem
only has a weak reference).
- Player is destructed and calls detach on its source TimedItem to clean up
TimedItem's raw pointer.

However, the TimedItem and Player here should strongly reference each other and
die together.

An expanded example (I fail at drawing ascii diagrams, so consider the two
Animations and SampledEffects to be the same objects):
V8 -> Animation <-> Player <- DocumentTimeline
      Animation <-> SampledEffect -> Player
      AnimationStack -> SampledEffect
- Eventually the animation will complete and DocumentTimeline will drop its
reference to Player.
- However, in this example, the animation still has an effect and will continue
to have an effect unless cancelled. In the event that Animation/Player become
unreachable from V8, the effect remains and is no longer cancellable but we need
to simplify these data structures to avoid leaking. The result is that only the
SampledEffect should remain, the Animation and Player should die.
- Currently SampledEffect has a strong reference to Player and a weak reference
to the Animation. When AnimationStack determines that SampledEffect is holding
the last reference to Player and can be simplified, it releases SampledEffect,
allowing Player and Animation to die.
- In Oilpan, SampledEffect's reference to Player should not be needed at all,
instead SampledEffect should only hold a weak reference to Animation.
SampledEffect then only needs to know that Animation has died in order to
complete simplification.

Sorry! It's complicated! I hope this helps :/

@timloh should read this and point out all my mistakes.

     Timing m_timing;
     OwnPtr<EventDelegate> m_eventDelegate;