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..542bc0236722e043c9f6ab855df929afb8aaf5e7 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"
@@ -268,6 +269,56 @@ String preloadTypeToString(WebMediaPlayer::Preload preloadType)
 
 } // anonymous namespace
 
+class HTMLMediaElement::Task : public WebTaskRunner::Task {

[philipj_slow, 2016/03/23 07:27:18]

It looks like it should be possible to use CancellableTaskFactory for this,
cancelAndCreate() returns a WebTaskRunner::Task and that can be passed to
enqueueMicrotask. Did you try that and it all falls apart because the
ScriptState tracking mentioned by TODO(jochen) in Microtask.h?

[Srirama, 2016/03/29 11:26:29]

I haven't tried that, do you want me to give a try inspite of Jochen's TODO
comments?

[philipj_slow, 2016/03/29 12:39:32]

Yeah, sure, I wouldn't assume that the ScriptState is actually needed, and in
fact if it is it could be a bug. (Because the most obvious thing one might need
a ScriptState for is to enqueue another microtask, and I don't think there's any
place per spec where that should happen in the sync steps after "await a stable
state".)

[Srirama, 2016/03/30 11:35:39]

Done.

+public:
+    static PassOwnPtr<Task> create(HTMLMediaElement* mediaElement)
+    {
+        return adoptPtr(new Task(mediaElement));
+    }
+
+    Task(HTMLMediaElement* mediaElement)
+        : m_mediaElement(mediaElement)
+        , m_weakFactory(this)
+    {
+        v8::Isolate* isolate =  V8PerIsolateData::mainThreadIsolate();
+        v8::HandleScope scope(isolate);
+        if (ScriptState::hasCurrentScriptState(isolate))
+            m_scriptState = ScriptState::current(isolate);
+        else
+            m_scriptState = ScriptState::forMainWorld(m_mediaElement->document().frame());
+    }
+
+    void run() override
+    {
+        WTF_LOG(Media, "HTMLMediaElement::Task::run(%p)", m_mediaElement.get());
+        if (!m_mediaElement)
+            return;
+
+        if (m_scriptState->contextIsValid()) {
+            ScriptState::Scope scope(m_scriptState.get());

[philipj_slow, 2016/03/24 04:43:05]

What break if there is no ScriptState::Scope here?

[Srirama, 2016/03/29 11:26:29]

Tested locally by removing and no cases are failing. I will remove it.

+            m_mediaElement->continueResourceSelectionAlgorithm();
+        } else {
+            m_mediaElement->continueResourceSelectionAlgorithm();
+        }
+    }
+
+    void clearTaskInfo()
+    {
+        m_mediaElement = nullptr;
+        m_scriptState.clear();
+    }
+
+    WeakPtr<Task> createWeakPtr()
+    {
+        return m_weakFactory.createWeakPtr();
+    }
+
+private:
+    RawPtrWillBeWeakPersistent<HTMLMediaElement> m_mediaElement;
+    RefPtr<ScriptState> m_scriptState;
+    WeakPtrFactory<Task> m_weakFactory;
+};
+
 void HTMLMediaElement::recordAutoplayMetric(AutoplayMetrics metric)
 {
     DEFINE_STATIC_LOCAL(EnumerationHistogram, autoplayHistogram, ("Blink.MediaElement.Autoplay", NumberOfAutoplayMetrics));
@@ -310,7 +361,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 +386,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)
@@ -398,6 +448,9 @@ HTMLMediaElement::~HTMLMediaElement()
 
     closeMediaSource();
 
+    if (m_pendingTask)
+        m_pendingTask->clearTaskInfo();
+
     removeElementFromDocumentMap(this, &document());
 
     // Destroying the player may cause a resource load to be canceled,
@@ -600,17 +653,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 +670,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 +770,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;
@@ -747,6 +779,10 @@ void HTMLMediaElement::invokeLoadAlgorithm()
     // 1 - Abort any already-running instance of the resource selection algorithm for this element.
     m_loadState = WaitingForSource;
     m_currentSourceNode = nullptr;
+    if (m_pendingTask) {
+        m_pendingTask->clearTaskInfo();
+        m_pendingTask.clear();
+    }
 
     // 2 - If there are any tasks from the media element's media element event task source in
     // one of the task queues, then remove those tasks.
@@ -833,9 +869,29 @@ 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();
+    enqueueMicrotask();
+}
+
+void HTMLMediaElement::continueResourceSelectionAlgorithm()
+{
+    m_pendingTask.clear();
+
+    if (m_textTrackLoadTimer.isActive()) {

[Srirama, 2016/03/18 07:38:11]

This is required to keep the order of honorUser.... and loading media resource
intact, as in some of the test cases texttrackloadtimer is fired a bit late. Do
you think we should still avoid this from here?

[philipj_slow, 2016/03/23 07:12:02]

Hmm, which test case is that?

[Srirama, 2016/03/29 11:26:29]

Sorry, couldn't remember exactly which one is failing. Removed it and now all
cases are passing.

+        m_textTrackLoadTimer.stop();
+        honorUserPreferencesForAutomaticTextTrackSelection();
+    }
+
+    if (m_loadState == LoadingFromSourceElement)

[philipj_slow, 2016/03/24 04:43:05]

This doesn't look right. The spec decides whether to use the src attributes or
source child elements after awaiting a stable state. Overall I would expect the
steps here to simply match steps 5 and onward. If it actually does, then
documenting why would be good, so that it can be later refactored to more
obviously match the spec.

[Srirama, 2016/03/29 11:26:29]

Hmm, probably this can simply be loadInternal() based on the decision for the
below comments.

+        loadNextSourceChild();
+    else
+        loadInternal();
+}
+
+void HTMLMediaElement::enqueueMicrotask()
+{
+    OwnPtr<Task> task = Task::create(this);
+    m_pendingTask = task->createWeakPtr();
+    Microtask::enqueueMicrotask(task.release());
 }
 
 void HTMLMediaElement::loadInternal()
