Playing media elements which call "pause(); play()" will have the play promise rejected.
authorjer.noble@apple.com <jer.noble@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 18 Dec 2017 17:43:50 +0000 (17:43 +0000)
committerjer.noble@apple.com <jer.noble@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 18 Dec 2017 17:43:50 +0000 (17:43 +0000)
https://bugs.webkit.org/show_bug.cgi?id=180781

Reviewed by Eric Carlson.

Source/WebCore:

Test: media/video-pause-play-resolve.html

When scheduling a rejection or resolution of existing play promises, move() the existing
promises into the block. This ensures that valid promises aren't added to the play promise
vector between when a rejection is scheduled and when it runs.

Drive-by fix: Don't return false from playInternal() just so the newly created promise will
get rejected. The pause() command will reject the promise, so just make sure it's added to
the m_pendingPlayPromises before calling playInternal().

Drive-by fix #2: The spec referenced by playInternal() and pauseInternal() doesn't say to
call the "Media Element Load Algorithm" (i.e., prepareForLoad()); it says to call the
"Resource Selection Algorithm" (i.e., selectMediaResource()). But fixing this bug caused
an assertion crash when the resource selection task was fired and m_player was null. This
was because the algorithm is being run at stop() time due to stop() calling pause(). The
solution to this ASSERT is to stop the m_resourceSelectionTaskQueue in stop().

* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::scheduleRejectPendingPlayPromises):
(WebCore::HTMLMediaElement::rejectPendingPlayPromises):
(WebCore::HTMLMediaElement::resolvePendingPlayPromises):
(WebCore::HTMLMediaElement::scheduleNotifyAboutPlaying):
(WebCore::HTMLMediaElement::notifyAboutPlaying):
(WebCore::HTMLMediaElement::noneSupported):
(WebCore::HTMLMediaElement::cancelPendingEventsAndCallbacks):
(WebCore::HTMLMediaElement::play):
(WebCore::HTMLMediaElement::playInternal):
(WebCore::HTMLMediaElement::pauseInternal):
(WebCore::HTMLMediaElement::stop):
* html/HTMLMediaElement.h:

LayoutTests:

* media/audio-dealloc-crash.html:
* media/video-pause-play-resolve-expected.txt: Added.
* media/video-pause-play-resolve.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@226059 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/http/tests/security/video-cross-origin-caching-expected.txt
LayoutTests/media/audio-dealloc-crash.html
LayoutTests/media/video-pause-play-resolve-expected.txt [new file with mode: 0644]
LayoutTests/media/video-pause-play-resolve.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/html/HTMLMediaElement.cpp
Source/WebCore/html/HTMLMediaElement.h

index a6d118d..1c95f33 100644 (file)
@@ -1,3 +1,14 @@
+2017-12-18  Jer Noble  <jer.noble@apple.com>
+
+        Playing media elements which call "pause(); play()" will have the play promise rejected.
+        https://bugs.webkit.org/show_bug.cgi?id=180781
+
+        Reviewed by Eric Carlson.
+
+        * media/audio-dealloc-crash.html:
+        * media/video-pause-play-resolve-expected.txt: Added.
+        * media/video-pause-play-resolve.html: Added.
+
 2017-12-18  Daniel Bates  <dabates@apple.com>
 
         Add a test to ensure that matched text markers are not highlighted when highlight is disabled
index c9f47bd..a272198 100644 (file)
@@ -1,4 +1,3 @@
-CONSOLE MESSAGE: Unhandled Promise Rejection: AbortError: The operation was aborted.
 
 This test passes if you do not see a CORS error.
 
