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

The consequence is that the Promise returned by play() is rejected by
the task created by pause(). The fix is to associate the tasks with a
list of promises.

BUG=593273
[email protected]

Review-Url: https://codereview.chromium.org/1865933002
Cr-Commit-Position: refs/heads/master@{#398370}
(cherry picked from commit 3a82d82)

Review URL: https://codereview.chromium.org/2053333002 .

Cr-Commit-Position: refs/branch-heads/2743@{crosswalk-project#311}
Cr-Branched-From: 2b3ae3b-refs/heads/master@{#394939}

third_party/WebKit/LayoutTests/media/media-play-promise-expected.txt

@@ -167,5 +167,20 @@ play()
 RUN(mediaElement.volume = 0.2)
 arguments.length: 1
 Promise failed with NotSupportedError: The element has no supported sources.
+
+pausePlayAfterPlaybackStarted()
+RUN(mediaElement = document.createElement('audio'))
+RUN(mediaElement.src = 'content/test.wav')
+EVENT(volumechange)
+EVENT(volumechange)
+EVENT(canplaythrough)
+RUN(mediaElement.play())
+EVENT(playing)
+EXPECTED (mediaElement.readyState == '4') OK
+EXPECTED (mediaElement.paused == 'false') OK
+RUN(mediaElement.pause())
+play()
+arguments.length: 1
+Promise resolved with undefined
 END OF TEST
 

third_party/WebKit/LayoutTests/media/media-play-promise.html

@@ -65,7 +65,7 @@
 
     var TESTS = [
         // Test that play() on an element that doesn't have enough data will
-        // return a promise that resolves successfuly.
+        // return a promise that resolves successfully.
         function playBeforeCanPlay()
         {
             consoleWrite("playBeforeCanPlay()");
@@ -85,7 +85,7 @@
         },
 
         // Test that play() on an element that has enough data will return a
-        // promise that resolves successfuly.
+        // promise that resolves successfully.
         function playWhenCanPlay()
         {
             consoleWrite("playWhenCanPlay()");
@@ -105,6 +105,8 @@
             });
         },
 
+        // Test that play() on an element that is already playing returns a
+        // promise that resolves successfully.
         function playAfterPlaybackStarted()
         {
             consoleWrite("playAfterPlaybackStarted()");
@@ -391,7 +393,32 @@
                 run("mediaElement.volume = 0.2");
             });
 
-        }
+        },
+
+        // Test that calling pause() then play() on an element that is already
+        // playing returns a promise that resolves successfully.
+        function pausePlayAfterPlaybackStarted()
+        {
+            consoleWrite("pausePlayAfterPlaybackStarted()");
+            internals.settings.setMediaPlaybackRequiresUserGesture(false);
+
+            run("mediaElement = document.createElement('audio')");
+            mediaElement.preload = "auto";
+            var mediaFile = findMediaFile("audio", "content/test");
+            run("mediaElement.src = '" + mediaFile + "'");
+
+            waitForEvent('playing', function() {
+                testExpected("mediaElement.readyState", HTMLMediaElement.HAVE_ENOUGH_DATA);
+                testExpected("mediaElement.paused", false);
+
+                run("mediaElement.pause()");
+                play();
+            });
+
+            waitForEvent('canplaythrough', function() {
+                run("mediaElement.play()");
+            });
+        },
     ];
 
     function start()

third_party/WebKit/LayoutTests/media/track/track-cues-pause-on-exit-expected.txt

@@ -1,4 +1,3 @@
-CONSOLE ERROR: Uncaught (in promise) AbortError: The play() request was interrupted by a call to pause().
 Tests that the video is paused after cues that have pause-on-exit flag are processed
 
 EVENT(canplaythrough)

third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button-expected.txt

@@ -1,4 +1,3 @@
-CONSOLE ERROR: Uncaught (in promise) AbortError: The play() request was interrupted by a call to pause().
 Test that the overlay play button respects the controls attribute
 
 EXPECTED (getComputedStyle(button).display == 'flex') OK

third_party/WebKit/LayoutTests/media/video-played-collapse-expected.txt

