TextTrackCue should check for non-NULL cue list pointer

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

[Matthew Heaney (Chromium), 2013-06-13 23:21:25]

I found a case in which the text track cue object dereferences the track's cue
list pointer when its value is NULL. This causes Chrome to crash.  Can you
review the patch?  Thanks, Matt

[scherkus (not reviewing), 2013-06-14 00:27:59]

is it possible to get a test written for this case?

https://chromiumcodereview.appspot.com/17002002/diff/1/Source/core/html/track...
File Source/core/html/track/TextTrackCue.cpp (right):

https://chromiumcodereview.appspot.com/17002002/diff/1/Source/core/html/track...
Source/core/html/track/TextTrackCue.cpp:484: if (TextTrackCueList* cues =
track()->cues())
I'm not sure if this is common Blink style, but it seems we could write this as:

if (m_cueIndex == invalidCueIndex && track()->cues())
    m_cueIndex = track()->cues()->getCueIndex(this);

[Matthew Heaney (Chromium), 2013-06-14 01:59:34]

On 2013/06/14 00:27:59, scherkus wrote:
> is it possible to get a test written for this case?

Did you have in mind a unit test, or some other kind of test?  (I haven't
written anything other than a unit test, so I might need a few hints about how
to proceed.)

[acolwell GONE FROM CHROMIUM, 2013-06-14 17:25:04]

On 2013/06/14 01:59:34, Matthew Heaney (Chromium) wrote:
> On 2013/06/14 00:27:59, scherkus wrote:
> > is it possible to get a test written for this case?
> 
> Did you have in mind a unit test, or some other kind of test?  (I haven't
> written anything other than a unit test, so I might need a few hints about how
> to proceed.)

Look in third_party/WebKit/LayoutTests/media/track/ for examples of text track
related tests.
You can run these tests by building the content_shell target and then running
the following command-line:

./webkit/tools/layout_tests/run_webkit_tests.sh --debug --no-retry-failures
media/track/

[Matthew Heaney (Chromium), 2013-06-21 18:56:29]

On 2013/06/14 17:25:04, acolwell wrote:
> 
> Look in third_party/WebKit/LayoutTests/media/track/ for examples of text track
> related tests.

I've been using a 30 sec big buck bunny clip to detect the error.  Are there any
constraints on the duration or size of the video file?

[acolwell GONE FROM CHROMIUM, 2013-06-21 19:11:24]

On 2013/06/21 18:56:29, Matthew Heaney (Chromium) wrote:
> On 2013/06/14 17:25:04, acolwell wrote:
> > 
> > Look in third_party/WebKit/LayoutTests/media/track/ for examples of text
track
> > related tests.
> 
> I've been using a 30 sec big buck bunny clip to detect the error.  Are there
any
> constraints on the duration or size of the video file?

If possible, you should reencode one of the existing test files and just add an
inband text track so that we don't have to worry about any rights issues with
using a clip like that one. The test should not be longer than 10 seconds or so
and should be rather small in size. The test should take less than 2 seconds to
run.

[Matthew Heaney (Chromium), 2013-06-21 19:27:39]

I made a style change to the predicate, to match what's done in another part of
Blink.

https://chromiumcodereview.appspot.com/17002002/diff/1/Source/core/html/track...
File Source/core/html/track/TextTrackCue.cpp (right):

https://chromiumcodereview.appspot.com/17002002/diff/1/Source/core/html/track...
Source/core/html/track/TextTrackCue.cpp:484: if (TextTrackCueList* cues =
track()->cues())
On 2013/06/14 00:28:00, scherkus wrote:
> I'm not sure if this is common Blink style, but it seems we could write this
as:

I modified the code to match the coding style from a similar predicate in
another part in Blink (HTMLMediaElement::removeTrack()).

[Matthew Heaney (Chromium), 2013-06-21 19:29:07]

On 2013/06/21 19:11:24, acolwell wrote:
> The test should take less than 2 seconds to run.

So do layout tests run faster than wall clock time?

[acolwell GONE FROM CHROMIUM, 2013-06-21 19:32:50]

