Oilpan: add transition types to track interface objects.

Source/core/html/track/TextTrackCue.h

Index: Source/core/html/track/TextTrackCue.h
diff --git a/Source/core/html/track/TextTrackCue.h b/Source/core/html/track/TextTrackCue.h
index 4faf72d6ce90ffd91e6b0bb4d1c34b09245c6d71..2f9524b6eaf666b8306ded85e8f7b3b59dff4d3a 100644
--- a/Source/core/html/track/TextTrackCue.h
+++ b/Source/core/html/track/TextTrackCue.h
@@ -34,14 +34,15 @@
 
 #include "core/events/EventTarget.h"
 #include "core/html/HTMLDivElement.h"
+#include "platform/heap/Handle.h"
 #include "wtf/RefCounted.h"
 
 namespace WebCore {
 
 class ExceptionState;
 
-class TextTrackCue : public RefCounted<TextTrackCue>, public EventTargetWithInlineData {
-    REFCOUNTED_EVENT_TARGET(TextTrackCue);
+class TextTrackCue : public RefCountedWillBeRefCountedGarbageCollected<TextTrackCue>, public EventTargetWithInlineData {
+    DEFINE_EVENT_TARGET_REFCOUNTING(RefCountedWillBeRefCountedGarbageCollected<TextTrackCue>);
 public:
     static bool isInfiniteOrNonNumber(double value, ExceptionState&);
 
@@ -96,6 +97,8 @@ public:
     DEFINE_ATTRIBUTE_EVENT_LISTENER(enter);
     DEFINE_ATTRIBUTE_EVENT_LISTENER(exit);
 
+    virtual void trace(Visitor*);
+
 protected:
     TextTrackCue(double start, double end);
 
@@ -108,7 +111,7 @@ private:
     double m_endTime;
     int m_cueIndex;
 
-    TextTrack* m_track;
+    RawPtrWillBeMember<TextTrack> m_track;

[haraken, 2014/04/23 01:51:45]

Shouldn't this be a WeakMember? The TextTrackCue object can outlive the
TextTrack object if a JS holds a wrapper to the TextTrack object.

[sof, 2014/04/23 05:38:47]

If you make it weak, the cue can loose reference to its track. That will break
event handler lookups (which is with respect to the media element, right?)

[haraken, 2014/04/23 05:53:54]

Yes, but isn't that expected? As far as I read the current implementation,
TextTrackCue::m_track is cleared out when the TextTrack object dies. (And the
TextTrackCue object can outlive the TextTrack object as Mads pointed out.)

[Mads Ager (chromium), 2014/04/23 05:59:38]

This should be a strong pointer. The pattern that is used all over the place
here is that these objects are associated with eachother. There are therefore
pointers in both directions. Since they cannot both be RefPtrs one of them has
been made into a raw pointer that is cleared in the other objects destructor.
However, that is just in order to break a cycle. The real object relationship is
that they belong together and should live and die together.

This should be a strong pointer.

In general, prefer strong pointers over weak pointers when it makes sense.

[haraken, 2014/04/23 06:21:26]

Understand, that makes sense.

So this CL is going to change the existing lifetime relationship. In non-oilpan
builds, we're treating m_track as a weak pointer (and accepting wrong behavior
because we have to break a cycle). In oilpan builds, m_track becomes a strong
pointer (since that's the behavior we expect).

sof: It would be great if you could elaborate on this kind of non-trivial
changes in the CL description :)

[sof, 2014/04/23 07:36:48]

Certainly; have expanded the description to (try to) cover this part.

[Erik Corry, 2014/04/23 08:15:21]

It's not clear that any "wrong behaviour" ever happens.  It could be that the
pointer clearing in the destructor only ever happens when both objects are
garbage. In that case it would not be necessary to clear the pointer in the
destructor, but it's really hard to reason about this in the pre-oilpan world
and if you get it wrong you have a security bug, which you may perhaps never
find, but the black hats may perhaps find and exploit.

In the Oilpan world we make both pointers strong, and it is safe and there is no
use-after-free. If this leads to memory leaks we can be forced to use
WeakMember, but we don't think it does in this case.  In the oilpan world if we
get it wrong then we have a memory leak, which can be detected, but not a
security bug which is much more serious and not as easy to detect, since it only
happens when the attacker wants it and not when you have asserts enabled or are
running in our test infrastructure.

So if the clearing-pointers-in-the-destructor is happening to actually allow
weak behaviour (one object lives on with a nulled pointer, after the other has
been collected) then we may have to use weak behaviour, but if the
clearing-pointers-in-the-destructor is only there for paranoia reasons to avoid
a use-after-free that we can't _prove_ can never happen, then we just use strong
pointers in oilpan.

One clue is to look for code that checks for null pointers in the code.  For
instance in this case the track() method returns the m_track field that is
nulled from the destructor.  But there is code that says track()->cues(), so
clearly we are not actually expecting the pointer to be nulled in an object that
is not dead.

 
     bool m_isActive : 1;
     bool m_pauseOnExit : 1;