Bug 94651 – ASSERT reached when TextTrack.mode is set to DISABLED

• Component: Media
• Status: RESOLVED FIXED

[Anna Cavender]

I believe this is because cues are stuck in the render tree when they become DISABLED.  They should be first removed from the render tree and then removed from the activeCues list.

[Anna Cavender]

Created attachment 159810 [details]
Patch

I will work on a test for this coming soon.

[Abhishek Arya]

What does the crash stack look like ? Is it a use after free or just a null crash. Null crashes are not security bugs.

[Radar WebKit Bug Importer]

<rdar://problem/12147493>

[Eric Seidel (no email)]

Comment on attachment 159810 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        No new tests - coming soon.

In this patch?

[Anna Cavender]

I now have a better understanding of this bug, and a fix!

Turns out that when a TextTrack mode is set to 'showing' or 'hidden', the TextTrack's cues are added to HTMLMediaElement's cueTree regardless of whether they might already have been added previously, creating duplicates.  When the TextTrack's mode changes to 'disabled' textTrackModeChanged() removes the cues, but not the duplicates.  Those cues are then still considered to be active (and they continue have references in m_currentlyActiveCues) and then the ASSERT(cue->isActive()); catches the problem in MediaControlTextTrackContainerElement::updateDisplay().

This was revealed with the new CC button because as the user toggles the button, default tracks start out showingByDefault (i.e. showing), then become hidden (and are added to the cueTree again!), then become showing (added to the cueTree a third time!), and then become disabled, resulting in problems.

The fact that default tracks follow this strange path (showingByDefault->hidden->showing->disabled) is a bug in the spec.

[Anna Cavender]

Created attachment 162355 [details]
Patch

[Victor Carbune]

Comment on attachment 162355 [details]
Patch

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

I think this is a great fix! There's also a high probability that some multiple rendering issues might have been caused at some point by this as well (but the code flow changes probably removed them).

> Source/WebCore/ChangeLog:17
> +        (WebCore::HTMLMediaElement::textTrackRemoveCues): Mark the track as having

Nit: I think you meant mark a track as *not* having.

[Eric Carlson]

Comment on attachment 162355 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:1334
> +    if (!track->hasCuesInTree()) {
> +        beginIgnoringTrackDisplayUpdateRequests();
> +        for (size_t i = 0; i < cues->length(); ++i)
> +            textTrackAddCue(cues->item(i)->track(), cues->item(i));
> +        track->setHasCuesInTree(true);
> +        endIgnoringTrackDisplayUpdateRequests();
> +    }

Won't this prevent *any* cues from being added to the cue tree once this runs? This will be a problem when a script creates a cue and adds it with addCue(), when the loader delivers a track's cue data in more than one chunk, etc.

Would it work to have textTrackAddCue() only add a cue if it was not already in the tree?

[Anna Cavender]

(In reply to comment #8)
> (From update of attachment 162355 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162355&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:1334
> > +    if (!track->hasCuesInTree()) {
> > +        beginIgnoringTrackDisplayUpdateRequests();
> > +        for (size_t i = 0; i < cues->length(); ++i)
> > +            textTrackAddCue(cues->item(i)->track(), cues->item(i));
> > +        track->setHasCuesInTree(true);
> > +        endIgnoringTrackDisplayUpdateRequests();
> > +    }
> 
> Won't this prevent *any* cues from being added to the cue tree once this runs? This will be a problem when a script creates a cue and adds it with addCue(), when the loader delivers a track's cue data in more than one chunk, etc.

Good call.  This isn't a good idea considering batching of cues.  Will revise.
 
> Would it work to have textTrackAddCue() only add a cue if it was not already in the tree?

Definitely.  I was looking for a more efficient route than checking for each cue.  But, I think that checking the tree before each cue is added is necessary.  New patch on the way!

[Anna Cavender]

Created attachment 162473 [details]
perform check in textTrackAddCue()

[WebKit Review Bot]

Comment on attachment 162473 [details]
perform check in textTrackAddCue()

Clearing flags on attachment: 162473

Committed r127779: <http://trac.webkit.org/changeset/127779>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.