Changeset 226059 in webkit


Ignore:
Timestamp:
Dec 18, 2017 9:43:50 AM (6 years ago)
Author:
jer.noble@apple.com
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.
Location:
trunk
Files:
2 added
6 edited

Legend:

Unmodified
Added
Removed
  • trunk/LayoutTests/ChangeLog

    r226054 r226059  
     12017-12-18  Jer Noble  <jer.noble@apple.com>
     2
     3        Playing media elements which call "pause(); play()" will have the play promise rejected.
     4        https://bugs.webkit.org/show_bug.cgi?id=180781
     5
     6        Reviewed by Eric Carlson.
     7
     8        * media/audio-dealloc-crash.html:
     9        * media/video-pause-play-resolve-expected.txt: Added.
     10        * media/video-pause-play-resolve.html: Added.
     11
    1122017-12-18  Daniel Bates  <dabates@apple.com>
    213
  • trunk/LayoutTests/http/tests/security/video-cross-origin-caching-expected.txt

    r222270 r226059  
    1 CONSOLE MESSAGE: Unhandled Promise Rejection: AbortError: The operation was aborted.
    21
    32This test passes if you do not see a CORS error.
  • trunk/LayoutTests/media/audio-dealloc-crash.html

    r211659 r226059  
    1010    runWithKeyDown(() => {
    1111        document.body.innerHTML = '<audio></audio>';
    12         document.body.childNodes[0].play();
     12        document.body.childNodes[0].play().catch(error => {});
    1313        document.body.innerHTML = '';
    1414        gc();
  • trunk/Source/WebCore/ChangeLog

    r226048 r226059  
     12017-12-18  Jer Noble  <jer.noble@apple.com>
     2
     3        Playing media elements which call "pause(); play()" will have the play promise rejected.
     4        https://bugs.webkit.org/show_bug.cgi?id=180781
     5
     6        Reviewed by Eric Carlson.
     7
     8        Test: media/video-pause-play-resolve.html
     9
     10        When scheduling a rejection or resolution of existing play promises, move() the existing
     11        promises into the block. This ensures that valid promises aren't added to the play promise
     12        vector between when a rejection is scheduled and when it runs.
     13
     14        Drive-by fix: Don't return false from playInternal() just so the newly created promise will
     15        get rejected. The pause() command will reject the promise, so just make sure it's added to
     16        the m_pendingPlayPromises before calling playInternal().
     17
     18        Drive-by fix #2: The spec referenced by playInternal() and pauseInternal() doesn't say to
     19        call the "Media Element Load Algorithm" (i.e., prepareForLoad()); it says to call the
     20        "Resource Selection Algorithm" (i.e., selectMediaResource()). But fixing this bug caused
     21        an assertion crash when the resource selection task was fired and m_player was null. This
     22        was because the algorithm is being run at stop() time due to stop() calling pause(). The
     23        solution to this ASSERT is to stop the m_resourceSelectionTaskQueue in stop().
     24
     25        * html/HTMLMediaElement.cpp:
     26        (WebCore::HTMLMediaElement::scheduleRejectPendingPlayPromises):
     27        (WebCore::HTMLMediaElement::rejectPendingPlayPromises):
     28        (WebCore::HTMLMediaElement::resolvePendingPlayPromises):
     29        (WebCore::HTMLMediaElement::scheduleNotifyAboutPlaying):
     30        (WebCore::HTMLMediaElement::notifyAboutPlaying):
     31        (WebCore::HTMLMediaElement::noneSupported):
     32        (WebCore::HTMLMediaElement::cancelPendingEventsAndCallbacks):
     33        (WebCore::HTMLMediaElement::play):
     34        (WebCore::HTMLMediaElement::playInternal):
     35        (WebCore::HTMLMediaElement::pauseInternal):
     36        (WebCore::HTMLMediaElement::stop):
     37        * html/HTMLMediaElement.h:
     38
    1392017-12-18  Daniel Bates  <dabates@apple.com>
    240
  • trunk/Source/WebCore/html/HTMLMediaElement.cpp

    r225830 r226059  
    10731073void HTMLMediaElement::scheduleResolvePendingPlayPromises()
    10741074{
    1075     m_promiseTaskQueue.enqueueTask(std::bind(&HTMLMediaElement::resolvePendingPlayPromises, this));
    1076 }
    1077 
    1078 void HTMLMediaElement::rejectPendingPlayPromises(DOMException& error)
    1079 {
    1080     Vector<DOMPromiseDeferred<void>> pendingPlayPromises = WTFMove(m_pendingPlayPromises);
    1081 
     1075    m_promiseTaskQueue.enqueueTask([this, pendingPlayPromises = WTFMove(m_pendingPlayPromises)] () mutable {
     1076        resolvePendingPlayPromises(WTFMove(pendingPlayPromises));
     1077    });
     1078}
     1079
     1080void HTMLMediaElement::scheduleRejectPendingPlayPromises(Ref<DOMException>&& error)
     1081{
     1082    m_promiseTaskQueue.enqueueTask([this, error = WTFMove(error), pendingPlayPromises = WTFMove(m_pendingPlayPromises)] () mutable {
     1083        rejectPendingPlayPromises(WTFMove(pendingPlayPromises), WTFMove(error));
     1084    });
     1085}
     1086
     1087void HTMLMediaElement::rejectPendingPlayPromises(PlayPromiseVector&& pendingPlayPromises, Ref<DOMException>&& error)
     1088{
    10821089    for (auto& promise : pendingPlayPromises)
    10831090        promise.rejectType<IDLInterface<DOMException>>(error);
    10841091}
    10851092
    1086 void HTMLMediaElement::resolvePendingPlayPromises()
    1087 {
    1088     Vector<DOMPromiseDeferred<void>> pendingPlayPromises = WTFMove(m_pendingPlayPromises);
    1089 
     1093void HTMLMediaElement::resolvePendingPlayPromises(PlayPromiseVector&& pendingPlayPromises)
     1094{
    10901095    for (auto& promise : pendingPlayPromises)
    10911096        promise.resolve();
     
    10941099void HTMLMediaElement::scheduleNotifyAboutPlaying()
    10951100{
    1096     m_promiseTaskQueue.enqueueTask(std::bind(&HTMLMediaElement::notifyAboutPlaying, this));
    1097 }
    1098 
    1099 void HTMLMediaElement::notifyAboutPlaying()
     1101    m_promiseTaskQueue.enqueueTask([this, pendingPlayPromises = WTFMove(m_pendingPlayPromises)] () mutable {
     1102        notifyAboutPlaying(WTFMove(pendingPlayPromises));
     1103    });
     1104}
     1105
     1106void HTMLMediaElement::notifyAboutPlaying(PlayPromiseVector&& pendingPlayPromises)
    11001107{
    11011108    Ref<HTMLMediaElement> protectedThis(*this); // The 'playing' event can make arbitrary DOM mutations.
    11021109    m_playbackStartedTime = currentMediaTime().toDouble();
    11031110    dispatchEvent(Event::create(eventNames().playingEvent, false, true));
    1104     resolvePendingPlayPromises();
     1111    resolvePendingPlayPromises(WTFMove(pendingPlayPromises));
    11051112
    11061113    m_hasEverNotifiedAboutPlaying = true;
     
    21692176    scheduleEvent(eventNames().errorEvent);
    21702177
    2171     rejectPendingPlayPromises(DOMException::create(NotSupportedError));
     2178    rejectPendingPlayPromises(WTFMove(m_pendingPlayPromises), DOMException::create(NotSupportedError));
    21722179
    21732180#if ENABLE(MEDIA_SOURCE)
     
    22322239        source.cancelPendingErrorEvent();
    22332240
    2234     rejectPendingPlayPromises(DOMException::create(AbortError));
     2241    rejectPendingPlayPromises(WTFMove(m_pendingPlayPromises), DOMException::create(AbortError));
    22352242}
    22362243
     
    34193426        removeBehaviorsRestrictionsAfterFirstUserGesture();
    34203427
    3421     if (!playInternal()) {
    3422         promise.reject(NotAllowedError);
    3423         return;
    3424     }
    3425 
    34263428    m_pendingPlayPromises.append(WTFMove(promise));
     3429    playInternal();
    34273430}
    34283431
     
    34433446}
    34443447
    3445 bool HTMLMediaElement::playInternal()
     3448void HTMLMediaElement::playInternal()
    34463449{
    34473450    ALWAYS_LOG(LOGIDENTIFIER);
     
    34493452    if (!m_mediaSession->clientWillBeginPlayback()) {
    34503453        ALWAYS_LOG(LOGIDENTIFIER, "  returning because of interruption");
    3451         return true; // Treat as success because we will begin playback on cessation of the interruption.
     3454        return; // Treat as success because we will begin playback on cessation of the interruption.
    34523455    }
    34533456
    34543457    // 4.8.10.9. Playing the media resource
    34553458    if (!m_player || m_networkState == NETWORK_EMPTY)
    3456         prepareForLoad();
     3459        selectMediaResource();
    34573460
    34583461    if (endedPlayback())
     
    34673470        m_playbackStartedTime = currentMediaTime().toDouble();
    34683471        scheduleEvent(eventNames().playEvent);
    3469 
    3470         if (m_readyState <= HAVE_CURRENT_DATA)
    3471             scheduleEvent(eventNames().waitingEvent);
    3472         else if (m_readyState >= HAVE_FUTURE_DATA)
    3473             scheduleNotifyAboutPlaying();
    34743472
    34753473#if ENABLE(MEDIA_SESSION)
     
    34933491                if (!m_session->invoke()) {
    34943492                    pause();
    3495                     return false;
     3493                    return;
    34963494                }
    34973495            }
    34983496        }
    34993497#endif
     3498        if (m_readyState <= HAVE_CURRENT_DATA)
     3499            scheduleEvent(eventNames().waitingEvent);
     3500        else if (m_readyState >= HAVE_FUTURE_DATA)
     3501            scheduleNotifyAboutPlaying();
    35003502    } else if (m_readyState >= HAVE_FUTURE_DATA)
    35013503        scheduleResolvePendingPlayPromises();
     
    35113513    m_autoplaying = false;
    35123514    updatePlayState();
    3513 
    3514     return true;
    35153515}
    35163516
     
    35463546        if (!m_mediaSession->playbackPermitted(*this))
    35473547            return;
    3548         prepareForLoad();
     3548        selectMediaResource();
    35493549    }
    35503550
     
    35603560        scheduleTimeupdateEvent(false);
    35613561        scheduleEvent(eventNames().pauseEvent);
    3562         m_promiseTaskQueue.enqueueTask([this]() {
    3563             rejectPendingPlayPromises(DOMException::create(AbortError));
    3564         });
     3562        scheduleRejectPendingPlayPromises(DOMException::create(AbortError));
    35653563        if (MemoryPressureHandler::singleton().isUnderMemoryPressure())
    35663564            purgeBufferedDataIfPossible();
     
    55325530    m_promiseTaskQueue.close();
    55335531    m_updatePlaybackControlsManagerQueue.close();
     5532    m_resourceSelectionTaskQueue.close();
    55345533
    55355534    // Once an active DOM object has been stopped it can not be restarted, so we can deallocate
  • trunk/Source/WebCore/html/HTMLMediaElement.h

    r225696 r226059  
    184184    void scheduleDelayedAction(DelayedActionType);
    185185    void scheduleResolvePendingPlayPromises();
    186     void rejectPendingPlayPromises(DOMException&);
    187     void resolvePendingPlayPromises();
     186    void scheduleRejectPendingPlayPromises(Ref<DOMException>&&);
     187    using PlayPromiseVector = Vector<DOMPromiseDeferred<void>>;
     188    void rejectPendingPlayPromises(PlayPromiseVector&&, Ref<DOMException>&&);
     189    void resolvePendingPlayPromises(PlayPromiseVector&&);
    188190    void scheduleNotifyAboutPlaying();
    189     void notifyAboutPlaying();
     191    void notifyAboutPlaying(PlayPromiseVector&&);
    190192   
    191193    MediaPlayerEnums::MovieLoadType movieLoadType() const;
     
    771773
    772774    // These "internal" functions do not check user gesture restrictions.
    773     bool playInternal();
     775    void playInternal();
    774776    void pauseInternal();
    775777
     
    927929    GenericEventQueue m_asyncEventQueue;
    928930
    929     Vector<DOMPromiseDeferred<void>> m_pendingPlayPromises;
     931    PlayPromiseVector m_pendingPlayPromises;
    930932
    931933    double m_requestedPlaybackRate { 1 };
Note: See TracChangeset for help on using the changeset viewer.