Oilpan: add transition types to track interface objects.

Oilpan: add transition types to track interface objects.

With Oilpan enabled, make TextTrack, TextTrackCue, VTTCue, TextTrackList,
TextTrackCueList, VTTRegion, VTTRegionList and VTTParser garbage
collected objects.

Along with doing so, the references between these objects are now (strong)
Member references, and not asymmetric RefPtr+raw pointer pairs needed
to break cycles. This means these inter-related objects will live
and die together.

Simpler to reason about, and prevents problematic cases where if a
"child" object (e.g., a text track cue object) is held onto longer
than its parent track object (or its track list), you'd be unable to
keep those "parent" objects alive. The best we can currently do without
Oilpan strong member references is to clear out the raw back pointers
upon destructing the parent. And we still do in the non-Oilpan case.

R=haraken@chromium.org,ager@chromium.org
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172365

[sof, 2014-04-20 18:35:36]

Please take a look.

[haraken, 2014-04-21 01:30:54]

https://codereview.chromium.org/244493002/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/244493002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.h:194: TrackGroup(GroupKind kind)

Add explicit.

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
File Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrack.cpp:128: }

Actually I don't fully understand the relationship between TextTrack,
TextTrackCueList and VTTRegionList, and thus it's not clear to me if it's OK to
just comment out the setTrack(0)s from the destructor. Would you elaborate on it
for me?

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
File Source/core/html/track/TextTrack.h (right):

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrack.h:137: Document* m_document;

This raw pointer looks safe, but if we fix the FIXME, we can make the lifetime
more explicit.

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrack.h:139: RawPtrWillBeMember<TextTrackList>
m_trackList;

Shouldn't this be a WeakMember? This pointer needs to be cleared when the
TextTrackList dies.

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
File Source/core/html/track/TextTrackList.h (right):

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrackList.h:89: HTMLMediaElement* m_owner;

This raw pointer looks safe because it's cleared in TextTrackList::clearOwner()
when the owner element dies.

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/vtt/V...
File Source/core/html/track/vtt/VTTCue.idl (right):

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTCue.idl:33: WillBeGarbageCollected,

[WillBeGarbageCollected] is not necessary since it's inherited from
TextTrackCue.

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/vtt/V...
File Source/core/html/track/vtt/VTTParser.h (right):

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTParser.h:117: Document* m_document;

Let me confirm: The VTTParser object can outlive the document (because both the
VTTParser object and the document are on-heap), but it's not possible that
methods of the VTTParser object are called after the document dies (because the
VTTParser object is owned by the client object, and the client object doesn't
outlive the document). Thus using a raw pointer for m_document is safe. Is my
understanding correct?

Nit: I'd change Document* to Document&.

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTParser.h:145: VTTParserClient* m_client;

Let me confirm: The VTTParser object can outlive the client object (because the
VTTParser object is on-heap but the client object is off-heap), but it's not
possible that methods of the VTTParser object are called after the client object
dies (because the VTTParser object is owned by the client). Thus using a raw
pointer for m_client is safe here. Right?

https://codereview.chromium.org/244493002/diff/1/Source/platform/heap/Handle.h
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/244493002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:742: #define RawPtrWillBePersistent
WebCore::Persistent

Oh, this macro seems dangerous :) I agree that we need this in some situation
but we should be very careful that it won't produce memory leaks.

[sof, 2014-04-21 15:23:42]

The dangers of RefCountedGarbageCollected..InbandTextTrack and LoadableTextTrack
hadn't been converted to Oilpan types completely. Done so here + followed up on
feedback in new patchset.

(Discovered the lack of Oilpan types for these objects while looking into object
lifetime issues of another kind for them --
https://codereview.chromium.org/242113012/ )

https://codereview.chromium.org/244493002/diff/1/Source/core/html/HTMLMediaEl...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/244493002/diff/1/Source/core/html/HTMLMediaEl...
Source/core/html/HTMLMediaElement.h:194: TrackGroup(GroupKind kind)
On 2014/04/21 01:30:54, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/Inban...
File Source/core/html/track/InbandTextTrack.cpp (right):

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/Inban...
Source/core/html/track/InbandTextTrack.cpp:94: m_webTrack->setClient(0);
This is a problem - informing the platform that the client has gone away is
needed for this particular TextTrack implementation & won't happen unless we
explicitly clear the owner list of TextTracks (by way of a TextTrackList
dispose() method.) Added now.

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
File Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrack.cpp:128: }
On 2014/04/21 01:30:54, haraken wrote:
> 
> Actually I don't fully understand the relationship between TextTrack,
> TextTrackCueList and VTTRegionList, and thus it's not clear to me if it's OK
to
> just comment out the setTrack(0)s from the destructor. Would you elaborate on
it
> for me?

Cues and regions for a text track are explicitly added and removed from this
list. They maintain a strong reference back to their list, which is cleared when
they're explicitly remove()d.

Consequently, not clearing them is fine, as they'll be destructed together.

For similar reasons, m_trackList doesn't have to be checked for being empty or
not in the Oilpan case. If it is null, then the TextTrack has been removed from
a TextTrackList. If it is non-null, then the TextTrack object is being
finalized/destructed at the same time as its TextTrackList parent.

A TextTrackList persistent reference is kept by a media element. And as it
explicitly clears itself as an owner of the TextTrackList, by the time this
destructor (and that for TextTrackList) gets to run, the effect of calling e.g.
setTrack(0) is effectively a no-op, it has been detached from the media element
and won't cause any updates.

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
File Source/core/html/track/TextTrack.h (right):

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrack.h:137: Document* m_document;
On 2014/04/21 01:30:54, haraken wrote:
> 
> This raw pointer looks safe, but if we fix the FIXME, we can make the lifetime
> more explicit.

Done, I think.

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrack.h:139: RawPtrWillBeMember<TextTrackList>
m_trackList;
On 2014/04/21 01:30:54, haraken wrote:
> 
> Shouldn't this be a WeakMember? This pointer needs to be cleared when the
> TextTrackList dies.

I don't think it needs to be a weak reference. Lifetime of TextTrackList is
controlled by the media element; once it is destructed, this TextTrackList is
let go of. The TextTrackList and its TextTracks will die together - isn't it
simpler not to introduce weak references here?

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
File Source/core/html/track/TextTrackList.h (right):

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrackList.h:89: HTMLMediaElement* m_owner;
On 2014/04/21 01:30:54, haraken wrote:
> 
> This raw pointer looks safe because it's cleared in
TextTrackList::clearOwner()
> when the owner element dies.

Yes, that clearing must happen at that point.

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/vtt/V...
File Source/core/html/track/vtt/VTTCue.idl (right):

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTCue.idl:33: WillBeGarbageCollected,
On 2014/04/21 01:30:54, haraken wrote:
> 
> [WillBeGarbageCollected] is not necessary since it's inherited from
> TextTrackCue.

