Title: [198780] trunk
Revision
198780
Author
[email protected]
Date
2016-03-29 08:40:33 -0700 (Tue, 29 Mar 2016)

Log Message

media/track/track-remove-track.html is flaky, crashing and failing
https://bugs.webkit.org/show_bug.cgi?id=130971

Reviewed by Alexey Proskuryakov.
Source/WebCore:

Prevent HTMLMediaElement from being collected while it is creating media controls.
These changes prevent the test from crashing but they do not fix the flakiness,
which is caused by another bug. Fixing that is tracked by
https://bugs.webkit.org/show_bug.cgi?id=155956.

* html/HTMLMediaElement.cpp:
(WebCore::actionName): New, debugging-only helper function.
(WebCore::HTMLMediaElement::HTMLMediaElement): Initialize new variables.
(WebCore::HTMLMediaElement::scheduleDelayedAction): Log the flag names to make debugging easier.
(WebCore::HTMLMediaElement::scheduleNextSourceChild): Add logging.
(WebCore::HTMLMediaElement::updateActiveTextTrackCues): Update logging.
(WebCore::HTMLMediaElement::configureTextTrackGroup): Drive-by optimization: don't call
  updateCaptionContainer here, call it before exiting configureTextTracks so we only call
  it once instead of once per track group.
(WebCore::controllerJSValue):
(WebCore::HTMLMediaElement::ensureMediaControlsShadowRoot): New, wrapper around calling
  ensureUserAgentShadowRoot so m_creatingControls can be set and cleared appropriately.
(WebCore::HTMLMediaElement::updateCaptionContainer): ensureUserAgentShadowRoot ->
  ensureMediaControlsShadowRoot. Drive by optimization: set/test m_haveSetupCaptionContainer
  so we only do this setup once.
(WebCore::HTMLMediaElement::configureTextTracks): Call updateCaptionContainer.
(WebCore::HTMLMediaElement::clearMediaPlayer): Log flag names.
(WebCore::HTMLMediaElement::hasPendingActivity): Return true when creating controls so GC
  won't happen during controls setup.
(WebCore::HTMLMediaElement::updateTextTrackDisplay): ensureUserAgentShadowRoot ->
  ensureMediaControlsShadowRoot.
(WebCore::HTMLMediaElement::createMediaControls): Ditto.
(WebCore::HTMLMediaElement::configureMediaControls): Ditto.
(WebCore::HTMLMediaElement::configureTextTrackDisplay): Ditto.
* html/HTMLMediaElement.h:

LayoutTests:

* platform/mac/TestExpectations: Mark crash as flaky only.

Modified Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (198779 => 198780)


--- trunk/LayoutTests/ChangeLog	2016-03-29 13:22:12 UTC (rev 198779)
+++ trunk/LayoutTests/ChangeLog	2016-03-29 15:40:33 UTC (rev 198780)
@@ -1,3 +1,12 @@
+2016-03-29  Eric Carlson  <[email protected]>
+
+        media/track/track-remove-track.html is flaky, crashing and failing
+        https://bugs.webkit.org/show_bug.cgi?id=130971
+
+        Reviewed by Alexey Proskuryakov.
+
+        * platform/mac/TestExpectations: Mark crash as flaky only.
+
 2016-03-29  Gyuyoung Kim  <[email protected]>
 
         [EFL] Skip to test custom element test cases

Modified: trunk/LayoutTests/platform/mac/TestExpectations (198779 => 198780)


--- trunk/LayoutTests/platform/mac/TestExpectations	2016-03-29 13:22:12 UTC (rev 198779)
+++ trunk/LayoutTests/platform/mac/TestExpectations	2016-03-29 15:40:33 UTC (rev 198780)
@@ -909,7 +909,6 @@
 webkit.org/b/123099 media/media-controller-time-clamp.html [ Pass Timeout ]
 webkit.org/b/123522 media/track/track-in-band-legacy-api.html [ Pass Failure Crash ]
 webkit.org/b/130490 media/video-remote-control-playpause.html [ Pass Failure ]
-webkit.org/b/130971 media/track/track-remove-track.html [ Pass Crash Failure Timeout ]
 webkit.org/b/131855 media/event-attributes.html [ Pass Failure Timeout ]
 webkit.org/b/133363 media/video-rtl.html [ Pass ImageOnlyFailure ]
 webkit.org/b/133686 media/track/w3c/interfaces/TextTrackCue/align.html [ Pass Failure ]
