- Revision
- 226059
- Author
- jer.no...@apple.com
- Date
- 2017-12-18 09:43:50 -0800 (Mon, 18 Dec 2017)
Log Message
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.
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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (226058 => 226059)
--- trunk/LayoutTests/ChangeLog 2017-12-18 17:42:53 UTC (rev 226058)
+++ trunk/LayoutTests/ChangeLog 2017-12-18 17:43:50 UTC (rev 226059)
@@ -1,3 +1,14 @@
+2017-12-18 Jer Noble <jer.no...@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 <daba...@apple.com>
Add a test to ensure that matched text markers are not highlighted when highlight is disabled
Modified: trunk/LayoutTests/http/tests/security/video-cross-origin-caching-expected.txt (226058 => 226059)
--- trunk/LayoutTests/http/tests/security/video-cross-origin-caching-expected.txt 2017-12-18 17:42:53 UTC (rev 226058)
+++ trunk/LayoutTests/http/tests/security/video-cross-origin-caching-expected.txt 2017-12-18 17:43:50 UTC (rev 226059)
@@ -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.
Modified: trunk/LayoutTests/media/audio-dealloc-crash.html (226058 => 226059)
--- trunk/LayoutTests/media/audio-dealloc-crash.html 2017-12-18 17:42:53 UTC (rev 226058)
+++ trunk/LayoutTests/media/audio-dealloc-crash.html 2017-12-18 17:43:50 UTC (rev 226059)
@@ -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);
Added: trunk/LayoutTests/media/video-pause-play-resolve-expected.txt (0 => 226059)
--- trunk/LayoutTests/media/video-pause-play-resolve-expected.txt (rev 0)
+++ trunk/LayoutTests/media/video-pause-play-resolve-expected.txt 2017-12-18 17:43:50 UTC (rev 226059)
@@ -0,0 +1,10 @@
+
+RUN(mediaElement.src = "" 'content/test'))
+EVENT(canplaythrough)
+RUN(promise = video.play())
+Promise resolved OK
+RUN(video.pause())
+RUN(promise = video.play())
+Promise resolved OK
+END OF TEST
+
Added: trunk/LayoutTests/media/video-pause-play-resolve.html (0 => 226059)
--- trunk/LayoutTests/media/video-pause-play-resolve.html (rev 0)
+++ trunk/LayoutTests/media/video-pause-play-resolve.html 2017-12-18 17:43:50 UTC (rev 226059)
@@ -0,0 +1,36 @@
+<!DOCTYPE html>
+<html>
+<head>
+ <title>video-pause-play-resolve</title>
+ <script src=""
+ <script src=""
+
+ <script>
+ var promise;
+ function runTest()
+ {
+ findMediaElement();
+ run("mediaElement.src = "" '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>
+
Modified: trunk/Source/WebCore/ChangeLog (226058 => 226059)
--- trunk/Source/WebCore/ChangeLog 2017-12-18 17:42:53 UTC (rev 226058)
+++ trunk/Source/WebCore/ChangeLog 2017-12-18 17:43:50 UTC (rev 226059)
@@ -1,3 +1,41 @@
+2017-12-18 Jer Noble <jer.no...@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 <daba...@apple.com>
Add SPI to query for the current and last auto fill button type and pass user data object to _webView:focusShouldStartInputSession:
Modified: trunk/Source/WebCore/html/HTMLMediaElement.cpp (226058 => 226059)
--- trunk/Source/WebCore/html/HTMLMediaElement.cpp 2017-12-18 17:42:53 UTC (rev 226058)
+++ trunk/Source/WebCore/html/HTMLMediaElement.cpp 2017-12-18 17:43:50 UTC (rev 226059)
@@ -1072,21 +1072,26 @@
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();
}
@@ -1093,15 +1098,17 @@
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 @@
// 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 @@
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 @@
if (processingUserGestureForMedia())
removeBehaviorsRestrictionsAfterFirstUserGesture();
- if (!playInternal()) {
- promise.reject(NotAllowedError);
- return;
- }
-
m_pendingPlayPromises.append(WTFMove(promise));
+ playInternal();
}
void HTMLMediaElement::play()
@@ -3442,18 +3445,18 @@
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 @@
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 @@
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 @@
m_autoplaying = false;
updatePlayState();
-
- return true;
}
void HTMLMediaElement::pause()
@@ -3545,7 +3545,7 @@
// don't trigger loading if a script calls pause().
if (!m_mediaSession->playbackPermitted(*this))
return;
- prepareForLoad();
+ selectMediaResource();
}
m_autoplaying = false;
@@ -3559,9 +3559,7 @@
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 @@
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
Modified: trunk/Source/WebCore/html/HTMLMediaElement.h (226058 => 226059)
--- trunk/Source/WebCore/html/HTMLMediaElement.h 2017-12-18 17:42:53 UTC (rev 226058)
+++ trunk/Source/WebCore/html/HTMLMediaElement.h 2017-12-18 17:43:50 UTC (rev 226059)
@@ -183,10 +183,12 @@
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 @@
#endif
// These "internal" functions do not check user gesture restrictions.
- bool playInternal();
+ void playInternal();
void pauseInternal();
void prepareForLoad();
@@ -926,7 +928,7 @@
RefPtr<TimeRanges> m_playedTimeRanges;
GenericEventQueue m_asyncEventQueue;
- Vector<DOMPromiseDeferred<void>> m_pendingPlayPromises;
+ PlayPromiseVector m_pendingPlayPromises;
double m_requestedPlaybackRate { 1 };
double m_reportedPlaybackRate { 1 };
