Bug 117815 – Fix TextTrackCue::cueIndex() to handle the null case of TextTrack::cues(() properly

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

[Ruth Fong]

TextTrackCue::cueIndex() calls TextTrackCueList::getCueIndex on the return value of TextTrack::cues() without checking if that return value is null.

[Radar WebKit Bug Importer]

<rdar://problem/14211041>

[Ruth Fong]

Jer discovered this bug when commit-queue was failing media/track/media-element-enqueue-event-crash.html on a patch for bug 117765. The stack information for the crashed test can be found there.

[Ruth Fong]

Created attachment 205137 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 205137 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        <rdar://problem/14211041> Fix TextTrackCue::cueIndex() to handle the null case of TextTrack::cues(() properly (117815)

Please list the radar URL in a separate line.
Also, there is no need to repeat the bug number here.
Simply copy & paste the bug title.

> Source/WebCore/ChangeLog:8
> +        This patch adds assert statements to try to catch when the call

s/assert statements/assertions/
It's probably redundant to say "the call" since "track()->cues()" is a function call already.

[Ruth Fong]

Created attachment 205141 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 205141 [details]
Patch

Clearing flags on attachment: 205141

Committed r151845: <http://trac.webkit.org/changeset/151845>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ruth Fong]

Consider merging: https://src.chromium.org/viewvc/blink?revision=153206&view=revision

[Ruth Fong]

(In reply to comment #8)
> Consider merging: https://src.chromium.org/viewvc/blink?revision=153206&view=revision

Could there ever be a case in which tracks() is null?

[Jon Lee]

(In reply to comment #9)
> (In reply to comment #8)
> > Consider merging: https://src.chromium.org/viewvc/blink?revision=153206&view=revision
> 
> Could there ever be a case in which tracks() is null?

According to that change list:

However, per HTML rules, a track object returns a non-NULL
cue list object only if cues haven't been disabled. The
problem is that in the case for an inband text track, the
cues are disabled by default, and so the track object
returns NULL as the value of the cue list pointer. As
currently implemented, the text track cue object assumes
that the track's cue list is always non-NULL, but this
crashes the browser when the pointer value is dereferenced
to get the cue index.

Need to verify this is the case; if so, we should probably file a new bug.

[Ruth Fong]

(In reply to comment #9)
> (In reply to comment #8)
> > Consider merging: https://src.chromium.org/viewvc/blink?revision=153206&view=revision
> 
> Could there ever be a case in which tracks() is null?

track() not tracks() and I think yes.

From W3 spec on TextTrackCue: 
"The track attribute, on getting, must return the TextTrack object of the text track in whose list of cues the text track cue that the TextTrackCue object represents finds itself, if any; or null otherwise."
(http://www.w3.org/html/wg/drafts/html/master/embedded-content-0.html#texttrackcue)

Also, by inspection of the implementation of TextTrackCue::track(), the getter returns m_track, which is a pointer to a TextTrack object and initialized to null in the constructor.

If so, then track() should first be checked for a null value before track()->cues(). In the case where track() is null, would it also be appropriate to return invalidCueIndex? Or is this a case that really should never be hit?

[Ruth Fong]

(In reply to comment #10)
> According to that change list:
> 
> However, per HTML rules, a track object returns a non-NULL
> cue list object only if cues haven't been disabled. The
> problem is that in the case for an inband text track, the
> cues are disabled by default, and so the track object
> returns NULL as the value of the cue list pointer. As
> currently implemented, the text track cue object assumes
> that the track's cue list is always non-NULL, but this
> crashes the browser when the pointer value is dereferenced
> to get the cue index.
> 
> Need to verify this is the case; if so, we should probably file a new bug.

From whatWG spec (http://www.whatwg.org/specs/web-apps/current-work/#dom-texttrack-cues):
"If the text track mode of the text track that the TextTrack object represents is not the text track disabled mode, then the cues attribute must return a live TextTrackCueList object that represents the subset of the text track list of cues of the text track that the TextTrack object represents whose end times occur at or after the earliest possible position when the script started, in text track cue order. Otherwise, it must return null. When an object is returned, the same object must be returned each time."

From w3 spec (http://www.w3.org/html/wg/drafts/html/master/embedded-content-0.html#text-track-disabled):
"Disabled: Indicates that the text track is not active. Other than for the purposes of exposing the track in the DOM, the user agent is ignoring the text track. No cues are active, no events are fired, and the user agent will not attempt to obtain the track's cues."

The last part of the w3 definition makes me a bit wary ("the user agent will not attempt to obtain the track's cues"), but the chromium patch makes sense in terms of whatWG's spec.

[Ruth Fong]

Created attachment 206253 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 206253 [details]
Patch

Clearing flags on attachment: 206253

Committed r152459: <http://trac.webkit.org/changeset/152459>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Why was test case not added? Did you try using test.mp4 instead of test.ogv?

[Ruth Fong]

(In reply to comment #16)
> Why was test case not added? Did you try using test.mp4 instead of test.ogv?

The test hits a case that isn't solved by this patch: https://chromiumcodereview.appspot.com/17002002#msg31

We should consider merging this updated patch: https://src.chromium.org/viewvc/blink?revision=153810&view=revision

[Eric Carlson]

(In reply to comment #16)
> Why was test case not added? Did you try using test.mp4 instead of test.ogv?

Because the test case is worthless, see my comment here: https://chromiumcodereview.appspot.com/17002002#msg29


(In reply to comment #17)
> (In reply to comment #16)
> > Why was test case not added? Did you try using test.mp4 instead of test.ogv?
> 
> The test hits a case that isn't solved by this patch: https://chromiumcodereview.appspot.com/17002002#msg31
> 
> We should consider merging this updated patch: https://src.chromium.org/viewvc/blink?revision=153810&view=revision

Agreed.

[Eric Carlson]

(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > Why was test case not added? Did you try using test.mp4 instead of test.ogv?
> > 
> > The test hits a case that isn't solved by this patch: https://chromiumcodereview.appspot.com/17002002#msg31
> > 
> > We should consider merging this updated patch: https://src.chromium.org/viewvc/blink?revision=153810&view=revision
> 
> Agreed.

Filed https://bugs.webkit.org/show_bug.cgi?id=118682 for merging the Blink changes.