From b62b49b650018c1367069a6c8e2a049e998fc4d9 Mon Sep 17 00:00:00 2001 From: Mounir Lamouri Date: Fri, 10 Jun 2016 11:34:20 +0100 Subject: [PATCH] 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 TBR=avayvod@chromium.org Review-Url: https://codereview.chromium.org/1865933002 Cr-Commit-Position: refs/heads/master@{#398370} (cherry picked from commit 3a82d82079ca79cddd932eb77f924db5428885b9) Review URL: https://codereview.chromium.org/2053333002 . Cr-Commit-Position: refs/branch-heads/2743@{#311} Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939} --- .../media/media-play-promise-expected.txt | 15 ++++ .../LayoutTests/media/media-play-promise.html | 33 ++++++- .../track-cues-pause-on-exit-expected.txt | 1 - ...-controls-overlay-play-button-expected.txt | 1 - .../media/video-played-collapse-expected.txt | 3 - .../media/video-played-ranges-1-expected.txt | 4 - .../Source/core/html/HTMLMediaElement.cpp | 90 +++++++++++++------ .../Source/core/html/HTMLMediaElement.h | 12 +-- 8 files changed, 115 insertions(+), 44 deletions(-) diff --git a/third_party/WebKit/LayoutTests/media/media-play-promise-expected.txt b/third_party/WebKit/LayoutTests/media/media-play-promise-expected.txt index 3cf066835ddd1..66d8677c324ac 100644 --- a/third_party/WebKit/LayoutTests/media/media-play-promise-expected.txt +++ b/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 diff --git a/third_party/WebKit/LayoutTests/media/media-play-promise.html b/third_party/WebKit/LayoutTests/media/media-play-promise.html index 77b4ac76d873b..992b0b0654d02 100644 --- a/third_party/WebKit/LayoutTests/media/media-play-promise.html +++ b/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() diff --git a/third_party/WebKit/LayoutTests/media/track/track-cues-pause-on-exit-expected.txt b/third_party/WebKit/LayoutTests/media/track/track-cues-pause-on-exit-expected.txt index 4deaded51e524..7fd3e15c55da3 100644 --- a/third_party/WebKit/LayoutTests/media/track/track-cues-pause-on-exit-expected.txt +++ b/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) diff --git a/third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button-expected.txt b/third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button-expected.txt index eb10c6a2cb2e7..277b5dba4e119 100644 --- a/third_party/WebKit/LayoutTests/media/video-controls-overlay-play-button-expected.txt +++ b/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 diff --git a/third_party/WebKit/LayoutTests/media/video-played-collapse-expected.txt b/third_party/WebKit/LayoutTests/media/video-played-collapse-expected.txt index 7af7151890405..02533c6861d11 100644 --- a/third_party/WebKit/LayoutTests/media/video-played-collapse-expected.txt +++ b/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) diff --git a/third_party/WebKit/LayoutTests/media/video-played-ranges-1-expected.txt b/third_party/WebKit/LayoutTests/media/video-played-ranges-1-expected.txt index 9bcc4894e9832..eeb5fd07896bd 100644 --- a/third_party/WebKit/LayoutTests/media/video-played-ranges-1-expected.txt +++ b/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) diff --git a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp index 246e0c42c49ca..497fc9d504d7e 100644 --- a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp +++ b/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 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 diff --git a/third_party/WebKit/Source/core/html/HTMLMediaElement.h b/third_party/WebKit/Source/core/html/HTMLMediaElement.h index 3aad6e757f7bc..7166e16b7b772 100644 --- a/third_party/WebKit/Source/core/html/HTMLMediaElement.h +++ b/third_party/WebKit/Source/core/html/HTMLMediaElement.h @@ -478,12 +478,10 @@ class CORE_EXPORT HTMLMediaElement : public HTMLElement, public Supplementable m_cueTimeline; - HeapVector> m_playResolvers; + HeapVector> m_playPromiseResolvers; OwnPtr m_playPromiseResolveTask; OwnPtr m_playPromiseRejectTask; + HeapVector> m_playPromiseResolveList; + HeapVector> m_playPromiseRejectList; ExceptionCode m_playPromiseErrorCode; // This is a weak reference, since m_audioSourceNode holds a reference to us.