Bug 72179 – JSC wrappers for TextTrack and TextTrackCue should not be collected during event dispatch or when owner is reachable

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Anna Cavender]

According to eric.carlson:
Every non-DOM object that is an EventTarget needs code (for JSC at lease, I don't know about V8) to make sure that the wrapper isn't collected during event dispatch or while the parent/owner object is reachable. For example, see JSTextTrackListOwner::isReachableFromOpaqueRoots and the tracklist-is-reachable.html test added in https://bugs.webkit.org/show_bug.cgi?id=71123.

[Anna Cavender]

*** Bug 72149 has been marked as a duplicate of this bug. ***

[Radar WebKit Bug Importer]

<rdar://problem/10464469>

[Eric Carlson]

Wrote up 73865 for the V8 changes.

[Eric Carlson]

Created attachment 117966 [details]
Proposed patch

[WebKit Review Bot]

Attachment 117966 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/medi..." exit_code: 1

LayoutTests/ChangeLog:8:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Eric Carlson]

Created attachment 117985 [details]
Updated patch

Fix the malformed ChangeLog.

[Geoffrey Garen]

Comment on attachment 117985 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=117985&action=review

r=me

> Source/WebCore/bindings/js/JSTextTrackCueCustom.cpp:57
> +    return visitor.containsOpaqueRoot(root(textTrackCue->track()->mediaElement()));

Slightly nicer to call "root(textTrackCue->track())", and put root(TextTrack*) inline in a header. That way, if a text track ever changes its mind about what its root is, we only have to update one function.

> Source/WebCore/bindings/js/JSTextTrackCustom.cpp:52
> +    return visitor.containsOpaqueRoot(root(textTrack->mediaElement()));

This would be "root(textTrack)", if you followed my advice above.

> Source/WebCore/html/track/TextTrackList.cpp:93
> +    scheduleAddTrackEvent(track);

Can this be "track.release()"?

> Source/WebCore/html/track/TextTrackList.cpp:96
> +void TextTrackList::remove(PassRefPtr<TextTrack> prpTrack)

Seems like prpTrack should just be a raw pointer, since we don't take ownership.

[Eric Carlson]

http://trac.webkit.org/changeset/102471

[Ryosuke Niwa]

Mac rebaseline done in http://trac.webkit.org/changeset/103642.