index 869109e..3d8dd26 100644 (file)
@@ -9,7 +9,7 @@
     <script>
     runWithKeyDown(() => {
         document.body.innerHTML = '<audio></audio>';
-        document.body.childNodes[0].play();
+        document.body.childNodes[0].play().catch(error => {});
         document.body.innerHTML = '';
         gc();
         setTimeout(endTest, 100);
diff --git a/LayoutTests/media/video-pause-play-resolve-expected.txt b/LayoutTests/media/video-pause-play-resolve-expected.txt
new file mode 100644 (file)
index 0000000..c2442bb
--- /dev/null
@@ -0,0 +1,10 @@
+
+RUN(mediaElement.src = findMediaFile('video', 'content/test'))
+EVENT(canplaythrough)
+RUN(promise = video.play())
+Promise resolved OK
+RUN(video.pause())
+RUN(promise = video.play())
+Promise resolved OK
+END OF TEST
+
diff --git a/LayoutTests/media/video-pause-play-resolve.html b/LayoutTests/media/video-pause-play-resolve.html
new file mode 100644 (file)
index 0000000..10a3454
--- /dev/null
@@ -0,0 +1,36 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <title>video-pause-play-resolve</title>
+    <script src=media-file.js></script>
+    <script src=video-test.js></script>
+
+    <script>
+    var promise;
+    function runTest()
+    {
+        findMediaElement();
+        run("mediaElement.src = findMediaFile('video', 'content/test')");
+        waitForEvent('canplaythrough', canplaythrough);
+    }
+
+    function canplaythrough()
+    {
+        run("promise = video.play()");
+        shouldResolve(promise).then(playing, endTest);
+    }
+
+    function playing()
+    {
+        run("video.pause()");
+        run("promise = video.play()");
+        shouldResolve(promise).then(endTest, endTest);
+    }
+    </script>
+</head>
+
+<body onload="runTest()">
+    <video controls></video>
+</body>
+</html>
+
index f8486f9..ddecb85 100644 (file)
@@ -1,3 +1,41 @@
+2017-12-18  Jer Noble  <jer.noble@apple.com>
+
+        Playing media elements which call "pause(); play()" will have the play promise rejected.
+        https://bugs.webkit.org/show_bug.cgi?id=180781
+
+        Reviewed by Eric Carlson.
+
+        Test: media/video-pause-play-resolve.html
+
+        When scheduling a rejection or resolution of existing play promises, move() the existing
+        promises into the block. This ensures that valid promises aren't added to the play promise
+        vector between when a rejection is scheduled and when it runs.
+
+        Drive-by fix: Don't return false from playInternal() just so the newly created promise will
+        get rejected. The pause() command will reject the promise, so just make sure it's added to
+        the m_pendingPlayPromises before calling playInternal().
+
+        Drive-by fix #2: The spec referenced by playInternal() and pauseInternal() doesn't say to
+        call the "Media Element Load Algorithm" (i.e., prepareForLoad()); it says to call the
+        "Resource Selection Algorithm" (i.e., selectMediaResource()). But fixing this bug caused
+        an assertion crash when the resource selection task was fired and m_player was null. This
+        was because the algorithm is being run at stop() time due to stop() calling pause(). The
+        solution to this ASSERT is to stop the m_resourceSelectionTaskQueue in stop().
+
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::scheduleRejectPendingPlayPromises):
+        (WebCore::HTMLMediaElement::rejectPendingPlayPromises):
+        (WebCore::HTMLMediaElement::resolvePendingPlayPromises):
+        (WebCore::HTMLMediaElement::scheduleNotifyAboutPlaying):
+        (WebCore::HTMLMediaElement::notifyAboutPlaying):
+        (WebCore::HTMLMediaElement::noneSupported):
+        (WebCore::HTMLMediaElement::cancelPendingEventsAndCallbacks):
+        (WebCore::HTMLMediaElement::play):
+        (WebCore::HTMLMediaElement::playInternal):
+        (WebCore::HTMLMediaElement::pauseInternal):
+        (WebCore::HTMLMediaElement::stop):
+        * html/HTMLMediaElement.h:
+
 2017-12-18  Daniel Bates  <dabates@apple.com>
 
         Add SPI to query for the current and last auto fill button type and pass user data object to _webView:focusShouldStartInputSession:
index 1252cbe..1dfb4e1 100644 (file)
@@ -1072,36 +1072,43 @@ void HTMLMediaElement::scheduleEvent(const AtomicString& eventName)
 
 void HTMLMediaElement::scheduleResolvePendingPlayPromises()
 {
-    m_promiseTaskQueue.enqueueTask(std::bind(&HTMLMediaElement::resolvePendingPlayPromises, this));
+    m_promiseTaskQueue.enqueueTask([this, pendingPlayPromises = WTFMove(m_pendingPlayPromises)] () mutable {
+        resolvePendingPlayPromises(WTFMove(pendingPlayPromises));
+    });
 }
 
-void HTMLMediaElement::rejectPendingPlayPromises(DOMException& error)
+void HTMLMediaElement::scheduleRejectPendingPlayPromises(Ref<DOMException>&& error)
 {
-    Vector<DOMPromiseDeferred<void>> pendingPlayPromises = WTFMove(m_pendingPlayPromises);
+    m_promiseTaskQueue.enqueueTask([this, error = WTFMove(error), pendingPlayPromises = WTFMove(m_pendingPlayPromises)] () mutable {
+        rejectPendingPlayPromises(WTFMove(pendingPlayPromises), WTFMove(error));
+    });
+}
 
+void HTMLMediaElement::rejectPendingPlayPromises(PlayPromiseVector&& pendingPlayPromises, Ref<DOMException>&& error)
+{
     for (auto& promise : pendingPlayPromises)
         promise.rejectType<IDLInterface<DOMException>>(error);
 }
 
-void HTMLMediaElement::resolvePendingPlayPromises()
+void HTMLMediaElement::resolvePendingPlayPromises(PlayPromiseVector&& pendingPlayPromises)
 {
-    Vector<DOMPromiseDeferred<void>> pendingPlayPromises = WTFMove(m_pendingPlayPromises);
-
     for (auto& promise : pendingPlayPromises)
         promise.resolve();
 }
 
 void HTMLMediaElement::scheduleNotifyAboutPlaying()
 {
-    m_promiseTaskQueue.enqueueTask(std::bind(&HTMLMediaElement::notifyAboutPlaying, this));
+    m_promiseTaskQueue.enqueueTask([this, pendingPlayPromises = WTFMove(m_pendingPlayPromises)] () mutable {
+        notifyAboutPlaying(WTFMove(pendingPlayPromises));
+    });
 }
 
-void HTMLMediaElement::notifyAboutPlaying()
+void HTMLMediaElement::notifyAboutPlaying(PlayPromiseVector&& pendingPlayPromises)
 {
     Ref<HTMLMediaElement> protectedThis(*this); // The 'playing' event can make arbitrary DOM mutations.
     m_playbackStartedTime = currentMediaTime().toDouble();
     dispatchEvent(Event::create(eventNames().playingEvent, false, true));
-    resolvePendingPlayPromises();
+    resolvePendingPlayPromises(WTFMove(pendingPlayPromises));
 
     m_hasEverNotifiedAboutPlaying = true;
     scheduleUpdatePlaybackControlsManager();
@@ -2168,7 +2175,7 @@ void HTMLMediaElement::noneSupported()
     // 7 - Queue a task to fire a simple event named error at the media element.
     scheduleEvent(eventNames().errorEvent);
 
-    rejectPendingPlayPromises(DOMException::create(NotSupportedError));
+    rejectPendingPlayPromises(WTFMove(m_pendingPlayPromises), DOMException::create(NotSupportedError));
 
 #if ENABLE(MEDIA_SOURCE)
     detachMediaSource();
@@ -2231,7 +2238,7 @@ void HTMLMediaElement::cancelPendingEventsAndCallbacks()
     for (auto& source : childrenOfType<HTMLSourceElement>(*this))
         source.cancelPendingErrorEvent();
 
-    rejectPendingPlayPromises(DOMException::create(AbortError));
+    rejectPendingPlayPromises(WTFMove(m_pendingPlayPromises), DOMException::create(AbortError));
 }
 
 void HTMLMediaElement::mediaPlayerNetworkStateChanged(MediaPlayer*)
@@ -3418,12 +3425,8 @@ void HTMLMediaElement::play(DOMPromiseDeferred<void>&& promise)
     if (processingUserGestureForMedia())
         removeBehaviorsRestrictionsAfterFirstUserGesture();
 
-    if (!playInternal()) {
-        promise.reject(NotAllowedError);
-        return;
-    }
-
     m_pendingPlayPromises.append(WTFMove(promise));
+    playInternal();
 }
 
 void HTMLMediaElement::play()
