Bug 72179 - JSC wrappers for TextTrack and TextTrackCue should not be collected during event dispatch or when owner is reachable
Summary: JSC wrappers for TextTrack and TextTrackCue should not be collected during ev...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
Keywords: InRadar
: 72149 (view as bug list)
Depends on:
Blocks: 43668
  Show dependency treegraph
Reported: 2011-11-11 13:52 PST by Anna Cavender
Modified: 2011-12-23 19:49 PST (History)
7 users (show)

See Also:

Proposed patch (26.00 KB, patch)
2011-12-05 17:46 PST, Eric Carlson
no flags Details | Formatted Diff | Diff
Updated patch (26.09 KB, patch)
2011-12-05 20:52 PST, Eric Carlson
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anna Cavender 2011-11-11 13:52:52 PST
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.
Comment 1 Anna Cavender 2011-11-14 13:22:32 PST
*** Bug 72149 has been marked as a duplicate of this bug. ***
Comment 2 Radar WebKit Bug Importer 2011-11-17 12:38:31 PST
Comment 3 Eric Carlson 2011-12-05 14:15:18 PST
Wrote up 73865 for the V8 changes.
Comment 4 Eric Carlson 2011-12-05 17:46:29 PST
Created attachment 117966 [details]
Proposed patch
Comment 5 WebKit Review Bot 2011-12-05 17:52:16 PST
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.
Comment 6 Eric Carlson 2011-12-05 20:52:46 PST
Created attachment 117985 [details]
Updated patch

Fix the malformed ChangeLog.
Comment 7 Geoffrey Garen 2011-12-06 20:36:02 PST
Comment on attachment 117985 [details]
Updated patch

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


> 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.
Comment 8 Eric Carlson 2011-12-09 13:35:17 PST
Comment 9 Ryosuke Niwa 2011-12-23 19:49:46 PST
Mac rebaseline done in http://trac.webkit.org/changeset/103642.