Media element resource selection algorithm should "await a stable state"

third_party/WebKit/Source/core/html/HTMLMediaElement.cpp

Index: third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
index 5a14ea40e1bed286a555021ffa380d58d79e775f..c2a21a60b1343c53f45b7b062bfd837b3d2fb1d8 100644
--- a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
@@ -35,6 +35,7 @@
 #include "core/dom/Attribute.h"
 #include "core/dom/ElementTraversal.h"
 #include "core/dom/Fullscreen.h"
+#include "core/dom/Microtask.h"
 #include "core/dom/shadow/ShadowRoot.h"
 #include "core/events/Event.h"
 #include "core/frame/LocalFrame.h"
@@ -310,7 +311,7 @@ bool HTMLMediaElement::isMediaStreamURL(const String& url)
 HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName, Document& document)
     : HTMLElement(tagName, document)
     , ActiveDOMObject(&document)
-    , m_loadTimer(this, &HTMLMediaElement::loadTimerFired)
+    , m_textTrackLoadTimer(this, &HTMLMediaElement::textTrackLoadTimerFired)
     , m_progressEventTimer(this, &HTMLMediaElement::progressEventTimerFired)
     , m_playbackProgressTimer(this, &HTMLMediaElement::playbackProgressTimerFired)
     , m_audioTracksTimer(this, &HTMLMediaElement::audioTracksTimerFired)
@@ -335,7 +336,6 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName, Document& docum
     , m_displayMode(Unknown)
     , m_cachedTime(std::numeric_limits<double>::quiet_NaN())
     , m_fragmentEndTime(std::numeric_limits<double>::quiet_NaN())
-    , m_pendingActionFlags(0)
     , m_userGestureRequiredForPlay(false)
     , m_playing(false)
     , m_shouldDelayLoadEvent(false)
@@ -361,6 +361,7 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName, Document& docum
     , m_textTracks(nullptr)
     , m_playPromiseResolveTask(CancellableTaskFactory::create(this, &HTMLMediaElement::resolvePlayPromises))
     , m_playPromiseRejectTask(CancellableTaskFactory::create(this, &HTMLMediaElement::rejectPlayPromises))
+    , m_resourceSelectionAlgorithmContinuationTask(CancellableTaskFactory::create(this, &HTMLMediaElement::continueResourceSelectionAlgorithm))
     , m_audioSourceNode(nullptr)
     , m_autoplayHelper(*this)
 {
@@ -398,6 +399,9 @@ HTMLMediaElement::~HTMLMediaElement()
 
     closeMediaSource();
 
+    if (m_resourceSelectionAlgorithmContinuationTask->isPending())
+        m_resourceSelectionAlgorithmContinuationTask->cancel();
+
     removeElementFromDocumentMap(this, &document());
 
     // Destroying the player may cause a resource load to be canceled,
@@ -600,17 +604,8 @@ void HTMLMediaElement::scheduleTextTrackResourceLoad()
 {
     WTF_LOG(Media, "HTMLMediaElement::scheduleTextTrackResourceLoad(%p)", this);
 
-    m_pendingActionFlags |= LoadTextTrackResource;
-
-    if (!m_loadTimer.isActive())
-        m_loadTimer.startOneShot(0, BLINK_FROM_HERE);
-}
-
-void HTMLMediaElement::scheduleNextSourceChild()
-{
-    // Schedule the timer to try the next <source> element WITHOUT resetting state ala invokeLoadAlgorithm.
-    m_pendingActionFlags |= LoadMediaResource;
-    m_loadTimer.startOneShot(0, BLINK_FROM_HERE);
+    if (!m_textTrackLoadTimer.isActive())
+        m_textTrackLoadTimer.startOneShot(0, BLINK_FROM_HERE);
 }
 
 void HTMLMediaElement::scheduleEvent(const AtomicString& eventName)
@@ -626,19 +621,10 @@ void HTMLMediaElement::scheduleEvent(PassRefPtrWillBeRawPtr<Event> event)
     m_asyncEventQueue->enqueueEvent(event);
 }
 
-void HTMLMediaElement::loadTimerFired(Timer<HTMLMediaElement>*)
+void HTMLMediaElement::textTrackLoadTimerFired(Timer<HTMLMediaElement>*)
 {
-    if (m_pendingActionFlags & LoadTextTrackResource)
-        honorUserPreferencesForAutomaticTextTrackSelection();
-
-    if (m_pendingActionFlags & LoadMediaResource) {
-        if (m_loadState == LoadingFromSourceElement)
-            loadNextSourceChild();
-        else
-            loadInternal();
-    }
-
-    m_pendingActionFlags = 0;
+    WTF_LOG(Media, "HTMLMediaElement::textTrackLoadTimerFired(%p)", this);
+    honorUserPreferencesForAutomaticTextTrackSelection();
 }
 
 MediaError* HTMLMediaElement::error() const
@@ -735,10 +721,7 @@ void HTMLMediaElement::invokeLoadAlgorithm()
 
     // Perform the cleanup required for the resource load algorithm to run.
     stopPeriodicTimers();
