Crash in TextTrack::trackIndexRelativeToRenderedTracks()
authoreric.carlson@apple.com <eric.carlson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 20 Dec 2012 05:50:34 +0000 (05:50 +0000)
committereric.carlson@apple.com <eric.carlson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 20 Dec 2012 05:50:34 +0000 (05:50 +0000)
https://bugs.webkit.org/show_bug.cgi?id=105371

Reviewed by Simon Fraser.

Source/WebCore:

Add an RAII object to manage text track update blocking, use it to always process the
current cues to ensure that cues from a track that is deleted are removed from the
shadow DOM before the next layout.

No new tests, this fixes a crash in media/video-controls-captions-trackmenu.html.

* html/HTMLMediaElement.cpp:
(WebCore::TrackDisplayUpdateScope::TrackDisplayUpdateScope): New, call beginIgnoringTrackDisplayUpdateRequests.
(WebCore::TrackDisplayUpdateScope::~TrackDisplayUpdateScope): New, call endIgnoringTrackDisplayUpdateRequests.
(WebCore::HTMLMediaElement::beginIgnoringTrackDisplayUpdateRequests):
(WebCore::HTMLMediaElement::endIgnoringTrackDisplayUpdateRequests): Call updateActiveTextTrackCues
    when the ignore count reaches zero.
(WebCore::HTMLMediaElement::textTrackAddCues): Use TrackDisplayUpdateScope instead of calling
    beginIgnoringTrackDisplayUpdateRequests and endIgnoringTrackDisplayUpdateRequests directly.
(WebCore::HTMLMediaElement::textTrackRemoveCues): Ditto.
(WebCore::HTMLMediaElement::removeTrack): Ditto.
(WebCore::HTMLMediaElement::removeAllInbandTracks): Ditto.
(WebCore::HTMLMediaElement::didRemoveTrack): Ditto. Call removeTrack.
* html/HTMLMediaElement.h: Declare TrackDisplayUpdateScope as a friend of HTMLMediaElement so it
    can call protected methods.

LayoutTests:

* platform/mac/TestExpectations: Unskip video-controls-captions-trackmenu.html.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@138224 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/platform/mac/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/html/HTMLMediaElement.cpp
Source/WebCore/html/HTMLMediaElement.h

index 28f7d12..5835413 100644 (file)
@@ -1,5 +1,14 @@
 2012-12-19  Eric Carlson  <eric.carlson@apple.com>
 
+        Crash in TextTrack::trackIndexRelativeToRenderedTracks()
+        https://bugs.webkit.org/show_bug.cgi?id=105371
+
+        Reviewed by Simon Fraser.
+
+        * platform/mac/TestExpectations: Unskip video-controls-captions-trackmenu.html.
+
+2012-12-19  Eric Carlson  <eric.carlson@apple.com>
+
         Update video-controls-captions-trackmenu.html
         https://bugs.webkit.org/show_bug.cgi?id=105455
 
index 499d7c1..eec59ab 100644 (file)
@@ -473,9 +473,6 @@ media/track/track-cue-rendering-snap-to-lines-not-set.html
 media/track/track-cue-rendering-vertical.html
 media/track/track-webvtt-tc028-unsupported-markup.html
 
-# Asserts: https://bugs.webkit.org/show_bug.cgi?id=105371
-[ Debug ] media/video-controls-captions-trackmenu.html
-
 # Opera-submitted tests to W3C for <track>, a lot of failures still.
 # Opera-submitted tests to W3C for <track>, a lot of failures still.
 webkit.org/b/103926 media/track/opera/idl/media-idl-tests.html [ Skip ]