Thanks, removed.

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/vtt/V...
File Source/core/html/track/vtt/VTTParser.h (right):

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTParser.h:117: Document* m_document;
On 2014/04/21 01:30:54, haraken wrote:
> 
> Let me confirm: The VTTParser object can outlive the document (because both
the
> VTTParser object and the document are on-heap), but it's not possible that
> methods of the VTTParser object are called after the document dies (because
the
> VTTParser object is owned by the client object, and the client object doesn't
> outlive the document). Thus using a raw pointer for m_document is safe. Is my
> understanding correct?
> 
> Nit: I'd change Document* to Document&.

The client object is owned by a media element (by way of a text track and a text
track list). The media element will not outlive its document.

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/vtt/V...
Source/core/html/track/vtt/VTTParser.h:145: VTTParserClient* m_client;
On 2014/04/21 01:30:54, haraken wrote:
> 
> Let me confirm: The VTTParser object can outlive the client object (because
the
> VTTParser object is on-heap but the client object is off-heap), but it's not
> possible that methods of the VTTParser object are called after the client
object
> dies (because the VTTParser object is owned by the client). Thus using a raw
> pointer for m_client is safe here. Right?

That is correct; TextTrackLoader is the only client, and it keeps a persistent
reference to VTTParser. Hence, it is simply not possible for the VTTParser to be
accessed after the loader (==client) goes away.

Actually, we can do better and avoid this persistent reference -- I had
overlooked that LoadableTextTrack (TextTrackLoader's only owner) is also a GCed
object, so we can move the loader to the heap also. Updated patch to do so.

https://codereview.chromium.org/244493002/diff/1/Source/platform/heap/Handle.h
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/244493002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:742: #define RawPtrWillBePersistent
WebCore::Persistent
On 2014/04/21 01:30:54, haraken wrote:
> 
> Oh, this macro seems dangerous :) I agree that we need this in some situation
> but we should be very careful that it won't produce memory leaks.

If we don't want to add it for that reason, the alternative is to keep a bare
pointer on VTTCueBox. Ownership and lifetime of it should already be such (I
hope!) in the non-Oilpan case that the cue cannot disappear before the cue box. 

But I considered it cleaner to explicitly keep a persistent reference, hence its
addition here.

[haraken, 2014-04-22 02:37:46]

Mostly looks good.

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
File Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrack.cpp:128: }
On 2014/04/21 15:23:42, sof wrote:
> On 2014/04/21 01:30:54, haraken wrote:
> > 
> > Actually I don't fully understand the relationship between TextTrack,
> > TextTrackCueList and VTTRegionList, and thus it's not clear to me if it's OK
> to
> > just comment out the setTrack(0)s from the destructor. Would you elaborate
on
> it
> > for me?
> 
> Cues and regions for a text track are explicitly added and removed from this
> list. They maintain a strong reference back to their list, which is cleared
when
> they're explicitly remove()d.
> 
> Consequently, not clearing them is fine, as they'll be destructed together.
> 
> For similar reasons, m_trackList doesn't have to be checked for being empty or
> not in the Oilpan case. If it is null, then the TextTrack has been removed
from
> a TextTrackList. If it is non-null, then the TextTrack object is being
> finalized/destructed at the same time as its TextTrackList parent.
> 
> A TextTrackList persistent reference is kept by a media element. And as it
> explicitly clears itself as an owner of the TextTrackList, by the time this
> destructor (and that for TextTrackList) gets to run, the effect of calling
e.g.
> setTrack(0) is effectively a no-op, it has been detached from the media
element
> and won't cause any updates.

Thanks for the detailed explanation. You explanation makes sense, but does it
mean that we don't need the code (line 118 - 127) even in non-oilpan builds? If
TextTrackList and TextTracks in the list die together, we won't need to
explicitly call setTrack(0).

https://codereview.chromium.org/244493002/diff/1/Source/platform/heap/Handle.h
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/244493002/diff/1/Source/platform/heap/Handle....
Source/platform/heap/Handle.h:742: #define RawPtrWillBePersistent
WebCore::Persistent
On 2014/04/21 15:23:42, sof wrote:
> On 2014/04/21 01:30:54, haraken wrote:
> > 
> > Oh, this macro seems dangerous :) I agree that we need this in some
situation
> > but we should be very careful that it won't produce memory leaks.
> 
> If we don't want to add it for that reason, the alternative is to keep a bare
> pointer on VTTCueBox. Ownership and lifetime of it should already be such (I
> hope!) in the non-Oilpan case that the cue cannot disappear before the cue
box. 
> 
> But I considered it cleaner to explicitly keep a persistent reference, hence
its
> addition here.

Agreed. Memory leak would be better than crash.

We just need to be careful that we don't introduce memory leaks when introducing
RawPtrWillBePersistent.

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:311: m_textTracks->dispose();

Can we merge clearOwner() into dispose()?

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/I...
File Source/core/html/track/InbandTextTrack.cpp (right):

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/I...
Source/core/html/track/InbandTextTrack.cpp:102: if (!owner)

Can this happen? In non-oilpan builds, it doesn't happen that addWebVTTCue is
called after the document is gone. Thus it shouldn't happen in oilpan builds
either.

Shall we add ASSERT(owner)?

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/L...
File Source/core/html/track/LoadableTextTrack.h (right):

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/L...
Source/core/html/track/LoadableTextTrack.h:71: HTMLTrackElement* m_trackElement;

This raw pointer looks safe because it's cleared in clearTrackElement() when the
track element dies.

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/T...
File Source/core/html/track/TextTrackList.cpp (right):

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/T...
Source/core/html/track/TextTrackList.cpp:56: item(i)->setTrackList(0);

Just help me understand: Why do we need to explicitly clear TextTracks in the
list? If HTMLMediaElement holds a persistent reference to TextTrackList, and
TextTrackList and TextTracks in the list have bi-directional strong references,
we won't need to do the explicit clear. When the HTMLMediaElement drops the
persistent reference, both the TextTrackList and TextTracks will be collected in
a next GC at the same time.

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTRegionList.h (right):

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTRegionList.h:55: void clear();

This clear() method is unused. You can remove it.

https://codereview.chromium.org/244493002/diff/20001/Source/core/loader/TextT...
File Source/core/loader/TextTrackLoader.h (right):

https://codereview.chromium.org/244493002/diff/20001/Source/core/loader/TextT...
Source/core/loader/TextTrackLoader.h:87: TextTrackLoaderClient& m_client;

This raw pointer looks safe, because the client holds an OwnPtr to the
TextTrackLoader object.

https://codereview.chromium.org/244493002/diff/20001/Source/core/loader/TextT...
Source/core/loader/TextTrackLoader.h:90: Document& m_document;

Are you sure m_document doesn't outlive the TextTrackLoader object?

[sof, 2014-04-22 06:27:51]

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
File Source/core/html/track/TextTrack.cpp (right):