On 2013/06/21 19:29:07, Matthew Heaney (Chromium) wrote:
> On 2013/06/21 19:11:24, acolwell wrote:
> > The test should take less than 2 seconds to run.
> 
> So do layout tests run faster than wall clock time?

no.

[Matthew Heaney (Chromium), 2013-06-21 19:38:53]

On 2013/06/21 19:32:50, acolwell wrote:
> On 2013/06/21 19:29:07, Matthew Heaney (Chromium) wrote:
> > On 2013/06/21 19:11:24, acolwell wrote:
> > > The test should take less than 2 seconds to run.
> > 
> > So do layout tests run faster than wall clock time?
> 
> no.

So how does a layout test using a 10s video clip run for less than 2 seconds?

[acolwell GONE FROM CHROMIUM, 2013-06-21 19:44:07]

On 2013/06/21 19:38:53, Matthew Heaney (Chromium) wrote:
> On 2013/06/21 19:32:50, acolwell wrote:
> > On 2013/06/21 19:29:07, Matthew Heaney (Chromium) wrote:
> > > On 2013/06/21 19:11:24, acolwell wrote:
> > > > The test should take less than 2 seconds to run.
> > > 
> > > So do layout tests run faster than wall clock time?
> > 
> > no.
> 
> So how does a layout test using a 10s video clip run for less than 2 seconds?

Have you looked at any of the existing tests? There should be plenty of examples
in there to accomplish what you need to do. For example you don't have to play
the whole clip to terminate the test.

[vcarbune.chromium, 2013-06-21 20:45:15]

On 2013/06/21 19:44:07, acolwell wrote:
> On 2013/06/21 19:38:53, Matthew Heaney (Chromium) wrote:
> > On 2013/06/21 19:32:50, acolwell wrote:
> > > On 2013/06/21 19:29:07, Matthew Heaney (Chromium) wrote:
> > > > On 2013/06/21 19:11:24, acolwell wrote:
> > > > > The test should take less than 2 seconds to run.
> > > > 
> > > > So do layout tests run faster than wall clock time?
> > > 
> > > no.
> > 
> > So how does a layout test using a 10s video clip run for less than 2
seconds?
> 
> Have you looked at any of the existing tests? There should be plenty of
examples
> in there to accomplish what you need to do. For example you don't have to play
> the whole clip to terminate the test.

Exactly, you can just listen to some event and trigger the condition that
crashes
the browser. If it doesn't crash, it passed.

Here's a specific test-case you start from:
https://chromium.googlesource.com/chromium/blink/+/f4200a0093b3d9376f70396161...


Also, on a different side-note, while the fix in the code is definitely
appropriate and fixes the crash, I think there might be another underlying
problem, but I haven't been able to reproduce locally.

TextTrackCue::cueIndex() is called only upon event dispatching and I don't see
any good reason for which such an event should be dispatched in the case where
the owner track has cues() == NULL

[Matthew Heaney (Chromium), 2013-06-21 20:59:01]

On 2013/06/21 20:45:15, vcarbune.chromium wrote:
> 
> Exactly, you can just listen to some event and trigger the condition that
> crashes the browser. If it doesn't crash, it passed.

That's what my question was about.  If we fix the bug and then the layout test
"doesn't crash", I was afraid that the test would then run for the duration of
the video clip.  I wasn't understanding how the test could run less time than
that (unless it failed).

[Matthew Heaney (Chromium), 2013-06-21 22:13:38]

On 2013/06/21 20:45:15, vcarbune.chromium wrote:
> 
> TextTrackCue::cueIndex() is called only upon event dispatching and I don't see
> any good reason for which such an event should be dispatched in the case where
> the owner track has cues() == NULL

I discovered this bug when I was testing using the Media Source API to play a
video containing an inband text track.  In that case, the track's mode is
"disabled" by default, and so the cue list pointer returned by cues() was NULL.

I haven't been able to get the crash to trigger when using the video + track
tags, even if I set the mode to "disabled" in the 'loadedmetadata' event
listener.

So maybe you need the Media Source API to see the problem.

