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 {
+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());
+            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 110 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 329 matching lines @@
     // 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();
+    OwnPtr<Task> task = Task::create(this);
+    m_pendingTask = task->createWeakPtr();
+    Microtask::enqueueMicrotask(task.release());

[philipj_slow, 2016/03/16 13:40:20]

If you have a look at RTCPeerConnection.cpp and HTMLSlotElement.cpp, there's a
way to enqueue a microtask that keeps the media element alive with a one-liner.
I learned about this in https://codereview.chromium.org/1712333002 and think it
should work here too.

[philipj_slow, 2016/03/16 15:35:10]

OK, so one problem could be where the spec says "Abort any already-running
instance of the resource selection algorithm for this element" so it'll actually
be necessary to cancel a pending microtask. Is this the only reason that you
ended up with a separate Task class, or is there more to it?

[Srirama, 2016/03/16 17:41:17]

Infact when i started i just copied it from ImageLoader but i realized that we
need it for aborting the already running instance. This is the only reason.
On 2016/03/16 15:35:10, philipj_UTC7 wrote:

[philipj_slow, 2016/03/17 11:30:55]

OK. So this problem has come up several times recently. To make HTMLMediaElement
more per spec, we really need both tasks and microtasks that can be canceled.
Events should also also be fired using such tasks, replacing the
m_asyncEventQueue.

Right now the best candidate for all of this is CancellableTaskFactory, but IIRC
it can only have one task in flight at a time, because that's just what
cancelAndCreate does. I'll add a few reviewers and ask for advice.

+    m_pendingActionFlags |= LoadMediaResource;
+}
+
+void HTMLMediaElement::continueResourceSelectionAlgorithm()

[philipj_slow, 2016/03/16 13:40:20]

All of this method looks a bit strange, should it not just consist of steps 5
and on in
https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo...
?

At least the text track bits shouldn't be here I think, and I think it ought to
be possible to get rid of the LoadMediaResource flag and only depend on the new
things you've added soon? If not, what remains before this can all be a 1:1
mapping of the spec?

[Srirama, 2016/03/16 14:25:40]

Without these changes all the media/track test cases are failing. Right now text
track uses loadtimer task and now we added micro task for resource selection. So
should we keep them seperate? May be microtask is fired faster than the
loadtimer task and probably that is the reason why all those tests are failing.
I will debug further.

[Srirama, 2016/03/16 14:35:23]

loadtimer started by scheduleTextTrackResourceLoad is being stopped in
invokeloadalgorithm and now we are posting microtask. Previously we used to post
loadtimer and that used to take care of texttrack as well.

[philipj_slow, 2016/03/16 15:35:10]

I think that really honorUserPreferencesForAutomaticTextTrackSelection() should
simply be called in the two places where the spec says so, if you follow the
back references in
https://html.spec.whatwg.org/multipage/embedded-content.html#honor-user-prefe...

If that still breaks some tests, I guess the current implementation isn't per
spec and the difference matters, in which case we'll have to look at precisely
what's wrong.

[Srirama, 2016/03/16 17:41:17]

Even i was going through it and wondering why it was held back till load timer.
I think this will work.
On 2016/03/16 15:35:10, philipj_UTC7 wrote:

+{
+    m_pendingTask.clear();
+    if (m_pendingActionFlags & LoadTextTrackResource)
+        honorUserPreferencesForAutomaticTextTrackSelection();
+
+    if (m_pendingActionFlags & LoadMediaResource) {
+        if (m_loadState == LoadingFromSourceElement)
+            loadNextSourceChild();
+        else
+            loadInternal();
+    }
+
+    m_pendingActionFlags = 0;
 }
 
 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 2330 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