https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/TextT...
Source/core/html/track/TextTrack.cpp:128: }
On 2014/04/22 02:37:47, haraken wrote:
> On 2014/04/21 15:23:42, sof wrote:
> > On 2014/04/21 01:30:54, haraken wrote:
> > > 
> > > Actually I don't fully understand the relationship between TextTrack,
> > > TextTrackCueList and VTTRegionList, and thus it's not clear to me if it's
OK
> > to
> > > just comment out the setTrack(0)s from the destructor. Would you elaborate
> on
> > it
> > > for me?
> > 
> > Cues and regions for a text track are explicitly added and removed from this
> > list. They maintain a strong reference back to their list, which is cleared
> when
> > they're explicitly remove()d.
> > 
> > Consequently, not clearing them is fine, as they'll be destructed together.
> > 
> > For similar reasons, m_trackList doesn't have to be checked for being empty
or
> > not in the Oilpan case. If it is null, then the TextTrack has been removed
> from
> > a TextTrackList. If it is non-null, then the TextTrack object is being
> > finalized/destructed at the same time as its TextTrackList parent.
> > 
> > A TextTrackList persistent reference is kept by a media element. And as it
> > explicitly clears itself as an owner of the TextTrackList, by the time this
> > destructor (and that for TextTrackList) gets to run, the effect of calling
> e.g.
> > setTrack(0) is effectively a no-op, it has been detached from the media
> element
> > and won't cause any updates.
> 
> Thanks for the detailed explanation. You explanation makes sense, but does it
> mean that we don't need the code (line 118 - 127) even in non-oilpan builds?
If
> TextTrackList and TextTracks in the list die together, we won't need to
> explicitly call setTrack(0).

That is a good observation. As the pointers in the reverse direction are just
raw pointers in the non-Oilpan case, clearing them out has no practical function
at this stage. And we're already letting go of the ref pointers in the other
direction to release the track & cue objects, so no further work needed here.

Removed these two loops + #if.

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:311: m_textTracks->dispose();
On 2014/04/22 02:37:47, haraken wrote:
> 
> Can we merge clearOwner() into dispose()?

Done; dispose() remained.

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/I...
File Source/core/html/track/InbandTextTrack.cpp (right):

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/I...
Source/core/html/track/InbandTextTrack.cpp:102: if (!owner)
On 2014/04/22 02:37:47, haraken wrote:
> 
> Can this happen? In non-oilpan builds, it doesn't happen that addWebVTTCue is
> called after the document is gone. Thus it shouldn't happen in oilpan builds
> either.
> 
> Shall we add ASSERT(owner)?

Right, let's keep the behavior the exact-same.

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/T...
File Source/core/html/track/TextTrackList.cpp (right):

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/T...
Source/core/html/track/TextTrackList.cpp:56: item(i)->setTrackList(0);
On 2014/04/22 02:37:47, haraken wrote:
> 
> Just help me understand: Why do we need to explicitly clear TextTracks in the
> list? If HTMLMediaElement holds a persistent reference to TextTrackList, and
> TextTrackList and TextTracks in the list have bi-directional strong
references,
> we won't need to do the explicit clear. When the HTMLMediaElement drops the
> persistent reference, both the TextTrackList and TextTracks will be collected
in
> a next GC at the same time.

Yes, no need to do the clearing for any of those. The reason it is needed is due
to the comment in
https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/Inban...

i.e., notifying the platform client that the track has gone away needs to
happen, and it looks/ed dicey to delay until the InbandTextTrack object is
finalized.

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/v...
File Source/core/html/track/vtt/VTTRegionList.h (right):

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/v...
Source/core/html/track/vtt/VTTRegionList.h:55: void clear();
On 2014/04/22 02:37:47, haraken wrote:
> 
> This clear() method is unused. You can remove it.

Nice; removed.

https://codereview.chromium.org/244493002/diff/20001/Source/core/loader/TextT...
File Source/core/loader/TextTrackLoader.h (right):

https://codereview.chromium.org/244493002/diff/20001/Source/core/loader/TextT...
Source/core/loader/TextTrackLoader.h:90: Document& m_document;
On 2014/04/22 02:37:47, haraken wrote:
> 
> Are you sure m_document doesn't outlive the TextTrackLoader object?

I am certain; it is provided by the client (LoadableTextTrack), and we've
determined that its lifetime is within bounds of the Document (i.e., it reduces
to the question if the media element can outlive its document.)

(I couldn't see a non ad-hoc way of using the client to supply the document in
the places where the loader consults it, so left it this way.)

[haraken, 2014-04-22 06:41:08]

LGTM.

I want to have one more oilpan reviewer take another close look.

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/T...
File Source/core/html/track/TextTrackList.cpp (right):

https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/T...
Source/core/html/track/TextTrackList.cpp:56: item(i)->setTrackList(0);
On 2014/04/22 06:27:52, sof wrote:
> On 2014/04/22 02:37:47, haraken wrote:
> > 
> > Just help me understand: Why do we need to explicitly clear TextTracks in
the
> > list? If HTMLMediaElement holds a persistent reference to TextTrackList, and
> > TextTrackList and TextTracks in the list have bi-directional strong
> references,
> > we won't need to do the explicit clear. When the HTMLMediaElement drops the
> > persistent reference, both the TextTrackList and TextTracks will be
collected
> in
> > a next GC at the same time.
> 
> Yes, no need to do the clearing for any of those. The reason it is needed is
due
> to the comment in
>
https://codereview.chromium.org/244493002/diff/1/Source/core/html/track/Inban...
> 
> i.e., notifying the platform client that the track has gone away needs to
> happen, and it looks/ed dicey to delay until the InbandTextTrack object is
> finalized.

ah, that makes sense. Thanks for the clarification.

[sof, 2014-04-22 06:51:02]

On 2014/04/22 06:41:08, haraken wrote:
> LGTM.
> 
> I want to have one more oilpan reviewer take another close look.
> 

Thanks, yes.