@@ -930,6 +929,7 @@
 webkit.org/b/147944 media/video-seek-to-current-time.html [ Pass Failure ]
 webkit.org/b/114177 media/event-queue-crash.html [ Pass Failure ]
 webkit.org/b/150408 http/tests/media/video-load-suspend.html [ Pass Timeout ]
+webkit.org/b/155956 media/track/track-remove-track.html [ Pass Failure Timeout ]
 ## --- End flaky media tests
 
 # Skipped while Eric Carlson works on a fix.

Modified: trunk/Source/WebCore/ChangeLog (198779 => 198780)


--- trunk/Source/WebCore/ChangeLog	2016-03-29 13:22:12 UTC (rev 198779)
+++ trunk/Source/WebCore/ChangeLog	2016-03-29 15:40:33 UTC (rev 198780)
@@ -1,3 +1,41 @@
+2016-03-29  Eric Carlson  <[email protected]>
+
+        media/track/track-remove-track.html is flaky, crashing and failing
+        https://bugs.webkit.org/show_bug.cgi?id=130971
+
+        Reviewed by Alexey Proskuryakov.
+        
+        Prevent HTMLMediaElement from being collected while it is creating media controls.
+        These changes prevent the test from crashing but they do not fix the flakiness,
+        which is caused by another bug. Fixing that is tracked by 
+        https://bugs.webkit.org/show_bug.cgi?id=155956.
+
+        * html/HTMLMediaElement.cpp:
+        (WebCore::actionName): New, debugging-only helper function.
+        (WebCore::HTMLMediaElement::HTMLMediaElement): Initialize new variables.
+        (WebCore::HTMLMediaElement::scheduleDelayedAction): Log the flag names to make debugging easier.
+        (WebCore::HTMLMediaElement::scheduleNextSourceChild): Add logging.
+        (WebCore::HTMLMediaElement::updateActiveTextTrackCues): Update logging.
+        (WebCore::HTMLMediaElement::configureTextTrackGroup): Drive-by optimization: don't call 
+          updateCaptionContainer here, call it before exiting configureTextTracks so we only call
+          it once instead of once per track group.
+        (WebCore::controllerJSValue):
+        (WebCore::HTMLMediaElement::ensureMediaControlsShadowRoot): New, wrapper around calling
+          ensureUserAgentShadowRoot so m_creatingControls can be set and cleared appropriately.
+        (WebCore::HTMLMediaElement::updateCaptionContainer): ensureUserAgentShadowRoot -> 
+          ensureMediaControlsShadowRoot. Drive by optimization: set/test m_haveSetupCaptionContainer
+          so we only do this setup once.
+        (WebCore::HTMLMediaElement::configureTextTracks): Call updateCaptionContainer.
+        (WebCore::HTMLMediaElement::clearMediaPlayer): Log flag names.
+        (WebCore::HTMLMediaElement::hasPendingActivity): Return true when creating controls so GC
+          won't happen during controls setup.
+        (WebCore::HTMLMediaElement::updateTextTrackDisplay): ensureUserAgentShadowRoot -> 
+          ensureMediaControlsShadowRoot.
+        (WebCore::HTMLMediaElement::createMediaControls): Ditto.
+        (WebCore::HTMLMediaElement::configureMediaControls): Ditto.
+        (WebCore::HTMLMediaElement::configureTextTrackDisplay): Ditto.
+        * html/HTMLMediaElement.h:
+
 2016-03-29  Gyuyoung Kim  <[email protected]>
 
         Unreviewed EFL build fix caused by r198773

Modified: trunk/Source/WebCore/html/HTMLMediaElement.cpp (198779 => 198780)


--- trunk/Source/WebCore/html/HTMLMediaElement.cpp	2016-03-29 13:22:12 UTC (rev 198779)
+++ trunk/Source/WebCore/html/HTMLMediaElement.cpp	2016-03-29 15:40:33 UTC (rev 198780)
@@ -185,6 +185,30 @@
 {
     return val ? "true" : "false";
 }