[Matthew Heaney (Chromium), 2013-06-21 22:34:17]

On 2013/06/21 22:13:38, Matthew Heaney (Chromium) wrote:
> 
> So maybe you need the Media Source API to see the problem.

It appears that if you set the text track mode to "disabled" in the 'loadstart'
event, then you're able to reproduce the bug.  Perhaps the 'loadedmetadata'
event arrives too late.

[Matthew Heaney (Chromium), 2013-06-23 00:49:11]

On 2013/06/21 22:34:17, Matthew Heaney (Chromium) wrote:
> 
> It appears that if you set the text track mode to "disabled" in the
'loadstart'
> event, then you're able to reproduce the bug.

I have a normal page (the file text_track_crash.htm, hosted at google.com) that
crashes chrome.  It also crashes content_shell.exe, but only if you run it
without the --dump_render_tree option.  (This doesn't mean that you can't
observe the crash while using the dump_render_tree option, only that I haven't
discovered such a way.)

Does run_webkit_tests.sh always imply that content_shell is run using the
--dump_render_tree option?

[Matthew Heaney (Chromium), 2013-06-23 01:35:04]

On 2013/06/23 00:49:11, Matthew Heaney (Chromium) wrote:
> 
> I have a normal page (the file text_track_crash.htm, hosted at
http://google.com) that
> crashes chrome.

I should also point out that this crash happens in released versions of chrome
as well.  It's not a bug that was introduced as a result of any recent changes.

[abarth-chromium, 2013-06-23 03:13:20]

On 2013/06/23 00:49:11, Matthew Heaney (Chromium) wrote:
> Does run_webkit_tests.sh always imply that content_shell is run using the
> --dump_render_tree option?

Yes, assuming you mean --dump-render-tree.

[Matthew Heaney (Chromium), 2013-06-27 20:02:53]

I added a webkit layout test that would trigger an exception, were the
NULL-valued cues list pointer not handled.

[acolwell GONE FROM CHROMIUM, 2013-06-27 21:45:20]

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
File LayoutTests/media/track/track-disabled-expected.html (right):

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
LayoutTests/media/track/track-disabled-expected.html:1: EVENT(loadstart)
nit: I'm pretty sure this is supposed to have a txt extension since this is a
text dump test.

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
File LayoutTests/media/track/track-disabled.html (right):

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
LayoutTests/media/track/track-disabled.html:4: <script
src=../media-file.js></script>
nit: script tags should be inside <head> and tags should have 4 space
indentation.

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
LayoutTests/media/track/track-disabled.html:10: testRunner.dumpAsText();
nit: Needs 4 space indent.

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
LayoutTests/media/track/track-disabled.html:12: waitForEvent('loadstart',
nit: The following is more inline with WebKit style.
waitForEvent('loadstart', function ()
{
    var v = document.getElementById('vid');
    v.textTracks[0].mode = "disabled";
});

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
LayoutTests/media/track/track-disabled.html:18: 
You should probably waitForEventAndEnd('loadedmetadata') here so that the test
ends when we reach the HAVE_METADATA state. I'm assuming the crash happens
sometime after the loadstart event handler returns.

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
LayoutTests/media/track/track-disabled.html:22: <video id="vid"
src="../content/test.mp4" autoplay controls>
nit: I'm pretty sure you need to use test.ogv here since Chromium builds don't
have MP4 support turned on by default.

[Matthew Heaney (Chromium), 2013-06-27 23:25:10]

Incorporated Aaron's comments.  (Thank you, Aaron!)

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
File LayoutTests/media/track/track-disabled-expected.html (right):

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
LayoutTests/media/track/track-disabled-expected.html:1: EVENT(loadstart)
On 2013/06/27 21:45:20, acolwell wrote:
> nit: I'm pretty sure this is supposed to have a txt extension since this is a
> text dump test.

Oh, curse you, git mv!  (fixed)

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
File LayoutTests/media/track/track-disabled.html (right):

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
LayoutTests/media/track/track-disabled.html:4: <script
src=../media-file.js></script>
On 2013/06/27 21:45:20, acolwell wrote:
> nit: script tags should be inside <head> and tags should have 4 space
> indentation.

OK, thanks for the info.  I was trying to infer the style rules, but many of
these test files seem to use different conventions.

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
LayoutTests/media/track/track-disabled.html:10: testRunner.dumpAsText();
On 2013/06/27 21:45:20, acolwell wrote:
> nit: Needs 4 space indent.

Done.

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
LayoutTests/media/track/track-disabled.html:12: waitForEvent('loadstart',
On 2013/06/27 21:45:20, acolwell wrote:
> nit: The following is more inline with WebKit style.
> waitForEvent('loadstart', function ()
> {
>     var v = document.getElementById('vid');
>     v.textTracks[0].mode = "disabled";
> });

Done.

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
LayoutTests/media/track/track-disabled.html:18: 
On 2013/06/27 21:45:20, acolwell wrote:
> You should probably waitForEventAndEnd('loadedmetadata') here so that the test
> ends when we reach the HAVE_METADATA state. I'm assuming the crash happens
> sometime after the loadstart event handler returns.

Done.

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
LayoutTests/media/track/track-disabled.html:22: <video id="vid"
src="../content/test.mp4" autoplay controls>
On 2013/06/27 21:45:20, acolwell wrote:
> nit: I'm pretty sure you need to use test.ogv here since Chromium builds don't
> have MP4 support turned on by default.

Done.

[acolwell GONE FROM CHROMIUM, 2013-06-27 23:51:52]

lgtm % nits

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
File LayoutTests/media/track/track-disabled.html (right):

https://chromiumcodereview.appspot.com/17002002/diff/25001/LayoutTests/media/...
LayoutTests/media/track/track-disabled.html:4: <script
src=../media-file.js></script>
On 2013/06/27 23:25:10, Matthew Heaney (Chromium) wrote:
> On 2013/06/27 21:45:20, acolwell wrote:
> > nit: script tags should be inside <head> and tags should have 4 space
> > indentation.
> 
> OK, thanks for the info.  I was trying to infer the style rules, but many of
> these test files seem to use different conventions.

Yeah. Unfortunately the style isn't consistent across a lot of the media
LayoutTests because of different bars set by different reviewers in the WebKit
days.

https://chromiumcodereview.appspot.com/17002002/diff/33001/LayoutTests/media/...
File LayoutTests/media/track/track-disabled.html (right):

https://chromiumcodereview.appspot.com/17002002/diff/33001/LayoutTests/media/...
LayoutTests/media/track/track-disabled.html:4: <script
src=../media-file.js></script>
nit: add 4 more spaces here and to everything else in the <head>. It's 4 spaces
per level. Sorry if I wasn't clear about that.

also please add quotes around the script paths.

[Matthew Heaney (Chromium), 2013-06-28 00:05:59]

Took care of the indentation issue.

https://chromiumcodereview.appspot.com/17002002/diff/33001/LayoutTests/media/...
File LayoutTests/media/track/track-disabled.html (right):

https://chromiumcodereview.appspot.com/17002002/diff/33001/LayoutTests/media/...
LayoutTests/media/track/track-disabled.html:4: <script
src=../media-file.js></script>
On 2013/06/27 23:51:53, acolwell wrote:
> nit: add 4 more spaces here and to everything else in the <head>. It's 4
spaces
> per level. Sorry if I wasn't clear about that.
> 
> also please add quotes around the script paths.

Done.

[commit-bot: I haz the power, 2013-06-28 00:07:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/17002002/3...

[commit-bot: I haz the power, 2013-06-28 00:55:23]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[abarth-chromium, 2013-06-28 03:03:13]

lgtm

[commit-bot: I haz the power, 2013-06-28 03:03:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/matthewjheaney@chromium.org/17002002/3...

[commit-bot: I haz the power, 2013-06-28 05:00:07]

Change committed as 153206

[Eric Carlson, 2013-07-05 21:34:59]

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.

[acolwell GONE FROM CHROMIUM, 2013-07-08 16:30:09]

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

[acolwell GONE FROM CHROMIUM, 2013-07-08 17:52:41]

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.