(Rebasing past r172065, as the patchset#3 fails to apply cleanly right now.)

[Mads Ager (chromium), 2014-04-22 10:33:25]

I think we can remove all the pointer clearing from this code. It seems to me
that TextTrack, TextTrackList and HTMLMediaElement should use only strong
pointers to each other in the oilpan build. Once that is the case we do not have
to set these pointers to zero during destructors.

We shouldn't clear pointers in destructors. If we need pointer clearing we
should use WeakMember. But I don't think that is needed here. We should just
have strong pointers. It looks like the raw pointers here are all to break
reference cycles which is not a problem with oilpan.

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.cpp:309: if (m_textTracks)
m_textTracks is another garbage collected object. We cannot touch it here. We
shouldn't have the dispose call here. It seems to me that the TextTrackList and
the HTMLMediaElement should have strong pointers to each other (Member in both
directions) and that the dispose call should not be needed with oilpan?
clearOwner is currently just setting the owner pointer to 0 which we don't have
to do if they live and die together. I think we can #ifdef this out for the
oilpan build?

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:507: RefPtrWillBePersistent<TextTrackList>
m_textTracks;
HTMLMediaElement is in the Oilpan heap and there is a trace method. Please use
Member here and trace it (which might lead to finalization-order changes, so we
need to be careful that the TextTrackList destructor does not touch the
HTMLMediaElement.)

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:508:
WillBePersistentHeapVector<RefPtrWillBeMember<TextTrack> >
m_textTracksWhenResourceSelectionBegan;
Remove the Persistent here as well and just make it a WillBeHeapVector and trace
it. Again, we need to double-check the TextTrack destructor.

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/track/T...
File Source/core/html/track/TextTrackList.cpp (right):

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/track/T...
Source/core/html/track/TextTrackList.cpp:53: void TextTrackList::dispose()
Do we need this with oilpan at all? Can't all the pointers be strong pointers
here? In that case you never have to clear them and this can stay in the
destructor and be #ifdef'd out in the oilpan build.

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/track/T...
File Source/core/html/track/TextTrackList.h (right):

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/track/T...
Source/core/html/track/TextTrackList.h:89: HTMLMediaElement* m_owner;
Is there any reason to keep this a raw pointer? It looks to me like this is only
a raw pointer to break a reference cycle? I think this should just be a Member
and then the text track list and the media element will die together unless the
text track list is explicitly remove from the media element. That sounds like
the right behavior.

[haraken, 2014-04-22 10:40:02]

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:507: RefPtrWillBePersistent<TextTrackList>
m_textTracks;
On 2014/04/22 10:33:25, Mads Ager (chromium) wrote:
> HTMLMediaElement is in the Oilpan heap and there is a trace method. Please use
> Member here and trace it (which might lead to finalization-order changes, so
we
> need to be careful that the TextTrackList destructor does not touch the
> HTMLMediaElement.)

Good point. I forgot that the Node hierarchy is already on the heap.

Probably do we need to implement a complete trace chain first?

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/track/T...
File Source/core/html/track/TextTrackList.cpp (right):

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/track/T...
Source/core/html/track/TextTrackList.cpp:53: void TextTrackList::dispose()
On 2014/04/22 10:33:25, Mads Ager (chromium) wrote:
> Do we need this with oilpan at all? Can't all the pointers be strong pointers
> here? In that case you never have to clear them and this can stay in the
> destructor and be #ifdef'd out in the oilpan build.

This dispose() is needed to explicitly call InbandTextTrack::setTrackList. See
https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/T...

[sof, 2014-04-22 10:43:42]

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/track/T...
File Source/core/html/track/TextTrackList.cpp (right):

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/track/T...
Source/core/html/track/TextTrackList.cpp:53: void TextTrackList::dispose()
On 2014/04/22 10:40:03, haraken wrote:
> On 2014/04/22 10:33:25, Mads Ager (chromium) wrote:
> > Do we need this with oilpan at all? Can't all the pointers be strong
pointers
> > here? In that case you never have to clear them and this can stay in the
> > destructor and be #ifdef'd out in the oilpan build.
> 
> This dispose() is needed to explicitly call InbandTextTrack::setTrackList. See
>
https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/T...

Yes, the only reason for explicitly clearing.

[Mads Ager (chromium), 2014-04-22 10:55:03]

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/track/T...
File Source/core/html/track/TextTrackList.cpp (right):

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/track/T...
Source/core/html/track/TextTrackList.cpp:53: void TextTrackList::dispose()
On 2014/04/22 10:43:43, sof wrote:
> On 2014/04/22 10:40:03, haraken wrote:
> > On 2014/04/22 10:33:25, Mads Ager (chromium) wrote:
> > > Do we need this with oilpan at all? Can't all the pointers be strong
> pointers
> > > here? In that case you never have to clear them and this can stay in the
> > > destructor and be #ifdef'd out in the oilpan build.
> > 
> > This dispose() is needed to explicitly call InbandTextTrack::setTrackList.
See
> >
>
https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/T...
> 
> Yes, the only reason for explicitly clearing.

I'm still not sure I understand why we cannot simplify. :)

We will have to do it differently since this dispose is called from a
destructor, right? In this version of the code you are calling through a
Persistent so this will work. However, that Persistent should be a Member and at
that point all of this can be dead at the same time.

These objects are exposed to JavaScript, right? In that case, the destruction of
these are already determined by when we get a garbage collection and in that
sense we will not introduce anything new, these calls of setTrackList(0) are
delayed until GC on the JS side anyway.

If we use strong pointers in all directions, we know that the TextTrack dies
together with its text track list (unless it has been explicitly removed). In
both cases (explicit removal and when the TextTrack itself dies) it can notify
the client?

[sof, 2014-04-22 11:19:25]

On 2014/04/22 10:55:03, Mads Ager (chromium) wrote:
>
https://codereview.chromium.org/244493002/diff/60001/Source/core/html/track/T...
> File Source/core/html/track/TextTrackList.cpp (right):
> 
>
https://codereview.chromium.org/244493002/diff/60001/Source/core/html/track/T...
> Source/core/html/track/TextTrackList.cpp:53: void TextTrackList::dispose()
> On 2014/04/22 10:43:43, sof wrote:
> > On 2014/04/22 10:40:03, haraken wrote:
> > > On 2014/04/22 10:33:25, Mads Ager (chromium) wrote:
> > > > Do we need this with oilpan at all? Can't all the pointers be strong
> > pointers
> > > > here? In that case you never have to clear them and this can stay in the
> > > > destructor and be #ifdef'd out in the oilpan build.
> > > 
> > > This dispose() is needed to explicitly call InbandTextTrack::setTrackList.
> See
> > >
> >
>
https://codereview.chromium.org/244493002/diff/20001/Source/core/html/track/T...
> > 
> > Yes, the only reason for explicitly clearing.
> 
> I'm still not sure I understand why we cannot simplify. :)
> 
> We will have to do it differently since this dispose is called from a
> destructor, right? In this version of the code you are calling through a
> Persistent so this will work. However, that Persistent should be a Member and
at
> that point all of this can be dead at the same time.
> 

Switching away from Persistent would no longer make an explicit dispose() call
from the HTMLMediaElement destructor valid, yes.

...
> 
> If we use strong pointers in all directions, we know that the TextTrack dies
> together with its text track list (unless it has been explicitly removed). In
> both cases (explicit removal and when the TextTrack itself dies) it can notify
> the client?

Hmm, yes - that slight delay in clearing the reference to the track object won't
be observable to the client, so it should be safe.

[sof, 2014-04-22 12:44:00]

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/HTMLMed...
File Source/core/html/HTMLMediaElement.h (right):

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:507: RefPtrWillBePersistent<TextTrackList>
m_textTracks;
On 2014/04/22 10:40:03, haraken wrote:
> On 2014/04/22 10:33:25, Mads Ager (chromium) wrote:
> > HTMLMediaElement is in the Oilpan heap and there is a trace method. Please
use
> > Member here and trace it (which might lead to finalization-order changes, so
> we
> > need to be careful that the TextTrackList destructor does not touch the
> > HTMLMediaElement.)
> 
> Good point. I forgot that the Node hierarchy is already on the heap.
> 
> Probably do we need to implement a complete trace chain first?

Already in place; switched to Persistent + removed the explicit clearing of the
tracklist as a result.

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/HTMLMed...
Source/core/html/HTMLMediaElement.h:508:
WillBePersistentHeapVector<RefPtrWillBeMember<TextTrack> >
m_textTracksWhenResourceSelectionBegan;
On 2014/04/22 10:33:25, Mads Ager (chromium) wrote:
> Remove the Persistent here as well and just make it a WillBeHeapVector and
trace
> it. Again, we need to double-check the TextTrack destructor.

I've gone through the destructors, and apart from InbandTextTrack notifying the
client, they're all empty now.

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/track/T...
File Source/core/html/track/TextTrackList.h (right):

https://codereview.chromium.org/244493002/diff/60001/Source/core/html/track/T...
Source/core/html/track/TextTrackList.h:89: HTMLMediaElement* m_owner;
On 2014/04/22 10:33:25, Mads Ager (chromium) wrote:
> Is there any reason to keep this a raw pointer? It looks to me like this is
only
> a raw pointer to break a reference cycle? I think this should just be a Member
> and then the text track list and the media element will die together unless
the
> text track list is explicitly remove from the media element. That sounds like
> the right behavior.

Yes, having a strong reference in that direction makes good sense. It takes care
of the crasher https://code.google.com/p/chromium/issues/detail?id=365260 (with
Oilpan enabled.)