+
+static String actionName(HTMLMediaElementEnums::DelayedActionType action)
+{
+    StringBuilder actionBuilder;
+
+#define CASE(_actionType) \
+    if (action & (HTMLMediaElementEnums::_actionType)) { \
+        if (!actionBuilder.isEmpty()) \
+        actionBuilder.append(", "); \
+        actionBuilder.append(#_actionType); \
+    } \
+
+    CASE(LoadMediaResource);
+    CASE(ConfigureTextTracks);
+    CASE(TextTrackChangesNotification);
+    CASE(ConfigureTextTrackDisplay);
+    CASE(CheckPlaybackTargetCompatablity);
+    CASE(CheckMediaState);
+
+    return actionBuilder.toString();
+
+#undef CASE
+}
+
 #endif
 
 #ifndef LOG_MEDIA_EVENTS
@@ -382,8 +406,10 @@
     , m_havePreparedToPlay(false)
     , m_parsingInProgress(createdByParser)
     , m_elementIsHidden(document.hidden())
+    , m_creatingControls(false)
 #if ENABLE(MEDIA_CONTROLS_SCRIPT)
     , m_mediaControlsDependOnPageScaleFactor(false)
+    , m_haveSetUpCaptionContainer(false)
 #endif
 #if ENABLE(VIDEO_TRACK)
     , m_tracksAreReady(true)
@@ -822,7 +848,7 @@
 
 void HTMLMediaElement::scheduleDelayedAction(DelayedActionType actionType)
 {
-    LOG(Media, "HTMLMediaElement::scheduleLoad(%p)", this);
+    LOG(Media, "HTMLMediaElement::scheduleDelayedAction(%p) - setting %s flag", this, actionName(actionType).utf8().data());
 
     if ((actionType & LoadMediaResource) && !(m_pendingActionFlags & LoadMediaResource)) {
         prepareForLoad();
@@ -848,6 +874,7 @@
 void HTMLMediaElement::scheduleNextSourceChild()
 {
     // Schedule the timer to try the next <source> element WITHOUT resetting state ala prepareForLoad.
+    LOG(Media, "HTMLMediaElement::scheduleNextSourceChild(%p) - setting %s flag", this, actionName(LoadMediaResource).utf8().data());
     setFlags(m_pendingActionFlags, LoadMediaResource);
     m_pendingActionTimer.startOneShot(0);
 }
@@ -1405,7 +1432,7 @@
     if (ignoreTrackDisplayUpdateRequests())
         return;
 
-    LOG(Media, "HTMLMediaElement::updateActiveTextTracks(%p)", this);
+    LOG(Media, "HTMLMediaElement::updateActiveTextTrackCues(%p)", this);
 
     // 1 - Let current cues be a list of cues, initialized to contain all the
     // cues of all the hidden, showing, or showing by default text tracks of the
@@ -3853,8 +3880,6 @@
             m_webkitLegacyClosedCaptionOverride = true;
     }
 
-    updateCaptionContainer();
-
     m_processingPreferenceChange = false;
 }
 
@@ -3883,11 +3908,22 @@
 
     return controllerJSWrapper;
 }
-    
+
+void HTMLMediaElement::ensureMediaControlsShadowRoot()
+{
+    ASSERT(!m_creatingControls);
+    m_creatingControls = true;
+    ensureUserAgentShadowRoot();
+    m_creatingControls = false;
+}
+
 void HTMLMediaElement::updateCaptionContainer()
 {
     LOG(Media, "HTMLMediaElement::updateCaptionContainer(%p)", this);
 #if ENABLE(MEDIA_CONTROLS_SCRIPT)
+    if (m_haveSetUpCaptionContainer)
+        return;
+
     Page* page = document().page();
     if (!page)
         return;
@@ -3897,7 +3933,7 @@
     if (!ensureMediaControlsInjectedScript())
         return;
 
-    ensureUserAgentShadowRoot();
+    ensureMediaControlsShadowRoot();
 
     if (!m_mediaControlsHost)
         m_mediaControlsHost = MediaControlsHost::create(this);
@@ -3931,6 +3967,8 @@
     JSC::MarkedArgumentBuffer noArguments;
     JSC::call(exec, methodObject, callType, callData, controllerObject, noArguments);
     exec->clearException();
+
+    m_haveSetUpCaptionContainer = true;
 #endif
 }
 