index 3969fbf..87757a5 100644 (file)
@@ -1,3 +1,31 @@
+2012-12-19  Eric Carlson  <eric.carlson@apple.com>
+
+        Crash in TextTrack::trackIndexRelativeToRenderedTracks()
+        https://bugs.webkit.org/show_bug.cgi?id=105371
+
+        Reviewed by Simon Fraser.
+
+        Add an RAII object to manage text track update blocking, use it to always process the 
+        current cues to ensure that cues from a track that is deleted are removed from the 
+        shadow DOM before the next layout.
+
+        No new tests, this fixes a crash in media/video-controls-captions-trackmenu.html.
+
+        * html/HTMLMediaElement.cpp:
+        (WebCore::TrackDisplayUpdateScope::TrackDisplayUpdateScope): New, call beginIgnoringTrackDisplayUpdateRequests.
+        (WebCore::TrackDisplayUpdateScope::~TrackDisplayUpdateScope): New, call endIgnoringTrackDisplayUpdateRequests.
+        (WebCore::HTMLMediaElement::beginIgnoringTrackDisplayUpdateRequests):
+        (WebCore::HTMLMediaElement::endIgnoringTrackDisplayUpdateRequests): Call updateActiveTextTrackCues
+            when the ignore count reaches zero.
+        (WebCore::HTMLMediaElement::textTrackAddCues): Use TrackDisplayUpdateScope instead of calling 
+            beginIgnoringTrackDisplayUpdateRequests and endIgnoringTrackDisplayUpdateRequests directly.
+        (WebCore::HTMLMediaElement::textTrackRemoveCues): Ditto.
+        (WebCore::HTMLMediaElement::removeTrack): Ditto.
+        (WebCore::HTMLMediaElement::removeAllInbandTracks): Ditto.
+        (WebCore::HTMLMediaElement::didRemoveTrack): Ditto. Call removeTrack.
+        * html/HTMLMediaElement.h: Declare TrackDisplayUpdateScope as a friend of HTMLMediaElement so it
+            can call protected methods.
+
 2012-12-19  Nate Chapin  <japhet@chromium.org>
 
         REGRESSION(r137607): resource load client callbacks are not called for the main resource when loading HTML string
index c0405c2..4e9125b 100644 (file)
@@ -202,6 +202,25 @@ static ExceptionCode exceptionCodeForMediaKeyException(MediaPlayer::MediaKeyExce
 }
 #endif
 
+#if ENABLE(VIDEO_TRACK)
+class TrackDisplayUpdateScope {
+public:
+    TrackDisplayUpdateScope(HTMLMediaElement* mediaElement)
+    {
+        m_mediaElement = mediaElement;
+        m_mediaElement->beginIgnoringTrackDisplayUpdateRequests();
+    }
+    ~TrackDisplayUpdateScope()
+    {
+        ASSERT(m_mediaElement);
+        m_mediaElement->endIgnoringTrackDisplayUpdateRequests();
+    }
+    
+private:
+    HTMLMediaElement* m_mediaElement;
+};
+#endif
+
 HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName, Document* document, bool createdByParser)
     : HTMLElement(tagName, document)
     , ActiveDOMObject(document, this)
@@ -1352,22 +1371,31 @@ void HTMLMediaElement::textTrackKindChanged(TextTrack* track)
         track->setMode(TextTrack::hiddenKeyword());
 }
 
+void HTMLMediaElement::beginIgnoringTrackDisplayUpdateRequests()
+{
+    ++m_ignoreTrackDisplayUpdate;
+}
+
+void HTMLMediaElement::endIgnoringTrackDisplayUpdateRequests()
+{
+    ASSERT(m_ignoreTrackDisplayUpdate);
+    --m_ignoreTrackDisplayUpdate;
+    if (!m_ignoreTrackDisplayUpdate)
+        updateActiveTextTrackCues(currentTime());
+}
+
 void HTMLMediaElement::textTrackAddCues(TextTrack*, const TextTrackCueList* cues) 
 {
-    beginIgnoringTrackDisplayUpdateRequests();
+    TrackDisplayUpdateScope(this);
     for (size_t i = 0; i < cues->length(); ++i)
         textTrackAddCue(cues->item(i)->track(), cues->item(i));
-    endIgnoringTrackDisplayUpdateRequests();
-    updateActiveTextTrackCues(currentTime());
 }
 
 void HTMLMediaElement::textTrackRemoveCues(TextTrack*, const TextTrackCueList* cues) 
 {
-    beginIgnoringTrackDisplayUpdateRequests();
+    TrackDisplayUpdateScope(this);
     for (size_t i = 0; i < cues->length(); ++i)
         textTrackRemoveCue(cues->item(i)->track(), cues->item(i));
-    endIgnoringTrackDisplayUpdateRequests();
-    updateActiveTextTrackCues(currentTime());
 }
 
 void HTMLMediaElement::textTrackAddCue(TextTrack*, PassRefPtr<TextTrackCue> cue)