[Mads Ager (chromium), 2014-04-22 13:07:14]

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:309: if (m_textTracks)
We should probably wrap this in #if !ENABLE(OILPAN) instead of deleting it. It
is probably still needed for non-oilpan builds?

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/HTMLTr...
File Source/core/html/HTMLTrackElement.cpp (left):

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/HTMLTr...
Source/core/html/HTMLTrackElement.cpp:63: if (m_track)
We should probably be careful and wrap this in #if !ENABLE(OILPAN) instead of
just deleting it?

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/track/...
File Source/core/html/track/LoadableTextTrack.cpp (left):

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/track/...
Source/core/html/track/LoadableTextTrack.cpp:52: void
LoadableTextTrack::clearTrackElement()
Probably still needed for non-oilpan?

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/track/...
File Source/core/html/track/TextTrack.cpp (left):

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/track/...
Source/core/html/track/TextTrack.cpp:117: ASSERT(!m_trackList);
#if !ENABLE(OILPAN) instead of deleting?

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/track/...
File Source/core/html/track/TextTrackList.cpp (left):

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/track/...
Source/core/html/track/TextTrackList.cpp:52: for (unsigned i = 0; i < length();
++i) {
#if !ENABLE(OILPAN)

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/track/...
File Source/core/html/track/vtt/VTTCue.h (right):

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/track/...
Source/core/html/track/vtt/VTTCue.h:59: RawPtrWillBePersistent<VTTCue> m_cue;
This looks like a leak?

We have a RefPtr from VTTCue to VTTCueBox and then we have a Persistent from
VTTCueBox to VTTCue.

I think we need to leave this as a raw pointer at this point. Unfortunately, we
cannot replace the RefPtr into the Node hierarchy with a Member yet. I suspect
that what we will want here is that the VTTCue and VTTCueBox objects live and
die together and have Members in both directions?

We can't do that yet, so we probably have to keep the raw pointer here to avoid
the leak. Maybe add a comment?

https://codereview.chromium.org/244493002/diff/100001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/244493002/diff/100001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:742: #define RawPtrWillBePersistent
WebCore::Persistent
These look a bit dangerous. If something could not or should not be a RefPtr
before there is a pretty high chance that it will leak to replace it with a
Persistent. We might have to have them at some point, but I think the use in
this changelist leaks.

[sof, 2014-04-22 14:02:47]

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:309: if (m_textTracks)
On 2014/04/22 13:07:15, Mads Ager (chromium) wrote:
> We should probably wrap this in #if !ENABLE(OILPAN) instead of deleting it. It
> is probably still needed for non-oilpan builds?

The media element is the sole creator and owner of TextTrackList instances, so
when it deref()s, the object will be destructed. Unless its destructor makes use
of the media element, this ought to be safe, right?

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/HTMLTr...
File Source/core/html/HTMLTrackElement.cpp (left):

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/HTMLTr...
Source/core/html/HTMLTrackElement.cpp:63: if (m_track)
On 2014/04/22 13:07:15, Mads Ager (chromium) wrote:
> We should probably be careful and wrap this in #if !ENABLE(OILPAN) instead of
> just deleting it?

I don't mind putting it back in (even if I risk getting complaints of redundant
#ifs :) ), but like for HTMLMediaElement & TextTrackList, the element will not
be used. HTMLTrackElement created the track object & it will be destructed just
afterwards in the non-Oilpan case.

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/track/...
File Source/core/html/track/TextTrackList.cpp (left):

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/track/...
Source/core/html/track/TextTrackList.cpp:52: for (unsigned i = 0; i < length();
++i) {
On 2014/04/22 13:07:15, Mads Ager (chromium) wrote:
> #if !ENABLE(OILPAN)

It isn't needed here either in the non-Oilpan case, unless I've overlooked some
dependency.

[Hmm, how hard to try to avoid using "#if ENABLE(OILPAN)"?]

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/track/...
File Source/core/html/track/vtt/VTTCue.h (right):

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/track/...
Source/core/html/track/vtt/VTTCue.h:59: RawPtrWillBePersistent<VTTCue> m_cue;
On 2014/04/22 13:07:15, Mads Ager (chromium) wrote:
> This looks like a leak?
> 
> We have a RefPtr from VTTCue to VTTCueBox and then we have a Persistent from
> VTTCueBox to VTTCue.
> 
> I think we need to leave this as a raw pointer at this point. Unfortunately,
we
> cannot replace the RefPtr into the Node hierarchy with a Member yet. I suspect
> that what we will want here is that the VTTCue and VTTCueBox objects live and
> die together and have Members in both directions?
> 
> We can't do that yet, so we probably have to keep the raw pointer here to
avoid
> the leak. Maybe add a comment?

Done (and removed RawPtrWillBePersistent in the process.) With the use of strong
references all the way back up to the media element (and back), it would leak
with the Persistent - thanks.

https://codereview.chromium.org/244493002/diff/100001/Source/platform/heap/Ha...
File Source/platform/heap/Handle.h (right):

https://codereview.chromium.org/244493002/diff/100001/Source/platform/heap/Ha...
Source/platform/heap/Handle.h:742: #define RawPtrWillBePersistent
WebCore::Persistent
On 2014/04/22 13:07:15, Mads Ager (chromium) wrote:
> These look a bit dangerous. If something could not or should not be a RefPtr
> before there is a pretty high chance that it will leak to replace it with a
> Persistent. We might have to have them at some point, but I think the use in
> this changelist leaks.

I've removed the transition type.

[Mads Ager (chromium), 2014-04-22 14:17:59]

I don't think removing the clearing of pointers is safe. There are JavaScript
wrappers for these objects that also keep them alive independently of the owner
object. Therefore, when the owner object goes away that does not mean that the
other objects are dead. There can still be JS wrappers for them.

With Oilpan that will be fine because neither of the objects will die until the
JS wrapper is also gone. However, in the non-oilpan world that can lead to use
after free because the owner pointer is a raw pointer.

At least I *think* that is the case and if there is any doubt, we need to leave
in the clearing in the non-oilpan build. Otherwise we will introduce
use-after-free bugs which is very serious seen from a security standpoint.
Better safe than sorry!

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/HTMLMe...
File Source/core/html/HTMLMediaElement.cpp (left):

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/HTMLMe...
Source/core/html/HTMLMediaElement.cpp:309: if (m_textTracks)
On 2014/04/22 14:02:48, sof wrote:
> On 2014/04/22 13:07:15, Mads Ager (chromium) wrote:
> > We should probably wrap this in #if !ENABLE(OILPAN) instead of deleting it.
It
> > is probably still needed for non-oilpan builds?
> 
> The media element is the sole creator and owner of TextTrackList instances, so
> when it deref()s, the object will be destructed. Unless its destructor makes
use
> of the media element, this ought to be safe, right?

No, I don't think it is. It is only safe if they always live and die together.
We make that happen in the oilpan build, but it is not the case in the
non-oilpan build. There can be JavaScript wrappers for the TextTrackList that
can out-live the owner I think. Therefore, I think it is extremely dangerous for
us to remove clearing of pointers from the non-oilpan build. We are likely to
introduce security issues where we allow dangling pointers.

I would strongly prefer that we ifdef out clearing of pointers only for the
oilpan build. We have to be 100% sure that we can leave out the clearing in the
non-oilpan build. I'm not at all sure in these cases and I think that because of
JavaScript wrappers for these objects we need to keep the clearing.

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/HTMLTr...
File Source/core/html/HTMLTrackElement.cpp (left):

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/HTMLTr...
Source/core/html/HTMLTrackElement.cpp:63: if (m_track)
On 2014/04/22 14:02:48, sof wrote:
> On 2014/04/22 13:07:15, Mads Ager (chromium) wrote:
> > We should probably be careful and wrap this in #if !ENABLE(OILPAN) instead
of
> > just deleting it?
> 
> I don't mind putting it back in (even if I risk getting complaints of
redundant
> #ifs :) ), but like for HTMLMediaElement & TextTrackList, the element will not
> be used. HTMLTrackElement created the track object & it will be destructed
just
> afterwards in the non-Oilpan case.

What if it has a JavaScript wrapper which also keeps it alive and can interact
with it? Use-after-free? We need to be very careful when removing pointer
clearing in the non-oilpan build. Some of them can probably be removed but it is
very dangerous.

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/track/...
File Source/core/html/track/TextTrackList.cpp (left):

https://codereview.chromium.org/244493002/diff/100001/Source/core/html/track/...
Source/core/html/track/TextTrackList.cpp:52: for (unsigned i = 0; i < length();
++i) {
On 2014/04/22 14:02:48, sof wrote:
> On 2014/04/22 13:07:15, Mads Ager (chromium) wrote:
> > #if !ENABLE(OILPAN)
> 
> It isn't needed here either in the non-Oilpan case, unless I've overlooked
some
> dependency.
> 
> [Hmm, how hard to try to avoid using "#if ENABLE(OILPAN)"?]

When it comes to clearing of pointers, please do not try too hard to avoid #if
!ENABLE(OILPAN). Getting this wrong is a serious security issue and we need to
be very careful.

[sof, 2014-04-22 15:50:36]

On 2014/04/22 14:17:59, Mads Ager (chromium) wrote:
> I don't think removing the clearing of pointers is safe. There are JavaScript
> wrappers for these objects that also keep them alive independently of the
owner
> object. Therefore, when the owner object goes away that does not mean that the
> other objects are dead. There can still be JS wrappers for them.
> 
> With Oilpan that will be fine because neither of the objects will die until
the
> JS wrapper is also gone. However, in the non-oilpan world that can lead to use
> after free because the owner pointer is a raw pointer.
> 
> At least I *think* that is the case and if there is any doubt, we need to
leave
> in the clearing in the non-oilpan build. Otherwise we will introduce
> use-after-free bugs which is very serious seen from a security standpoint.
> Better safe than sorry!
> 

I am taking no risks, added back. There is extra synchrony in lifetime with the
wrapper dependencies on the media element now being registered, but we could
still run into problems (without clearing.)

[haraken, 2014-04-23 01:51:44]

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/InbandTextTrack.cpp (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/InbandTextTrack.cpp:80: m_webTrack->setClient(0);

Just to confirm: In the previous code review, you were saying that setClient()
needs to be executed promptly when the last reference to the InbandTextTrack
object is gone. That is not true, and it's OK to delay the setClient()
notification to the next GC, right?

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/LoadableTextTrack.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/LoadableTextTrack.h:73: HTMLTrackElement* m_trackElement;

Can't this become a RawPtrWillBeWeakMember? (Probably is it not yet safe to use
a (Weak)Member to the Node hierarchy?)

If we need to keep it as a raw pointer, I think we also need to keep
clearTrackElement() in oilpan builds as well. Otherwise, m_trackElement can
become a stale pointer (although I don't know if it causes any real issue in
this specific case).

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/TextTrackCue.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/TextTrackCue.h:114: RawPtrWillBeMember<TextTrack>
m_track;

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

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/TextTrackList.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/TextTrackList.h:91: RawPtrWillBeMember<HTMLMediaElement>
m_owner;

Shouldn't this be a WeakMember?

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/vtt/VTTRegion.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/vtt/VTTRegion.h:143: RawPtrWillBeMember<TextTrack>
m_track;

Ditto. Probably this should be a WeakMember because the VTTRegion object can
outlive the TextTrack object if a JS wrapper holds the VTTRegion object.

[sof, 2014-04-23 05:38:46]

I'm sorry, but I find my self repeating myself and the reasoning too much in
these reviews.

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/TextTrackCue.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/TextTrackCue.h:114: RawPtrWillBeMember<TextTrack>
m_track;
On 2014/04/23 01:51:45, haraken wrote:
> 
> Shouldn't this be a WeakMember? The TextTrackCue object can outlive the
> TextTrack object if a JS holds a wrapper to the TextTrack object.

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?)

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/TextTrackList.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/TextTrackList.h:91: RawPtrWillBeMember<HTMLMediaElement>
m_owner;
On 2014/04/23 01:51:45, haraken wrote:
> 
> Shouldn't this be a WeakMember?

No. 

See https://code.google.com/p/chromium/issues/detail?id=365260

[haraken, 2014-04-23 05:47:08]

On 2014/04/23 05:38:46, sof wrote:
> I'm sorry, but I find my self repeating myself and the reasoning too much in
> these reviews.

Sorry for the repeated reasoning, but the situation changed since we changed the
persistent handle in HTMLMediaElement to a member and reverted clearing code.
I'm double-checking.

[haraken, 2014-04-23 05:53:53]

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/TextTrackCue.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/TextTrackCue.h:114: RawPtrWillBeMember<TextTrack>
m_track;
On 2014/04/23 05:38:47, sof wrote:
> On 2014/04/23 01:51:45, haraken wrote:
> > 
> > Shouldn't this be a WeakMember? The TextTrackCue object can outlive the
> > TextTrack object if a JS holds a wrapper to the TextTrack object.
> 
> 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?)

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.)

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/TextTrackList.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/TextTrackList.h:91: RawPtrWillBeMember<HTMLMediaElement>
m_owner;
On 2014/04/23 05:38:47, sof wrote:
> On 2014/04/23 01:51:45, haraken wrote:
> > 
> > Shouldn't this be a WeakMember?
> 
> No. 
> 
> See https://code.google.com/p/chromium/issues/detail?id=365260