@@ -3442,18 +3445,18 @@ void HTMLMediaElement::play()
     playInternal();
 }
 
-bool HTMLMediaElement::playInternal()
+void HTMLMediaElement::playInternal()
 {
     ALWAYS_LOG(LOGIDENTIFIER);
 
     if (!m_mediaSession->clientWillBeginPlayback()) {
         ALWAYS_LOG(LOGIDENTIFIER, "  returning because of interruption");
-        return true; // Treat as success because we will begin playback on cessation of the interruption.
+        return; // Treat as success because we will begin playback on cessation of the interruption.
     }
 
     // 4.8.10.9. Playing the media resource
     if (!m_player || m_networkState == NETWORK_EMPTY)
-        prepareForLoad();
+        selectMediaResource();
 
     if (endedPlayback())
         seekInternal(MediaTime::zeroTime());
@@ -3467,11 +3470,6 @@ bool HTMLMediaElement::playInternal()
         m_playbackStartedTime = currentMediaTime().toDouble();
         scheduleEvent(eventNames().playEvent);
 
-        if (m_readyState <= HAVE_CURRENT_DATA)
-            scheduleEvent(eventNames().waitingEvent);
-        else if (m_readyState >= HAVE_FUTURE_DATA)
-            scheduleNotifyAboutPlaying();
-
 #if ENABLE(MEDIA_SESSION)
         // 6.3 Activating a media session from a media element
         // When the play() method is invoked, the paused attribute is true, and the readyState attribute has the value
@@ -3492,11 +3490,15 @@ bool HTMLMediaElement::playInternal()
 
                 if (!m_session->invoke()) {
                     pause();
-                    return false;
+                    return;
                 }
             }
         }
 #endif