@@ -2773,12 +2801,11 @@ void HTMLMediaElement::mediaPlayerDidRemoveTrack(PassRefPtr<InbandTextTrackPriva
 
 void HTMLMediaElement::removeTrack(TextTrack* track)
 {
-    beginIgnoringTrackDisplayUpdateRequests();
-    m_textTracks->remove(track);
+    TrackDisplayUpdateScope(this);
     TextTrackCueList* cues = track->cues();
     if (cues)
         textTrackRemoveCues(track, cues);
-    endIgnoringTrackDisplayUpdateRequests();
+    m_textTracks->remove(track);
 }
 
 void HTMLMediaElement::removeAllInbandTracks()
@@ -2786,14 +2813,13 @@ void HTMLMediaElement::removeAllInbandTracks()
     if (!m_textTracks)
         return;
 
-    beginIgnoringTrackDisplayUpdateRequests();
+    TrackDisplayUpdateScope(this);
     for (int i = m_textTracks->length() - 1; i >= 0; --i) {
         TextTrack* track = m_textTracks->item(i);
 
         if (track->trackType() == TextTrack::InBand)
             removeTrack(track);
     }
-    endIgnoringTrackDisplayUpdateRequests();
 }
 
 PassRefPtr<TextTrack> HTMLMediaElement::addTextTrack(const String& kind, const String& label, const String& language, ExceptionCode& ec)
@@ -2897,14 +2923,7 @@ void HTMLMediaElement::didRemoveTrack(HTMLTrackElement* trackElement)
     // When a track element's parent element changes and the old parent was a media element, 
     // then the user agent must remove the track element's corresponding text track from the 
     // media element's list of text tracks.
-    m_textTracks->remove(textTrack.get());
-    if (textTrack->cues()) {
-        TextTrackCueList* cues = textTrack->cues();
-        beginIgnoringTrackDisplayUpdateRequests();
-        for (size_t i = 0; i < cues->length(); ++i)
-            textTrackRemoveCue(cues->item(i)->track(), cues->item(i));
-        endIgnoringTrackDisplayUpdateRequests();
-    }
+    removeTrack(textTrack.get());
 
     if (hasMediaControls())
         mediaControls()->closedCaptionTracksChanged();
index 6c6f64b..4026519 100644 (file)
@@ -372,7 +372,13 @@ protected:
     
     void addBehaviorRestriction(BehaviorRestrictions restriction) { m_restrictions |= restriction; }
     void removeBehaviorRestriction(BehaviorRestrictions restriction) { m_restrictions &= ~restriction; }
-    
+
+#if ENABLE(VIDEO_TRACK)
+    bool ignoreTrackDisplayUpdateRequests() const { return m_ignoreTrackDisplayUpdate > 0; }
+    void beginIgnoringTrackDisplayUpdateRequests();
+    void endIgnoringTrackDisplayUpdateRequests();
+#endif
+
 private:
     void createMediaPlayer();
 
@@ -501,10 +507,6 @@ private:
     void updateActiveTextTrackCues(float);
     HTMLTrackElement* showingTrackWithSameKind(HTMLTrackElement*) const;
 
-    bool ignoreTrackDisplayUpdateRequests() const { return m_ignoreTrackDisplayUpdate > 0; }
-    void beginIgnoringTrackDisplayUpdateRequests() { ++m_ignoreTrackDisplayUpdate; }
-    void endIgnoringTrackDisplayUpdateRequests() { ASSERT(m_ignoreTrackDisplayUpdate); --m_ignoreTrackDisplayUpdate; }
-
     void markCaptionAndSubtitleTracksAsUnconfigured();
     virtual void captionPreferencesChanged() OVERRIDE;
 #endif
@@ -701,6 +703,8 @@ private:
 #if PLATFORM(MAC)
     OwnPtr<DisplaySleepDisabler> m_sleepDisabler;
 #endif
+
+    friend class TrackDisplayUpdateScope;
 };
 
 #if ENABLE(VIDEO_TRACK)