Ah, makes sense. This is implemented as a weak member in the current non-oilpan
implementation, but that is wrong. So you're fixing the wrong behavior in the
oilpan implementation. Am I understanding correctly?

Then do you need to add some tests to OilpanTestExpectations or add a new test
for this behavior change?

[Mads Ager (chromium), 2014-04-23 05:59:37]

Quick comments. I'll do the full review next.

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/LoadableTextTrack.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/LoadableTextTrack.h:73: HTMLTrackElement* m_trackElement;
On 2014/04/23 01:51:45, haraken wrote:
> 
> Can't this become a RawPtrWillBeWeakMember? (Probably is it not yet safe to
use
> a (Weak)Member to the Node hierarchy?)
> 
> If we need to keep it as a raw pointer, I think we also need to keep
> clearTrackElement() in oilpan builds as well. Otherwise, m_trackElement can
> become a stale pointer (although I don't know if it causes any real issue in
> this specific case).

This is slightly annoying. We have to use RefPtrs and not Members into the Node
hierarchy for a little while more. However, weak members are fine. The problem
with using Member is that the node hierarchy is not yet properly traced and
therefore a Member to a Node does not keep the entire tree alive the way it
should. I'm working on that. WeakMembers are not intended to keep anything
alive, so they work as they should. So, for now this should probably be a weak
member.
I think this should really be a strong pointer though. Once a LoadableTextTrack
is associated with an HTMLTrackElement they belong together and should live and
die together. We should have strong pointers between them to ensure that. Once
we have that we don't have to clear anything. In general, we should use strong
pointers over weak pointers whenever it makes sense and it seems to me that it
makes a lot of sense to use strong pointers in most of this code.

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/TextTrackCue.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/TextTrackCue.h:114: RawPtrWillBeMember<TextTrack>
m_track;
On 2014/04/23 05:53:54, haraken wrote:
> On 2014/04/23 05:38:47, sof wrote:
> > On 2014/04/23 01:51:45, haraken wrote:
> > > 
> > > Shouldn't this be a WeakMember? The TextTrackCue object can outlive the
> > > TextTrack object if a JS holds a wrapper to the TextTrack object.
> > 
> > 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?)
> 
> 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.)

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.

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/vtt/VTTRegion.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/vtt/VTTRegion.h:143: RawPtrWillBeMember<TextTrack>
m_track;
On 2014/04/23 01:51:45, haraken wrote:
> 
> Ditto. Probably this should be a WeakMember because the VTTRegion object can
> outlive the TextTrack object if a JS wrapper holds the VTTRegion object.