@@ -4050,6 +4088,7 @@
     if (otherTracks.tracks.size())
         configureTextTrackGroup(otherTracks);
 
+    updateCaptionContainer();
     configureTextTrackDisplay();
     if (hasMediaControls())
         mediaControls()->closedCaptionTracksChanged();
@@ -4916,9 +4955,9 @@
 #endif
 }
 
-void HTMLMediaElement::clearMediaPlayer(int flags)
+void HTMLMediaElement::clearMediaPlayer(DelayedActionType flags)
 {
-    LOG(Media, "HTMLMediaElement::clearMediaPlayer(%p) - flags = %x", this, (unsigned)flags);
+    LOG(Media, "HTMLMediaElement::clearMediaPlayer(%p) - flags = %s", this, actionName(flags).utf8().data());
 
 #if ENABLE(VIDEO_TRACK)
     forgetResourceSpecificTracks();
@@ -5067,7 +5106,7 @@
 
 bool HTMLMediaElement::hasPendingActivity() const
 {
-    return (hasAudio() && isPlaying()) || m_asyncEventQueue.hasPendingEvents();
+    return (hasAudio() && isPlaying()) || m_asyncEventQueue.hasPendingEvents() || m_creatingControls;
 }
 
 void HTMLMediaElement::mediaVolumeDidChange()
@@ -5455,7 +5494,7 @@
 void HTMLMediaElement::updateTextTrackDisplay()
 {
 #if ENABLE(MEDIA_CONTROLS_SCRIPT)
-    ensureUserAgentShadowRoot();
+    ensureMediaControlsShadowRoot();
     ASSERT(m_mediaControlsHost);
     m_mediaControlsHost->updateTextTrackContainer();
 #else
@@ -5615,7 +5654,7 @@
 bool HTMLMediaElement::createMediaControls()
 {
 #if ENABLE(MEDIA_CONTROLS_SCRIPT)
-    ensureUserAgentShadowRoot();
+    ensureMediaControlsShadowRoot();
     return false;
 #else
     if (hasMediaControls())
@@ -5660,7 +5699,7 @@
     if (!requireControls || !inDocument() || !inActiveDocument())
         return;
 
-    ensureUserAgentShadowRoot();
+    ensureMediaControlsShadowRoot();
 #else
     if (!requireControls || !inDocument() || !inActiveDocument()) {
         if (hasMediaControls())
@@ -5706,7 +5745,7 @@
     if (!m_haveVisibleTextTrack)
         return;
 
-    ensureUserAgentShadowRoot();
+    ensureMediaControlsShadowRoot();
 #else
     if (!m_haveVisibleTextTrack && !hasMediaControls())
         return;

Modified: trunk/Source/WebCore/html/HTMLMediaElement.h (198779 => 198780)


--- trunk/Source/WebCore/html/HTMLMediaElement.h	2016-03-29 13:22:12 UTC (rev 198779)
+++ trunk/Source/WebCore/html/HTMLMediaElement.h	2016-03-29 15:40:33 UTC (rev 198780)
@@ -649,7 +649,7 @@
     void scheduleNextSourceChild();
     void loadNextSourceChild();
     void userCancelledLoad();
-    void clearMediaPlayer(int flags);
+    void clearMediaPlayer(DelayedActionType flags);
     bool havePotentialSourceChild();
     void noneSupported();
     void cancelPendingEventsAndCallbacks();
@@ -754,6 +754,7 @@
     void unregisterWithDocument(Document&);
 
     void updateCaptionContainer();
+    void ensureMediaControlsShadowRoot();
 
 #if ENABLE(WIRELESS_PLAYBACK_TARGET)
     void prepareForDocumentSuspension() final;
@@ -904,9 +905,11 @@
     bool m_havePreparedToPlay : 1;
     bool m_parsingInProgress : 1;
     bool m_elementIsHidden : 1;
+    bool m_creatingControls : 1;
 
 #if ENABLE(MEDIA_CONTROLS_SCRIPT)
     bool m_mediaControlsDependOnPageScaleFactor : 1;
+    bool m_haveSetUpCaptionContainer : 1;
 #endif
 
 #if ENABLE(VIDEO_TRACK)
_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to