+        if (m_readyState <= HAVE_CURRENT_DATA)
+            scheduleEvent(eventNames().waitingEvent);
+        else if (m_readyState >= HAVE_FUTURE_DATA)
+            scheduleNotifyAboutPlaying();
     } else if (m_readyState >= HAVE_FUTURE_DATA)
         scheduleResolvePendingPlayPromises();
 
@@ -3510,8 +3512,6 @@ bool HTMLMediaElement::playInternal()
 
     m_autoplaying = false;
     updatePlayState();
-
-    return true;
 }
 
 void HTMLMediaElement::pause()
@@ -3545,7 +3545,7 @@ void HTMLMediaElement::pauseInternal()
         // don't trigger loading if a script calls pause().
         if (!m_mediaSession->playbackPermitted(*this))
             return;
-        prepareForLoad();
+        selectMediaResource();
     }
 
     m_autoplaying = false;
@@ -3559,9 +3559,7 @@ void HTMLMediaElement::pauseInternal()
         m_paused = true;
         scheduleTimeupdateEvent(false);
         scheduleEvent(eventNames().pauseEvent);
-        m_promiseTaskQueue.enqueueTask([this]() {
-            rejectPendingPlayPromises(DOMException::create(AbortError));
-        });
+        scheduleRejectPendingPlayPromises(DOMException::create(AbortError));
         if (MemoryPressureHandler::singleton().isUnderMemoryPressure())
             purgeBufferedDataIfPossible();
     }
@@ -5531,6 +5529,7 @@ void HTMLMediaElement::stop()
     m_asyncEventQueue.close();
     m_promiseTaskQueue.close();
     m_updatePlaybackControlsManagerQueue.close();
+    m_resourceSelectionTaskQueue.close();
 
     // Once an active DOM object has been stopped it can not be restarted, so we can deallocate
     // the media player now. Note that userCancelledLoad will already called clearMediaPlayer
index bf2f73b..1128f9e 100644 (file)
@@ -183,10 +183,12 @@ public:
     using HTMLMediaElementEnums::DelayedActionType;
     void scheduleDelayedAction(DelayedActionType);
     void scheduleResolvePendingPlayPromises();
-    void rejectPendingPlayPromises(DOMException&);
-    void resolvePendingPlayPromises();
+    void scheduleRejectPendingPlayPromises(Ref<DOMException>&&);
+    using PlayPromiseVector = Vector<DOMPromiseDeferred<void>>;
+    void rejectPendingPlayPromises(PlayPromiseVector&&, Ref<DOMException>&&);
+    void resolvePendingPlayPromises(PlayPromiseVector&&);
     void scheduleNotifyAboutPlaying();
-    void notifyAboutPlaying();
+    void notifyAboutPlaying(PlayPromiseVector&&);
     
     MediaPlayerEnums::MovieLoadType movieLoadType() const;
     
@@ -770,7 +772,7 @@ private:
 #endif
 
     // These "internal" functions do not check user gesture restrictions.
-    bool playInternal();
+    void playInternal();
     void pauseInternal();
 
     void prepareForLoad();
@@ -926,7 +928,7 @@ private:
     RefPtr<TimeRanges> m_playedTimeRanges;
     GenericEventQueue m_asyncEventQueue;
 
-    Vector<DOMPromiseDeferred<void>> m_pendingPlayPromises;
+    PlayPromiseVector m_pendingPlayPromises;
 
     double m_requestedPlaybackRate { 1 };
     double m_reportedPlaybackRate { 1 };