Fix race when HTMLMediaElement.play() is called just after pause().

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 06d4a93b6dfca9cb3d42b26427ecb7be2d3f6673..dda056c1086c4dfd11fbd1a94d90f1a1b494d537 100644
--- a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
@@ -791,6 +791,9 @@ void HTMLMediaElement::invokeLoadAlgorithm()
     // one of the task queues, then remove those tasks.
     cancelPendingEventsAndCallbacks();
 
+    for (auto& resolver : m_playPromiseResolvers)
+        m_playPromiseRejectList.append(resolver);

[philipj_slow, 2016/04/06 14:12:59]

m_playPromiseRejectList.append(m_playPromiseResolvers) instead of loop?

[mlamouri (slow - plz ping), 2016/04/12 14:36:20]

Done. Here and other places.

+    m_playPromiseResolvers.clear();
     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
@@ -2011,8 +2014,20 @@ WebMediaPlayer::Preload HTMLMediaElement::effectivePreloadType() const
 
 ScriptPromise HTMLMediaElement::playForBindings(ScriptState* scriptState)
 {
+    // We have to share the same logic for internal and external callers. The
+    // internal callers do not want to receive a Promise back but when ::play()
+    // is called, |m_playPromiseResolvers| needs to be populated. What this code do

[philipj_slow, 2016/04/06 14:12:59]

s/do then/does/

[mlamouri (slow - plz ping), 2016/04/12 14:36:20]

Done.

+    // then is to populate |m_playPromiseResolvers| before calling ::play() and remove
+    // the Promise if ::play() failed.
+    ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
+    ScriptPromise promise = resolver->promise();
+    m_playPromiseResolvers.append(resolver);

[philipj_slow, 2016/04/06 14:12:59]

I don't follow why this change was needed. Does it have to do with the order
relative to something inside play()?

[mlamouri (slow - plz ping), 2016/04/12 14:36:20]

Assume the media is already playing. It will resolve all the promises in a task.
Obviously, that include the promise linked to the current play() request.
However, without this change, we add the promise *after* the task is created. It
was fine before because we would check the promise list when running the task.
Given that we keep it aside when creating the task now, we would populate an
empty list of promises and never resolve the promise.

+
     Nullable<ExceptionCode> code = play();
     if (!code.isNull()) {
+        ASSERT(!m_playPromiseResolvers.isEmpty());
+        m_playPromiseResolvers.removeLast();
+
         String message;
         switch (code.get()) {
         case NotAllowedError:
@@ -2027,10 +2042,6 @@ ScriptPromise HTMLMediaElement::playForBindings(ScriptState* scriptState)
         return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(code.get(), message));
     }
 
-    ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
-    ScriptPromise promise = resolver->promise();
-
-    m_playResolvers.append(resolver);
     return promise;
 }
 
@@ -3633,7 +3644,9 @@ DEFINE_TRACE(HTMLMediaElement)
     visitor->trace(m_cueTimeline);
     visitor->trace(m_textTracks);
     visitor->trace(m_textTracksWhenResourceSelectionBegan);
-    visitor->trace(m_playResolvers);
+    visitor->trace(m_playPromiseResolvers);
+    visitor->trace(m_playPromiseResolveList);
+    visitor->trace(m_playPromiseRejectList);
     visitor->trace(m_audioSourceProvider);
     visitor->trace(m_autoplayHelperClient);
     visitor->trace(m_autoplayHelper);