No, this is a raw pointer because it couldn't be a RefPtr because that would
lead to a ref cycle that would not be reclaimed. This should be a strong
pointer.

[Mads Ager (chromium), 2014-04-23 06:10:10]

LGTM, thanks for iterating on this Sigbjorn!

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/LoadableTextTrack.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/LoadableTextTrack.h:73: HTMLTrackElement* m_trackElement;
We should probably make this a WeakMember for now. Could you add a "FIXME:
Oilpan: This should be a strong pointer once Member pointers into the Node
hierarchy can be used." Then I can find it and update it when I make that
change.

[haraken, 2014-04-23 06:21:25]

LGTM.

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/InbandTextTrack.cpp (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/InbandTextTrack.cpp:80: m_webTrack->setClient(0);
On 2014/04/23 01:51:45, haraken wrote:
> 
> Just to confirm: In the previous code review, you were saying that setClient()
> needs to be executed promptly when the last reference to the InbandTextTrack
> object is gone. That is not true, and it's OK to delay the setClient()
> notification to the next GC, right?

As far as I see the code, it looks OK to defer setClient() to the next GC.

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/TextTrackCue.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/TextTrackCue.h:114: RawPtrWillBeMember<TextTrack>
m_track;
On 2014/04/23 05:59:38, Mads Ager (chromium) wrote:
> On 2014/04/23 05:53:54, haraken wrote:
> > On 2014/04/23 05:38:47, sof wrote:
> > > On 2014/04/23 01:51:45, haraken wrote:
> > > > 
> > > > Shouldn't this be a WeakMember? The TextTrackCue object can outlive the
> > > > TextTrack object if a JS holds a wrapper to the TextTrack object.
> > > 
> > > 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?)
> > 
> > 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.)
> 
> 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.

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 06:30:00]

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/InbandTextTrack.cpp (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/InbandTextTrack.cpp:80: m_webTrack->setClient(0);
On 2014/04/23 01:51:45, haraken wrote:
> 
> Just to confirm: In the previous code review, you were saying that setClient()
> needs to be executed promptly when the last reference to the InbandTextTrack
> object is gone. That is not true, and it's OK to delay the setClient()
> notification to the next GC, right?

The media element no longer holding a persistent reference changed matters --
this track object will now be finalized in the same sweep, so switching from
doing it in an explicit clearing operation to the destructor, will not be
observable to the client.

[sof, 2014-04-23 07:36:47]

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/LoadableTextTrack.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/LoadableTextTrack.h:73: HTMLTrackElement* m_trackElement;
On 2014/04/23 06:10:11, Mads Ager (chromium) wrote:
> We should probably make this a WeakMember for now. Could you add a "FIXME:
> Oilpan: This should be a strong pointer once Member pointers into the Node
> hierarchy can be used." Then I can find it and update it when I make that
> change.

Did that just that & adopted haraken's RawPtrWillBeWeakMember suggestion, along
with a comment.

Leaving it raw pointer in the Oilpan case might cause trouble if you take
HTMLTrackElement.track and put it inside another text track list (looks unusual,
but I think that's possible). The object could then live longer and possibly
access the invalid pointer.

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/TextTrackCue.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/TextTrackCue.h:114: RawPtrWillBeMember<TextTrack>
m_track;
On 2014/04/23 06:21:26, haraken wrote:
> On 2014/04/23 05:59:38, Mads Ager (chromium) wrote:
> > On 2014/04/23 05:53:54, haraken wrote:
> > > On 2014/04/23 05:38:47, sof wrote:
> > > > On 2014/04/23 01:51:45, haraken wrote:
> > > > > 
> > > > > Shouldn't this be a WeakMember? The TextTrackCue object can outlive
the
> > > > > TextTrack object if a JS holds a wrapper to the TextTrack object.
> > > > 
> > > > 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?)
> > > 
> > > 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.)
> > 
> > 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.
> 
> 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 :)

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

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/TextTrackList.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/TextTrackList.h:91: RawPtrWillBeMember<HTMLMediaElement>
m_owner;
On 2014/04/23 05:53:54, haraken wrote:
> On 2014/04/23 05:38:47, sof wrote:
> > On 2014/04/23 01:51:45, haraken wrote:
> > > 
> > > Shouldn't this be a WeakMember?
> > 
> > No. 
> > 
> > See https://code.google.com/p/chromium/issues/detail?id=365260
> 
> Ah, makes sense. This is implemented as a weak member in the current
non-oilpan
> implementation, but that is wrong. So you're fixing the wrong behavior in the
> oilpan implementation. Am I understanding correctly?
> 
> Then do you need to add some tests to OilpanTestExpectations or add a new test
> for this behavior change?

