Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(16)

Issue 17002002: TextTrackCue should check for non-NULL cue list pointer (Closed)

Created:
6 years, 5 months ago by Matthew Heaney (Chromium)
Modified:
6 years, 4 months ago
CC:
blink-reviews, eae+blinkwatch, adamk+blink_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

TextTrackCue should check for non-NULL cue list pointer The member function TextTrackCue::cueIndex returns the index for a cue item in a cue list (the result of which is used by for sorting purposes). That function is implemented by first calling the track object to get its cues, and then calling the cue list object to get the index of the cue item. 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. The fix is for the text track cue object to check whether the cue list is non-NULL, and only then to get the index of the cue item in the list. Otherwise, if the cue list is NULL, then the standard nonce value ("invalid index") is returned as the index value (as there is no meaningful index value for the cue item of a non-existent cue list). There are no adverse consequences, however, because the index value is only used for sorting, and "invalid index" can indeed be used to determine sort order. BUG=230708 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153206

Patch Set 1 #

Total comments: 2

Patch Set 2 : addressed andrew's comments #

Patch Set 3 : added webkit layout test #

Total comments: 13

Patch Set 4 : incorporated aaron's comments #

Total comments: 2

Patch Set 5 : incorporated more comments from aaron #

Unified diffs Side-by-side diffs Delta from patch set Stats (+30 lines, -3 lines) Patch
A LayoutTests/media/track/track-disabled.html View 1 2 3 4 1 chunk +24 lines, -0 lines 0 comments Download
A + LayoutTests/media/track/track-disabled-expected.txt View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/track/TextTrackCue.cpp View 1 2 1 chunk +5 lines, -2 lines 0 comments Download

Messages

