TextTrackCue::cueIndex() calls TextTrackCueList::getCueIndex on the return value of TextTrack::cues() without checking if that return value is null.
<rdar://problem/14211041>
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.
Created attachment 205137 [details] Patch
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.
Created attachment 205141 [details] Patch
Comment on attachment 205141 [details] Patch Clearing flags on attachment: 205141 Committed r151845: <http://trac.webkit.org/changeset/151845>
All reviewed patches have been landed. Closing bug.
Consider merging: https://src.chromium.org/viewvc/blink?revision=153206&view=revision
(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?
(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.
(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?
(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.
Created attachment 206253 [details] Patch
Comment on attachment 206253 [details] Patch Clearing flags on attachment: 206253 Committed r152459: <http://trac.webkit.org/changeset/152459>
Why was test case not added? Did you try using test.mp4 instead of test.ogv?
(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
(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.
(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.