@@ -1372,7 +1428,7 @@ void HTMLMediaElement::mediaLoadingFailed(WebMediaPlayer::NetworkState error)
 
         if (havePotentialSourceChild()) {
             WTF_LOG(Media, "HTMLMediaElement::setNetworkState(%p) - scheduling next <source>", this);
-            scheduleNextSourceChild();
+            enqueueMicrotask();

[philipj_slow, 2016/03/24 04:43:05]

I guess this is the "Await a stable state" after "Failed with elements"? If
continueResourceSelectionAlgorithm() is made to match step 5 and onward, this
will have to change as well. A specific function for each continuation point of
the algorithm is probably the most clear.  I'm guessing that m_loadState won't
be needed at all, but if it really is, then perhaps setting that state
explicitly here or ASSERTing that it's in the appropriate state would work too.

[Srirama, 2016/03/29 11:26:29]

Probably some more refactoring is needed before i can remove m_loadState. For
ex, we need to differentiate whether mediaLoadingFailed is called for 'source'
or 'src'.
And m_loadState is already validated above, so do we need to set/assert here
again?
And if we make different functions for different continuation points, do you
want me to call those functions based on m_loadState in
HTMLMediaElement::Task::run function?

[philipj_slow, 2016/03/29 12:39:32]

Which bits do you mean for "m_loadState is already validated above"?

I think the ideal state of things would be separate functions for every point
where the algorithm can continue, and that WTF::bind or similar is used to
schedule those continuations. I think keeping m_loadState around to
differentiate between src and src on error seems OK. One might consider renaming
it to correspond to the mode in
https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo...
but that would change with srcObject so perhaps it's best to just leave that
as-is for now. If the WaitingForSource entry ends up unused it could be removed.

I would probably suggest first checking if CancellableTaskFactory and WTF::bind
can be used together to replace the current inner class Task. If it can, then it
probably isn't too hard to continue with the changes I suggested. But if not,
then plan B would be more state. It could be an enum that basically keeps track
of where in the resource selection algorithm we currently are.

[Srirama, 2016/03/29 12:49:12]

This is actually part of mediaLoadingFailed() and i am referring to the conditon
at the start of the function 
if (m_readyState < HAVE_METADATA && m_loadState == LoadingFromSourceElement).
These lines are shrinked in the review window.

[philipj_slow, 2016/03/29 12:57:51]

Oh OK. Yeah in that case it would obviously make no sense to assert anything
about m_loadState or setting it to the same value again :)

[Srirama, 2016/03/30 11:35:39]

Done. Tried removing WaitingForSource blindly and some <source> element related
tests are failing (media-source-append-multiple.html). Probably needs more
refactoring to map it to 'mode' as you mentioned.

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

[philipj_slow, 2016/03/24 04:43:05]

This should actually synchronously jump to the "Find next candidate" step. I
think there should be tests in web-platform-tests that fail as long as that
isn't the case, I remember simonp@opera.com writing a lot of those.

The trouble here is to use some other mechanism than recursion, or too many
source elements will result in a stack overflow. Either an actual goto, a do {
... continue; ... } while(false), a return value that means "call this function
again" could work.

[Srirama, 2016/03/29 11:26:29]

This looks a bit confusing, this function sourceWasAdded() will be called from
script, so how can we synchronously go back to the "Find next candidate" step?
We are already invoking resource selection algo at the start of this function as
per spec
https://html.spec.whatwg.org/multipage/embedded-content.html#the-source-element.
Probably this function also needs some refactoring?

[philipj_slow, 2016/03/29 12:39:32]

Yes, the spec expresses this as a single big algorithm where we can end up
waiting. What we'd need is to make sure that the steps beginning at "find next
candidate" are in a separate function that be called here. With extra care taken
so that it's never called recursively in the case or many source elements.

[Srirama, 2016/03/30 11:35:39]

I thought loadNextSourceChild() is more appropriate for "Find next candidate"
section, so i just called. And regarding recursion, sorry i couldn't exactly
understand how it can go into a recursion. Here in sourceWasAdded we are calling
loadNextSourceChild() and i couldn't see from where loadNextSourceChild() will
be called again and again from inside. There is a "await a stable state"
(mediaLoadingFailed) before loadNextSourceChild() gets called again. Please let
me know if i miss something in understanding your point.

 }
 
 void HTMLMediaElement::sourceWasRemoved(HTMLSourceElement* source)
@@ -3110,9 +3166,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 +3241,10 @@ bool HTMLMediaElement::hasPendingActivity() const
     if (m_asyncEventQueue->hasPendingEvents())
         return true;
 
+    // Wait for any pending microtask to be fired.
+    if (m_pendingTask)
+        return true;
+
     return false;
 }