@@ -1,6 +1,3 @@
-CONSOLE ERROR: Uncaught (in promise) AbortError: The play() request was interrupted by a call to pause().
-CONSOLE ERROR: Uncaught (in promise) AbortError: The play() request was interrupted by a call to pause().
-CONSOLE ERROR: Uncaught (in promise) AbortError: The play() request was interrupted by a call to pause().
 Test of the media element 'played' attribute
 
 EVENT(loadstart)

third_party/WebKit/LayoutTests/media/video-played-ranges-1-expected.txt

@@ -1,7 +1,3 @@
-CONSOLE ERROR: Uncaught (in promise) AbortError: The play() request was interrupted by a call to pause().
-CONSOLE ERROR: Uncaught (in promise) AbortError: The play() request was interrupted by a call to pause().
-CONSOLE ERROR: Uncaught (in promise) AbortError: The play() request was interrupted by a call to pause().
-CONSOLE ERROR: Uncaught (in promise) AbortError: The play() request was interrupted by a call to pause().
 Test of the media element 'played' attribute, ranges part 1.
 
 EVENT(loadstart)

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

@@ -444,8 +444,8 @@ HTMLMediaElement::HTMLMediaElement(const QualifiedName& tagName, Document& docum
     , m_audioTracks(AudioTrackList::create(*this))
     , m_videoTracks(VideoTrackList::create(*this))
     , m_textTracks(nullptr)
-    , m_playPromiseResolveTask(CancellableTaskFactory::create(this, &HTMLMediaElement::resolvePlayPromises))
-    , m_playPromiseRejectTask(CancellableTaskFactory::create(this, &HTMLMediaElement::rejectPlayPromises))
+    , m_playPromiseResolveTask(CancellableTaskFactory::create(this, &HTMLMediaElement::resolveScheduledPlayPromises))
+    , m_playPromiseRejectTask(CancellableTaskFactory::create(this, &HTMLMediaElement::rejectScheduledPlayPromises))
     , m_audioSourceNode(nullptr)
     , m_autoplayHelperClient(AutoplayHelperClientImpl::create(this))
     , m_autoplayHelper(AutoplayExperimentHelper::create(m_autoplayHelperClient.get()))
@@ -1404,7 +1404,9 @@ void HTMLMediaElement::cancelPendingEventsAndCallbacks()
         source->cancelPendingErrorEvent();
 
     m_playPromiseResolveTask->cancel();
+    m_playPromiseResolveList.clear();
     m_playPromiseRejectTask->cancel();
+    m_playPromiseRejectList.clear();
 }
 
 void HTMLMediaElement::networkStateChanged()
@@ -2029,8 +2031,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
+    // does 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);
+
     Nullable<ExceptionCode> code = play();
     if (!code.isNull()) {
+        DCHECK(!m_playPromiseResolvers.isEmpty());
+        m_playPromiseResolvers.removeLast();
+
         String message;
         switch (code.get()) {
         case NotAllowedError:
@@ -2042,13 +2056,10 @@ ScriptPromise HTMLMediaElement::playForBindings(ScriptState* scriptState)
         default:
             ASSERT_NOT_REACHED();
         }
-        return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(code.get(), message));
+        resolver->reject(DOMException::create(code.get(), message));
+        return promise;
     }
 
-    ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
-    ScriptPromise promise = resolver->promise();
-
-    m_playResolvers.append(resolver);
     return promise;
 }
 
@@ -3611,7 +3622,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);
@@ -3708,10 +3721,19 @@ 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
+    // latter approach is preferred because it might be the less observable
+    // change.
+    DCHECK(m_playPromiseResolveList.isEmpty() || m_playPromiseResolveTask->isPending());
+    if (m_playPromiseResolvers.isEmpty())
+        return;
+
+    m_playPromiseResolveList.appendVector(m_playPromiseResolvers);
+    m_playPromiseResolvers.clear();
+
     if (m_playPromiseResolveTask->isPending())
         return;
 