Total messages: 31 (0 generated)
Matthew Heaney (Chromium)
I found a case in which the text track cue object dereferences the track's cue ...
6 years, 5 months ago (2013-06-13 23:21:25 UTC) #1
scherkus (not reviewing)
is it possible to get a test written for this case? https://chromiumcodereview.appspot.com/17002002/diff/1/Source/core/html/track/TextTrackCue.cpp File Source/core/html/track/TextTrackCue.cpp (right): ...
6 years, 5 months ago (2013-06-14 00:27:59 UTC) #2
Matthew Heaney (Chromium)
On 2013/06/14 00:27:59, scherkus wrote: > is it possible to get a test written for ...
6 years, 5 months ago (2013-06-14 01:59:34 UTC) #3
acolwell GONE FROM CHROMIUM
On 2013/06/14 01:59:34, Matthew Heaney (Chromium) wrote: > On 2013/06/14 00:27:59, scherkus wrote: > > ...
6 years, 5 months ago (2013-06-14 17:25:04 UTC) #4
Matthew Heaney (Chromium)
On 2013/06/14 17:25:04, acolwell wrote: > > Look in third_party/WebKit/LayoutTests/media/track/ for examples of text track ...
6 years, 4 months ago (2013-06-21 18:56:29 UTC) #5
acolwell GONE FROM CHROMIUM
On 2013/06/21 18:56:29, Matthew Heaney (Chromium) wrote: > On 2013/06/14 17:25:04, acolwell wrote: > > ...
6 years, 4 months ago (2013-06-21 19:11:24 UTC) #6
Matthew Heaney (Chromium)
I made a style change to the predicate, to match what's done in another part ...
6 years, 4 months ago (2013-06-21 19:27:39 UTC) #7
Matthew Heaney (Chromium)
On 2013/06/21 19:11:24, acolwell wrote: > The test should take less than 2 seconds to ...
6 years, 4 months ago (2013-06-21 19:29:07 UTC) #8
acolwell GONE FROM CHROMIUM
On 2013/06/21 19:29:07, Matthew Heaney (Chromium) wrote: > On 2013/06/21 19:11:24, acolwell wrote: > > ...
6 years, 4 months ago (2013-06-21 19:32:50 UTC) #9
Matthew Heaney (Chromium)
On 2013/06/21 19:32:50, acolwell wrote: > On 2013/06/21 19:29:07, Matthew Heaney (Chromium) wrote: > > ...
6 years, 4 months ago (2013-06-21 19:38:53 UTC) #10
acolwell GONE FROM CHROMIUM
On 2013/06/21 19:38:53, Matthew Heaney (Chromium) wrote: > On 2013/06/21 19:32:50, acolwell wrote: > > ...
6 years, 4 months ago (2013-06-21 19:44:07 UTC) #11
vcarbune.chromium
On 2013/06/21 19:44:07, acolwell wrote: > On 2013/06/21 19:38:53, Matthew Heaney (Chromium) wrote: > > ...
6 years, 4 months ago (2013-06-21 20:45:15 UTC) #12
Matthew Heaney (Chromium)
On 2013/06/21 20:45:15, vcarbune.chromium wrote: > > Exactly, you can just listen to some event ...
6 years, 4 months ago (2013-06-21 20:59:01 UTC) #13
Matthew Heaney (Chromium)
On 2013/06/21 20:45:15, vcarbune.chromium wrote: > > TextTrackCue::cueIndex() is called only upon event dispatching and ...
6 years, 4 months ago (2013-06-21 22:13:38 UTC) #14
Matthew Heaney (Chromium)
On 2013/06/21 22:13:38, Matthew Heaney (Chromium) wrote: > > So maybe you need the Media ...
6 years, 4 months ago (2013-06-21 22:34:17 UTC) #15
Matthew Heaney (Chromium)
On 2013/06/21 22:34:17, Matthew Heaney (Chromium) wrote: > > It appears that if you set ...
6 years, 4 months ago (2013-06-23 00:49:11 UTC) #16
Matthew Heaney (Chromium)
On 2013/06/23 00:49:11, Matthew Heaney (Chromium) wrote: > > I have a normal page (the ...
6 years, 4 months ago (2013-06-23 01:35:04 UTC) #17
abarth-chromium
On 2013/06/23 00:49:11, Matthew Heaney (Chromium) wrote: > Does run_webkit_tests.sh always imply that content_shell is ...
6 years, 4 months ago (2013-06-23 03:13:20 UTC) #18
Matthew Heaney (Chromium)
I added a webkit layout test that would trigger an exception, were the NULL-valued cues ...
6 years, 4 months ago (2013-06-27 20:02:53 UTC) #19
acolwell GONE FROM CHROMIUM
https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/track/track-disabled-expected.html File LayoutTests/media/track/track-disabled-expected.html (right): https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/track/track-disabled-expected.html#newcode1 LayoutTests/media/track/track-disabled-expected.html:1: EVENT(loadstart) nit: I'm pretty sure this is supposed to ...
6 years, 4 months ago (2013-06-27 21:45:20 UTC) #20
Matthew Heaney (Chromium)
Incorporated Aaron's comments. (Thank you, Aaron!) https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/track/track-disabled-expected.html File LayoutTests/media/track/track-disabled-expected.html (right): https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/track/track-disabled-expected.html#newcode1 LayoutTests/media/track/track-disabled-expected.html:1: EVENT(loadstart) On 2013/06/27 ...
6 years, 4 months ago (2013-06-27 23:25:10 UTC) #21
acolwell GONE FROM CHROMIUM
lgtm % nits https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/track/track-disabled.html File LayoutTests/media/track/track-disabled.html (right): https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/track/track-disabled.html#newcode4 LayoutTests/media/track/track-disabled.html:4: <script src=../media-file.js></script> On 2013/06/27 23:25:10, Matthew ...
6 years, 4 months ago (2013-06-27 23:51:52 UTC) #22
Matthew Heaney (Chromium)
Took care of the indentation issue. https://chromiumcodereview.appspot.com/17002002/diff/33001/LayoutTests/media/track/track-disabled.html File LayoutTests/media/track/track-disabled.html (right): https://chromiumcodereview.appspot.com/17002002/diff/33001/LayoutTests/media/track/track-disabled.html#newcode4 LayoutTests/media/track/track-disabled.html:4: <script src=../media-file.js></script> On ...
6 years, 4 months ago (2013-06-28 00:05:59 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/17002002/38001
6 years, 4 months ago (2013-06-28 00:07:33 UTC) #24
commit-bot: I haz the power
Retried try job too often on blink_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_presubmit&number=2554
6 years, 4 months ago (2013-06-28 00:55:23 UTC) #25
abarth-chromium
lgtm
6 years, 4 months ago (2013-06-28 03:03:13 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/17002002/38001
6 years, 4 months ago (2013-06-28 03:03:23 UTC) #27
commit-bot: I haz the power
Change committed as 153206
6 years, 4 months ago (2013-06-28 05:00:07 UTC) #28
Eric Carlson
On 2013/06/27 20:02:53, Matthew Heaney (Chromium) wrote: > I added a webkit layout test that ...
6 years, 4 months ago (2013-07-05 21:34:59 UTC) #29
acolwell GONE FROM CHROMIUM
On 2013/07/05 21:34:59, carlson.eric wrote: > On 2013/06/27 20:02:53, Matthew Heaney (Chromium) wrote: > > ...
6 years, 4 months ago (2013-07-08 16:30:09 UTC) #30
acolwell GONE FROM CHROMIUM
6 years, 4 months ago (2013-07-08 17:52:41 UTC) #31
Message was sent while issue was closed.
On 2013/07/08 16:30:09, acolwell wrote:
> On 2013/07/05 21:34:59, carlson.eric wrote:
> > On 2013/06/27 20:02:53, Matthew Heaney (Chromium) wrote:
> > > I added a webkit layout test that would trigger an exception, were the
> > > NULL-valued cues list pointer not handled.
> > 
> > Why does this:
> > 
> > waitForEvent('loadstart', function ()
> > {
> >     var v = document.getElementById('vid');
> >     v.textTracks[0].mode = "disabled";
> > });
> > 
> > force TextTrackCue::cueIndex to be called when track()->cues() is NULL?
> > 
> > TextTrackCue::cueIndex is only called when
> > HTMLMediaElement::updateActiveTextTrackCues is sorting cues for display, so
> the
> > cue is in HTMLMediaElement::m_cueTree. A cue is only placed in
> > HTMLMediaElement's cue tree by a track, when track()->cues() is clearly not
> > NULL, so I believe that this crash indicates that a cue has been removed
from
> > its track but not from HTMLMediaElement::m_cueTree. 
> > 
> > If I am correct, this crash is/was an indication of a problem that this
change
> > does not fix.
> 
> Hi Eric,
> Thanks for looking into this. I'll take a closer look today and get back to
you.
> Aaron

Hi Eric,

You were right that this specific fix did not address the real underlying
problem. I've uploaded a new patch (https://codereview.chromium.org/18856005/)
that addresses the real problem. LoadableTextTrack was adding cues to
HTMLMediaElement w/o actually checking to see if the track was disabled or not.
This allowed the cues to sneak in w/o going through the normal checks in
HTMLMediaElement::textTrackModeChanged() because the mode is changed before the
cues have finished loading.

Please comment on the new patch if you believe changes should be made to it. 

Thanks again for pointing this out to me.

Powered by Google App Engine
This is Rietveld 408576698