Fix HTMLMediaElement so that it doesn't add cues for disabled text tracks.

Fix HTMLMediaElement so that it doesn't add cues for disabled text tracks.

This patch reverts the change to TextTrackCue added in
https://src.chromium.org/viewvc/blink?revision=153206&view=revision
and addresses the actual underlying issue. The real problem
is that HTMLMediaElement was allowing cues from disabled
tracks to be added to  m_cueTree under certain
circumstances.  This patch adds checks to textTrackAddCues() 
and textTrackAddCue() to prevent disabled track cues from
being added.

BUG=230708
TEST=media/track/track-disabled.html & media/track/track-disabled-addcue.html LayoutTests

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=153810

[acolwell GONE FROM CHROMIUM, 2013-07-08 18:04:27]

Relevant discussion on the original CL.
https://chromiumcodereview.appspot.com/17002002/#msg31

[adamk, 2013-07-08 19:16:38]

lgtm with a comment

https://codereview.chromium.org/18856005/diff/1/Source/core/html/HTMLMediaEle...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/18856005/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:1268: ASSERT(track->mode() !=
TextTrack::disabledKeyword());
Not sure if we have any compilers that actually complain about unused arguments,
but for consistency I think this should use

ASSERT_UNUSED(track, ...);

[commit-bot: I haz the power, 2013-07-08 19:24:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/18856005/4001

[acolwell GONE FROM CHROMIUM, 2013-07-08 19:25:48]

https://codereview.chromium.org/18856005/diff/1/Source/core/html/HTMLMediaEle...
File Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/18856005/diff/1/Source/core/html/HTMLMediaEle...
Source/core/html/HTMLMediaElement.cpp:1268: ASSERT(track->mode() !=
TextTrack::disabledKeyword());
On 2013/07/08 19:16:38, adamk wrote:
> Not sure if we have any compilers that actually complain about unused
arguments,
> but for consistency I think this should use
> 
> ASSERT_UNUSED(track, ...);

Done.

[Eric Carlson, 2013-07-08 20:23:30]

On 2013/07/08 19:25:48, acolwell wrote:
>
https://codereview.chromium.org/18856005/diff/1/Source/core/html/HTMLMediaEle...
> File Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/18856005/diff/1/Source/core/html/HTMLMediaEle...
> Source/core/html/HTMLMediaElement.cpp:1268: ASSERT(track->mode() !=
> TextTrack::disabledKeyword());
> On 2013/07/08 19:16:38, adamk wrote:
> > Not sure if we have any compilers that actually complain about unused
> arguments,
> > but for consistency I think this should use
> > 
> > ASSERT_UNUSED(track, ...);
> 
> Done.

I believe this change does address the issue, but do you really want
LoadableTextTrack to decide which cues HTMLMediaElement is interested in adding?
IOW, which class should have the logic about which cues are relevant? I ask
because when a track's mode changes from "disabled",
HTMLMediaElement::textTrackModeChanged fetches the track's cues and adds them to
the tree.

[acolwell GONE FROM CHROMIUM, 2013-07-08 22:57:12]

On 2013/07/08 20:23:30, carlson.eric wrote:
> On 2013/07/08 19:25:48, acolwell wrote:
> >
>
https://codereview.chromium.org/18856005/diff/1/Source/core/html/HTMLMediaEle...
> > File Source/core/html/HTMLMediaElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/18856005/diff/1/Source/core/html/HTMLMediaEle...
> > Source/core/html/HTMLMediaElement.cpp:1268: ASSERT(track->mode() !=
> > TextTrack::disabledKeyword());
> > On 2013/07/08 19:16:38, adamk wrote:
> > > Not sure if we have any compilers that actually complain about unused
> > arguments,
> > > but for consistency I think this should use
> > > 
> > > ASSERT_UNUSED(track, ...);
> > 
> > Done.
> 
> I believe this change does address the issue, but do you really want
> LoadableTextTrack to decide which cues HTMLMediaElement is interested in
adding?
> IOW, which class should have the logic about which cues are relevant? I ask
> because when a track's mode changes from "disabled",
> HTMLMediaElement::textTrackModeChanged fetches the track's cues and adds them
to
> the tree.

I see what you are saying. Unfortunately it doesn't look like the code is
consistent about letting HTMLMediaElement always decide. For example the
TextTrack::setMode("disabled") code path assumes that textTrackModeChanged()
added the cues at some point and calls textTrackRemoveCues() before calling
textTrackModeChanged(). This seems to imply that the decision about what cues
HTMLMediaElement cares about is shared between these 2 classes. My guess is that
this is unintentional and the textTrackRemoveCues() call should eventually be
moved inside HTMLMediaElement::textTrackModeChanged(). I agree that adding
LoadableTextTrack as a third conspirator is not heading in the right direction.
I'll move the logic into HTMLMediaElement.

Also after further inspection, I believe there are more problems with disabled
tracks. I haven't fully tested it yet, but I believe the textTrackAddCue() also
needs the "disabled" guard to prevent JS from triggering the same crash by
adding cues to disabled tracks. I'll keep investigating and add another
LayoutTest if I am able to repro a crash via this alternate vector.

[acolwell GONE FROM CHROMIUM, 2013-07-09 18:33:42]

PTAL. I've addressed Eric's concerns and added another test to verify a crash
that was caused by JavaScript adding cues to disabled tracks.

[adamk, 2013-07-09 19:03:23]

lgtm

Looks much nicer! Thanks, Eric, for the extra review.

[commit-bot: I haz the power, 2013-07-09 19:05:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/18856005/27001

[commit-bot: I haz the power, 2013-07-09 19:59:18]

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

[commit-bot: I haz the power, 2013-07-09 20:14:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/acolwell@chromium.org/18856005/27001

[commit-bot: I haz the power, 2013-07-09 20:29:30]

Change committed as 153810