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...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
: 72149 (view as bug list)
Depends on:
Blocks: 43668
  Show dependency tree / graph
 
Reported: 2011-11-11 13:52 PST by Anna Cavender
Modified: 2011-12-23 19:49 PST (History)
7 users (show)

See Also:

Attachments
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
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

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 
<rdar://problem/10464469>
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

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