-    m_loadTimer.stop();
     cancelDeferredLoad();
-    // FIXME: Figure out appropriate place to reset LoadTextTrackResource if necessary and set m_pendingActionFlags to 0 here.
-    m_pendingActionFlags &= ~LoadMediaResource;
     m_sentEndEvent = false;
     m_sentStalledEvent = false;
     m_haveFiredLoadedData = false;
@@ -833,9 +816,19 @@ void HTMLMediaElement::invokeResourceSelectionAlgorithm()
         mediaControls()->reset();
 
     // 4 - Await a stable state, allowing the task that invoked this algorithm to continue
-    // TODO(srirama.m): Remove scheduleNextSourceChild() and post a microtask instead.
-    // See http://crbug.com/593289 for more details.
-    scheduleNextSourceChild();
+    enqueueMicrotaskForResourceSelectionAlgorithmContinuation();
+}
+
+void HTMLMediaElement::continueResourceSelectionAlgorithm()
+{
+    WTF_LOG(Media, "HTMLMediaElement::continueResourceSelectionAlgorithm(%p)", this);
+    loadInternal();
+}
+
+void HTMLMediaElement::enqueueMicrotaskForResourceSelectionAlgorithmContinuation()
+{
+    OwnPtr<WebTaskRunner::Task> task = adoptPtr(m_resourceSelectionAlgorithmContinuationTask->cancelAndCreate());
+    Microtask::enqueueMicrotask(task.release());
 }
 
 void HTMLMediaElement::loadInternal()
@@ -1372,7 +1365,7 @@ void HTMLMediaElement::mediaLoadingFailed(WebMediaPlayer::NetworkState error)
 
         if (havePotentialSourceChild()) {
             WTF_LOG(Media, "HTMLMediaElement::setNetworkState(%p) - scheduling next <source>", this);
-            scheduleNextSourceChild();
+            Microtask::enqueueMicrotask(WTF::bind(&HTMLMediaElement::loadNextSourceChild, this));

[philipj_slow, 2016/04/06 12:06:52]

Per spec the "await a stable state" is before forgetResourceSpecificTracks(),
could you try to move it up to get the same effect? Note that you'll have to
make this microtask cancelable as well, and cancel it if the resource selection
algorithm is aborted.

[Srirama, 2016/04/11 08:07:59]

Done.

         } else {
             WTF_LOG(Media, "HTMLMediaElement::setNetworkState(%p) - no more <source> elements, waiting", this);
             waitForSourceChange();
@@ -2772,7 +2765,7 @@ void HTMLMediaElement::sourceWasAdded(HTMLSourceElement* source)
 
     // 25. Jump back to the find next candidate step above.
     m_nextChildNodeToConsider = source;
-    scheduleNextSourceChild();
+    loadNextSourceChild();

[philipj_slow, 2016/04/06 12:06:52]

If there are many source children, will this recurse on the stack? Can you make
it crash by having 10000 of them?

[Srirama, 2016/04/11 08:07:59]

I wrote a test content like below.
*********************
audio = document.getElementById('a');
var audioFile = findMediaFile("audio", "content/test");
for (var i = 0; i < 10; i++) {
    source = document.createElement("source");
    source.setAttribute("src", "invalid.mp3");
    audio.appendChild(source);
}

source = document.createElement("source");
source.setAttribute("src", audioFile);
audio.appendChild(source);
***********************
loadNextSourceChild is never called from sourceWasAdded function (it bails out
early from one of the conditions), probably i will change the test case, but i
still feel that if it reaches to loadNextSourceChild step here it would have
finished previous source child processing.

[philipj_slow, 2016/04/19 14:52:44]

Can you change the test so that it appends 10000 plain source elements with no
attributes or anything, and then a source element that would actually load? When
you get to the source element with the src attribute, what does the stack look
like?

 }
 
 void HTMLMediaElement::sourceWasRemoved(HTMLSourceElement* source)
@@ -3110,9 +3103,8 @@ void HTMLMediaElement::clearMediaPlayer()
     }
 
     stopPeriodicTimers();
-    m_loadTimer.stop();
+    m_textTrackLoadTimer.stop();
 
-    m_pendingActionFlags = 0;
     m_loadState = WaitingForSource;
 
     // We can't cast if we don't have a media player.
@@ -3186,6 +3178,10 @@ bool HTMLMediaElement::hasPendingActivity() const
     if (m_asyncEventQueue->hasPendingEvents())
         return true;
 
+    // Wait for any pending microtask to be fired.
+    if (m_resourceSelectionAlgorithmContinuationTask->isPending())

[philipj_slow, 2016/04/06 12:06:52]

Now that we're in an Oilpan only world I'm not entirely sure this is needed.
Will the pending task itself not have a strong reference to the HTMLMediaElement
object, or is it weak?

[Srirama, 2016/04/11 08:07:59]

I think it will be a strong pointer (ownptr), isn't it. I will remove it.

+        return true;
+
     return false;
 }