Bug 94651

Created attachment 159810 [details] Patch I will work on a test for this coming soon.

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

<rdar://problem/12147493>

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?

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.

Created attachment 162355 [details] Patch

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.

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?

(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!

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

Comment on attachment 162473 [details] perform check in textTrackAddCue() Clearing flags on attachment: 162473 Committed r127779: <http://trac.webkit.org/changeset/127779>

All reviewed patches have been landed. Closing bug.