@@ -3715,10 +3728,20 @@ void HTMLMediaElement::triggerAutoplayViewportCheckForTesting()
 
 void HTMLMediaElement::scheduleResolvePlayPromises()
 {
-    // Per spec, if there are two tasks in the queue, the first task will remove
-    // all the pending promises making the second task useless unless a promise
-    // can be added between the first and second task being run which is not
-    // possible at the moment.
+    // TODO(mlamouri): per spec, we should create a new task but we can't create
+    // a new cancellable task without cancelling the previous one. There are two
+    // approaches then: cancel the previous task and create a new one with the
+    // appended promise list or append the new promise to the current list. The

[philipj_slow, 2016/04/06 14:12:59]

Hmm, so these are two decidedly not great options. I guess the easiest way to
see the problem is with a play();pause();enqueueTask();play();pause() sequence,
where both promises will be rejected by either the first or the second task, but
there should be one before and one after the unrelated task, which could perhaps
be from setting volume.

Can you write a failing test for this, and any other scenarios I haven't thought
of here? Those tests might indicate which option is the safest. FWIW, the
behavior with events is to just bundle them all up with the first task. That's
per spec because a media element has its own task source, but it still makes me
slightly inclined to favor a similar behavior here, simply adding to the list of
promises without posting a new task. Definitely on the fence, what risks do you
see?

[mlamouri (slow - plz ping), 2016/04/12 14:36:20]

I believe I'm doing what you are suggesting here or I misunderstood your
comment: when play(); pause(); stuf(); play(); pause(); happen, if the second
pause() rejects before the first is run, both of them will be rejected when the
first one should have been rejected.

The reason why I went with this solution, as explained is because it is likely
less observable: the second task will be resolved after pause() is called, just
maybe quicker than expected. Creating a new task would delay the first event
which could have side effects.

WDYT?

+    // former approach is prefered because it might be the less observable
+    // change.
+    ASSERT(m_playPromiseResolveList.isEmpty() || m_playPromiseResolveTask->isPending());
+    if (m_playPromiseResolvers.isEmpty())
+        return;
+
+    for (auto& resolver : m_playPromiseResolvers)
+        m_playPromiseResolveList.append(resolver);
+    m_playPromiseResolvers.clear();
+
     if (m_playPromiseResolveTask->isPending())
         return;
 
@@ -3727,10 +3750,20 @@ void HTMLMediaElement::scheduleResolvePlayPromises()
 
 void HTMLMediaElement::scheduleRejectPlayPromises(ExceptionCode code)
 {
-    // Per spec, if there are two tasks in the queue, the first task will remove
-    // all the pending promises making the second task useless unless a promise
-    // can be added between the first and second task being run which is not
-    // possible at the moment.
+    // TODO(mlamouri): per spec, we should create a new task but we can't create
+    // a new cancellable task without cancelling the previous one. There are two
+    // approaches then: cancel the previous task and create a new one with the
+    // appended promise list or append the new promise to the current list. The
+    // former approach is prefered because it might be the less observable
+    // change.
+    ASSERT(m_playPromiseRejectList.isEmpty() || m_playPromiseRejectTask->isPending());
+    if (m_playPromiseResolvers.isEmpty())
+        return;
+
+    for (auto& resolver : m_playPromiseResolvers)
+        m_playPromiseRejectList.append(resolver);
+    m_playPromiseResolvers.clear();
+
     if (m_playPromiseRejectTask->isPending())
         return;
 
@@ -3748,10 +3781,10 @@ void HTMLMediaElement::scheduleNotifyPlaying()
 
 void HTMLMediaElement::resolvePlayPromises()
 {
-    for (auto& resolver: m_playResolvers)
+    for (auto& resolver: m_playPromiseResolveList)
         resolver->resolve();
 
-    m_playResolvers.clear();
+    m_playPromiseResolveList.clear();
 }
 
 void HTMLMediaElement::rejectPlayPromises()
@@ -3770,10 +3803,10 @@ void HTMLMediaElement::rejectPlayPromises(ExceptionCode code, const String& mess
 {
     ASSERT(code == AbortError || code == NotSupportedError);
 
-    for (auto& resolver: m_playResolvers)
+    for (auto& resolver: m_playPromiseRejectList)
         resolver->reject(DOMException::create(code, message));
 
-    m_playResolvers.clear();
+    m_playPromiseRejectList.clear();
 }
 
 void HTMLMediaElement::clearWeakMembers(Visitor* visitor)