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

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

 /*
  * Copyright (C) 2007, 2008, 2009, 2010, 2011, 2012, 2013 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 17 matching lines @@
 #include "bindings/core/v8/ExceptionState.h"
 #include "bindings/core/v8/ExceptionStatePlaceholder.h"
 #include "bindings/core/v8/ScriptController.h"
 #include "bindings/core/v8/ScriptEventListener.h"
 #include "bindings/core/v8/ScriptPromiseResolver.h"
 #include "core/HTMLNames.h"
 #include "core/css/MediaList.h"
 #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"
 #include "core/frame/Settings.h"
 #include "core/frame/UseCounter.h"
 #include "core/frame/csp/ContentSecurityPolicy.h"
 #include "core/html/HTMLMediaSource.h"
 #include "core/html/HTMLSourceElement.h"
 #include "core/html/HTMLTrackElement.h"
 #include "core/html/MediaError.h"
@@ skipping 213 matching lines @@
     case WebMediaPlayer::PreloadAuto:
         return "auto";
     }
 
     ASSERT_NOT_REACHED();
     return String();
 }
 
 } // 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));
     autoplayHistogram.count(metric);
 }
 
 WebMimeRegistry::SupportsType HTMLMediaElement::supportsType(const ContentType& contentType)
 {
     DEFINE_STATIC_LOCAL(const String, codecs, ("codecs"));
 
@@ skipping 22 matching lines @@
 }
 
 bool HTMLMediaElement::isMediaStreamURL(const String& url)
 {
     return s_mediaStreamRegistry ? s_mediaStreamRegistry->contains(url) : false;
 }
 
 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)
     , m_playedTimeRanges()
     , m_asyncEventQueue(GenericEventQueue::create(this))
     , m_playbackRate(1.0f)
     , m_defaultPlaybackRate(1.0f)
     , m_networkState(NETWORK_EMPTY)
     , m_readyState(HAVE_NOTHING)
     , m_readyStateMaximum(HAVE_NOTHING)
     , m_volume(1.0f)
     , m_lastSeekTime(0)
     , m_previousProgressTime(std::numeric_limits<double>::max())
     , m_duration(std::numeric_limits<double>::quiet_NaN())
     , m_lastTimeUpdateEventWallTime(0)
     , m_lastTimeUpdateEventMovieTime(0)
     , m_defaultPlaybackStartPosition(0)
     , m_loadState(WaitingForSource)
     , m_deferredLoadState(NotDeferred)
     , m_deferredLoadTimer(this, &HTMLMediaElement::deferredLoadTimerFired)
     , m_webLayer(nullptr)
     , 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)
     , m_haveFiredLoadedData(false)
     , m_autoplaying(true)
     , m_muted(false)
     , m_paused(true)
     , m_seeking(false)
     , m_sentStalledEvent(false)
     , m_sentEndEvent(false)
@@ skipping 42 matching lines @@
 
     setShouldDelayLoadEvent(false);
 
     if (m_textTracks)
         m_textTracks->clearOwner();
     m_audioTracks->shutdown();
     m_videoTracks->shutdown();
 
     closeMediaSource();
 
+    if (m_pendingTask)
+        m_pendingTask->clearTaskInfo();
+
     removeElementFromDocumentMap(this, &document());
 
     // Destroying the player may cause a resource load to be canceled,
     // which could result in LocalDOMWindow::dispatchWindowLoadEvent() being
     // called via ResourceFetch::didLoadResource() then
     // FrameLoader::checkCompleted(). To prevent load event dispatching during
     // object destruction, we use Document::incrementLoadEventDelayCount().
     // See http://crbug.com/275223 for more details.
     document().incrementLoadEventDelayCount();
 
@@ skipping 182 matching lines @@
 void HTMLMediaElement::didRecalcStyle(StyleRecalcChange)
 {
     if (layoutObject())
         layoutObject()->updateFromElement();
 }
 
 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)
 {
     scheduleEvent(Event::createCancelable(eventName));
 }
 
 void HTMLMediaElement::scheduleEvent(PassRefPtrWillBeRawPtr<Event> event)
 {
 #if LOG_MEDIA_EVENTS
     WTF_LOG(Media, "HTMLMediaElement::scheduleEvent(%p) - scheduling '%s'", this, event->type().ascii().data());
 #endif
     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
 {
     return m_error;
 }
 
 void HTMLMediaElement::setSrc(const AtomicString& url)
 {
     setAttribute(srcAttr, url);
@@ skipping 76 matching lines @@
 // TODO(srirama.m): Currently m_ignorePreloadNone is reset before calling
 // invokeLoadAlgorithm() in all places except load(). Move it inside here
 // once microtask is implemented for "Await a stable state" step
 // in resource selection algorithm.
 void HTMLMediaElement::invokeLoadAlgorithm()
 {
     WTF_LOG(Media, "HTMLMediaElement::invokeLoadAlgorithm(%p)", this);
 
     // 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;
     m_displayMode = Unknown;
 
     // 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.
     cancelPendingEventsAndCallbacks();
 
     rejectPlayPromises(AbortError, "The play() request was interrupted by a new load request.");
 
     // 3 - If the media element's networkState is set to NETWORK_LOADING or NETWORK_IDLE, queue
     // a task to fire a simple event named abort at the media element.
     if (m_networkState == NETWORK_LOADING || m_networkState == NETWORK_IDLE)
@@ skipping 66 matching lines @@
     // so they are closer to the relevant spec steps.
     m_lastSeekTime = 0;
     m_duration = std::numeric_limits<double>::quiet_NaN();
 
     // 3 - Set the media element's delaying-the-load-event flag to true (this delays the load event)
     setShouldDelayLoadEvent(true);
     if (mediaControls())
         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()
 {
     // HTMLMediaElement::textTracksAreReady will need "... the text tracks whose mode was not in the
     // disabled state when the element's resource selection algorithm last started".
     m_textTracksWhenResourceSelectionBegan.clear();
     if (m_textTracks) {
         for (unsigned i = 0; i < m_textTracks->length(); ++i) {
             TextTrack* track = m_textTracks->anonymousIndexedGetter(i);
@@ skipping 516 matching lines @@
         else
             WTF_LOG(Media, "HTMLMediaElement::setNetworkState(%p) - error event not sent, <source> was removed", this);
 
         // 9.Otherwise.10 - Asynchronously await a stable state. The synchronous section consists of all the remaining steps of this algorithm until the algorithm says the synchronous section has ended.
 
         // 9.Otherwise.11 - Forget the media element's media-resource-specific tracks.
         forgetResourceSpecificTracks();
 
         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();
         }
 
         return;
     }
 
     if (error == WebMediaPlayer::NetworkStateNetworkError && m_readyState >= HAVE_METADATA)
         mediaEngineError(MediaError::create(MediaError::MEDIA_ERR_NETWORK));
@@ skipping 1379 matching lines @@
     // 22. Asynchronously await a stable state...
     // 23. Set the element's delaying-the-load-event flag back to true (this delays the load event again, in case
     // it hasn't been fired yet).
     setShouldDelayLoadEvent(true);
 
     // 24. Set the networkState back to NETWORK_LOADING.
     setNetworkState(NETWORK_LOADING);
 
     // 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)
 {
     WTF_LOG(Media, "HTMLMediaElement::sourceWasRemoved(%p, %p)", this, source);
 
 #if !LOG_DISABLED
     KURL url = source->getNonEmptyURLAttribute(srcAttr);
     WTF_LOG(Media, "HTMLMediaElement::sourceWasRemoved(%p) - 'src' is %s", this, urlForLoggingMedia(url).utf8().data());
 #endif
@@ skipping 317 matching lines @@
     closeMediaSource();
 
     cancelDeferredLoad();
 
     {
         AudioSourceProviderClientLockScope scope(*this);
         clearMediaPlayerAndAudioSourceProviderClientWithoutLocking();
     }
 
     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.
     m_remoteRoutesAvailable = false;
     m_playingRemotely = false;
     if (mediaControls())
         mediaControls()->refreshCastButtonVisibilityWithoutUpdate();
 
     if (layoutObject())
         layoutObject()->setShouldDoFullPaintInvalidation();
@@ skipping 53 matching lines @@
 
     // When connected to a MediaSource, e.g. setting MediaSource.duration will cause a
     // durationchange event to be fired.
     if (m_mediaSource)
         return true;
 
     // Wait for any pending events to be fired.
     if (m_asyncEventQueue->hasPendingEvents())
         return true;
 
+    // Wait for any pending microtask to be fired.
+    if (m_pendingTask)
+        return true;
+
     return false;
 }
 
 bool HTMLMediaElement::isFullscreen() const
 {
     return Fullscreen::isActiveFullScreenElement(*this);
 }
 
 void HTMLMediaElement::enterFullscreen()
 {
@@ skipping 605 matching lines @@
 {
     visitor->trace(m_client);
 }
 
 DEFINE_TRACE(HTMLMediaElement::AudioSourceProviderImpl)
 {
     visitor->trace(m_client);
 }
 
 } // namespace blink