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..c273ad9de4fa8aac98614c947e3cb3bf4b601b41 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 {
+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));
@@ -398,6 +449,9 @@ HTMLMediaElement::~HTMLMediaElement()
 
     closeMediaSource();
 
+    if (m_pendingTask)
+        m_pendingTask->clearTaskInfo();
+
     removeElementFromDocumentMap(this, &document());
 
     // Destroying the player may cause a resource load to be canceled,
@@ -747,6 +801,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 +891,26 @@ 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();
+    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()
@@ -3186,6 +3261,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;
 }