@@ -3720,10 +3742,19 @@ 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
+    // latter approach is preferred because it might be the less observable
+    // change.
+    DCHECK(m_playPromiseRejectList.isEmpty() || m_playPromiseRejectTask->isPending());
+    if (m_playPromiseResolvers.isEmpty())
+        return;
+
+    m_playPromiseRejectList.appendVector(m_playPromiseResolvers);
+    m_playPromiseResolvers.clear();
+
     if (m_playPromiseRejectTask->isPending())
         return;
 
@@ -3739,34 +3770,41 @@ void HTMLMediaElement::scheduleNotifyPlaying()
     scheduleResolvePlayPromises();
 }
 
-void HTMLMediaElement::resolvePlayPromises()
+void HTMLMediaElement::resolveScheduledPlayPromises()
 {
-    for (auto& resolver: m_playResolvers)
+    for (auto& resolver: m_playPromiseResolveList)
         resolver->resolve();
 
-    m_playResolvers.clear();
+    m_playPromiseResolveList.clear();
 }
 
-void HTMLMediaElement::rejectPlayPromises()
+void HTMLMediaElement::rejectScheduledPlayPromises()
 {
     // TODO(mlamouri): the message is generated based on the code because
     // arguments can't be passed to a cancellable task. In order to save space
     // used by the object, the string isn't saved.
-    ASSERT(m_playPromiseErrorCode == AbortError || m_playPromiseErrorCode == NotSupportedError);
+    DCHECK(m_playPromiseErrorCode == AbortError || m_playPromiseErrorCode == NotSupportedError);
     if (m_playPromiseErrorCode == AbortError)
-        rejectPlayPromises(AbortError, "The play() request was interrupted by a call to pause().");
+        rejectPlayPromisesInternal(AbortError, "The play() request was interrupted by a call to pause().");
     else
-        rejectPlayPromises(NotSupportedError, "Failed to load because no supported source was found.");
+        rejectPlayPromisesInternal(NotSupportedError, "Failed to load because no supported source was found.");
 }
 
 void HTMLMediaElement::rejectPlayPromises(ExceptionCode code, const String& message)
 {
-    ASSERT(code == AbortError || code == NotSupportedError);
+    m_playPromiseRejectList.appendVector(m_playPromiseResolvers);
+    m_playPromiseResolvers.clear();
+    rejectPlayPromisesInternal(code, message);
+}
+
+void HTMLMediaElement::rejectPlayPromisesInternal(ExceptionCode code, const String& message)
+{
+    DCHECK(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();
 }
 
 EnumerationHistogram& HTMLMediaElement::showControlsHistogram() const

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

@@ -478,12 +478,10 @@ class CORE_EXPORT HTMLMediaElement : public HTMLElement, public Supplementable<H
     void scheduleResolvePlayPromises();
     void scheduleRejectPlayPromises(ExceptionCode);
     void scheduleNotifyPlaying();
-
-    void resolvePlayPromises();
-    // TODO(mlamouri): this is used for cancellable tasks because we can't pass
-    // parameters.
-    void rejectPlayPromises();
+    void resolveScheduledPlayPromises();
+    void rejectScheduledPlayPromises();
     void rejectPlayPromises(ExceptionCode, const String&);
+    void rejectPlayPromisesInternal(ExceptionCode, const String&);
 
     EnumerationHistogram& showControlsHistogram() const;
 
@@ -593,9 +591,11 @@ class CORE_EXPORT HTMLMediaElement : public HTMLElement, public Supplementable<H
 
     Member<CueTimeline> m_cueTimeline;
 
-    HeapVector<Member<ScriptPromiseResolver>> m_playResolvers;
+    HeapVector<Member<ScriptPromiseResolver>> m_playPromiseResolvers;
     OwnPtr<CancellableTaskFactory> m_playPromiseResolveTask;
     OwnPtr<CancellableTaskFactory> m_playPromiseRejectTask;
+    HeapVector<Member<ScriptPromiseResolver>> m_playPromiseResolveList;
+    HeapVector<Member<ScriptPromiseResolver>> m_playPromiseRejectList;
     ExceptionCode m_playPromiseErrorCode;
 
     // This is a weak reference, since m_audioSourceNode holds a reference to us.