We need to address that crasher in some way for the non-Oilpan case. Perhaps we
can handle the extra tests via it?

[haraken, 2014-04-23 07:45:25]

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/TextTrackList.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/TextTrackList.h:91: RawPtrWillBeMember<HTMLMediaElement>
m_owner;
On 2014/04/23 07:36:48, sof wrote:
> On 2014/04/23 05:53:54, haraken wrote:
> > On 2014/04/23 05:38:47, sof wrote:
> > > On 2014/04/23 01:51:45, haraken wrote:
> > > > 
> > > > Shouldn't this be a WeakMember?
> > > 
> > > No. 
> > > 
> > > See https://code.google.com/p/chromium/issues/detail?id=365260
> > 
> > Ah, makes sense. This is implemented as a weak member in the current
> non-oilpan
> > implementation, but that is wrong. So you're fixing the wrong behavior in
the
> > oilpan implementation. Am I understanding correctly?
> > 
> > Then do you need to add some tests to OilpanTestExpectations or add a new
test
> > for this behavior change?
> 
> We need to address that crasher in some way for the non-Oilpan case. Perhaps
we
> can handle the extra tests via it?

Probably you can add the test to this CL, and then mark it as [CRASH] in
TestExpectation but mark nothing in OilpanTestExpectation?

It sounds nicer if you could fix the crash in the non-oilpan case, but it won't
be worth spending a lot of time on it.

[Erik Corry, 2014-04-23 08:15:20]

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
File Source/core/html/track/TextTrackCue.h (right):

https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
Source/core/html/track/TextTrackCue.h:114: RawPtrWillBeMember<TextTrack>
m_track;
On 2014/04/23 06:21:26, haraken wrote:
> 
> 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).

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.

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

[sof, 2014-04-23 09:26:06]

On 2014/04/23 07:45:25, haraken wrote:
>
https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
> File Source/core/html/track/TextTrackList.h (right):
> 
>
https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
> Source/core/html/track/TextTrackList.h:91:
RawPtrWillBeMember<HTMLMediaElement>
> m_owner;
> On 2014/04/23 07:36:48, sof wrote:
> > On 2014/04/23 05:53:54, haraken wrote:
> > > On 2014/04/23 05:38:47, sof wrote:
> > > > On 2014/04/23 01:51:45, haraken wrote:
> > > > > 
> > > > > Shouldn't this be a WeakMember?
> > > > 
> > > > No. 
> > > > 
> > > > See https://code.google.com/p/chromium/issues/detail?id=365260
> > > 
> > > Ah, makes sense. This is implemented as a weak member in the current
> > non-oilpan
> > > implementation, but that is wrong. So you're fixing the wrong behavior in
> the
> > > oilpan implementation. Am I understanding correctly?
> > > 
> > > Then do you need to add some tests to OilpanTestExpectations or add a new
> test
> > > for this behavior change?
> > 
> > We need to address that crasher in some way for the non-Oilpan case. Perhaps
> we
> > can handle the extra tests via it?
> 
> Probably you can add the test to this CL, and then mark it as [CRASH] in
> TestExpectation but mark nothing in OilpanTestExpectation?
> 

Discovered that there is coverage for this already,
https://code.google.com/p/chromium/issues/detail?id=339592 

That (external, I suppose) test could be less flaky and force a GC, so I added a
local, reduced version of it.

Let me know if that's ok (and if media/video-focused people could review the
testcase if so, that'd be great.)

[haraken, 2014-04-23 11:26:06]

The added test LGTM.

[sof, 2014-04-23 11:38:45]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-04-23 11:38:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sigbjornf@opera.com/244493002/180001

[Erik Corry, 2014-04-23 12:21:09]

On 2014/04/23 09:26:06, sof wrote:
> On 2014/04/23 07:45:25, haraken wrote:
> >
>
https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
> > File Source/core/html/track/TextTrackList.h (right):
> > 
> >
>
https://codereview.chromium.org/244493002/diff/140001/Source/core/html/track/...
> > Source/core/html/track/TextTrackList.h:91:
> RawPtrWillBeMember<HTMLMediaElement>
> > m_owner;
> > On 2014/04/23 07:36:48, sof wrote:
> > > On 2014/04/23 05:53:54, haraken wrote:
> > > > On 2014/04/23 05:38:47, sof wrote:
> > > > > On 2014/04/23 01:51:45, haraken wrote:
> > > > > > 
> > > > > > Shouldn't this be a WeakMember?
> > > > > 
> > > > > No. 
> > > > > 
> > > > > See https://code.google.com/p/chromium/issues/detail?id=365260
> > > > 
> > > > Ah, makes sense. This is implemented as a weak member in the current
> > > non-oilpan
> > > > implementation, but that is wrong. So you're fixing the wrong behavior
in
> > the
> > > > oilpan implementation. Am I understanding correctly?
> > > > 
> > > > Then do you need to add some tests to OilpanTestExpectations or add a
new
> > test
> > > > for this behavior change?
> > > 
> > > We need to address that crasher in some way for the non-Oilpan case.
Perhaps
> > we
> > > can handle the extra tests via it?
> > 
> > Probably you can add the test to this CL, and then mark it as [CRASH] in
> > TestExpectation but mark nothing in OilpanTestExpectation?
> > 
> 
> Discovered that there is coverage for this already,
> https://code.google.com/p/chromium/issues/detail?id=339592 
> 
> That (external, I suppose) test could be less flaky and force a GC, so I added
a
> local, reduced version of it.
> 
> Let me know if that's ok (and if media/video-focused people could review the
> testcase if so, that'd be great.)

Oh, ok in that case there was actually wrong behaviour!  Sorry for the
misinformation in this case.

[commit-bot: I haz the power, 2014-04-23 12:46:30]

Change committed as 172365