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

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

 /*
  * Copyright (C) 2007, 2008, 2009, 2010, 2011, 2012, 2013 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 416 matching lines @@
     , m_textTracksVisible(false)
     , m_shouldPerformAutomaticTrackSelection(true)
     , m_tracksAreReady(true)
     , m_processingPreferenceChange(false)
     , m_remoteRoutesAvailable(false)
     , m_playingRemotely(false)
     , m_inOverlayFullscreenVideo(false)
     , 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()))
     , m_remotePlaybackClient(nullptr)
 {
     ThreadState::current()->registerPreFinalizer(this);
 
     DVLOG(MEDIA_LOG_LEVEL) << "HTMLMediaElement(" << (void*)this << ")";
 
     // If any experiment is enabled, then we want to enable a user gesture by
@@ skipping 1565 matching lines @@
 
     WebMediaPlayer::Preload preload = preloadType();
     if (m_ignorePreloadNone && preload == WebMediaPlayer::PreloadNone)
         return WebMediaPlayer::PreloadMetaData;
 
     return preload;
 }
 
 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:
             message = "play() can only be initiated by a user gesture.";
             break;
         case NotSupportedError:
             message = "The element has no supported sources.";
             break;
         default:
             NOTREACHED();
         }
         return ScriptPromise::rejectWithDOMException(scriptState, DOMException::create(code.get(), message));
     }
 
-    ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
-    ScriptPromise promise = resolver->promise();
-
-    m_playResolvers.append(resolver);
     return promise;
 }
 
 Nullable<ExceptionCode> HTMLMediaElement::play()
 {
     DVLOG(MEDIA_LOG_LEVEL) << "play(" << (void*)this << ")";
 
     m_autoplayHelper->playMethodCalled();
 
     if (!UserGestureIndicator::processingUserGesture()) {
@@ skipping 1541 matching lines @@
     visitor->trace(m_asyncEventQueue);
     visitor->trace(m_error);
     visitor->trace(m_currentSourceNode);
     visitor->trace(m_nextChildNodeToConsider);
     visitor->trace(m_mediaSource);
     visitor->trace(m_audioTracks);
     visitor->trace(m_videoTracks);
     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);
     visitor->trace(m_srcObject);
     visitor->template registerWeakMembers<HTMLMediaElement, &HTMLMediaElement::clearWeakMembers>(this);
     Supplementable<HTMLMediaElement>::trace(visitor);
     HTMLElement::trace(visitor);
     ActiveDOMObject::trace(visitor);
 }
 
@@ skipping 76 matching lines @@
 // TODO(liberato): remove once autoplay gesture override experiment concludes.
 void HTMLMediaElement::triggerAutoplayViewportCheckForTesting()
 {
     if (FrameView* view = document().view())
         m_autoplayHelper->positionChanged(view->rootFrameToContents(view->computeVisibleArea()));
     m_autoplayHelper->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
+    // former approach is prefered because it might be the less observable

[fs, 2016/06/06 09:13:24]

Nit: preferred (also below)

[mlamouri (slow - plz ping), 2016/06/06 16:00:55]

Done.

+    // 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;
 
     Platform::current()->currentThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, m_playPromiseResolveTask->cancelAndCreate());
 }
 
 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
+    // former approach is prefered because it might be the less observable

[whywhat, 2016/06/06 13:08:57]

nit: the code seems to be doing the latter, not former. same above

[mlamouri (slow - plz ping), 2016/06/06 16:00:55]

Done.

+    // 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;
 
     // TODO(mlamouri): because cancellable tasks can't take parameters, the
     // error code needs to be saved.
     m_playPromiseErrorCode = code;

[whywhat, 2016/06/06 13:08:57]

nit: should we update the current code for the pending task?
what if this method is called with two different codes? would we reject all the
promises with the first one only?

     Platform::current()->currentThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, m_playPromiseRejectTask->cancelAndCreate());
 }
 
 void HTMLMediaElement::scheduleNotifyPlaying()
 {
     scheduleEvent(EventTypeNames::playing);
     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.
     DCHECK(m_playPromiseErrorCode == AbortError || m_playPromiseErrorCode == NotSupportedError);
     if (m_playPromiseErrorCode == AbortError)
         rejectPlayPromises(AbortError, "The play() request was interrupted by a call to pause().");
     else
         rejectPlayPromises(NotSupportedError, "Failed to load because no supported source was found.");
 }
 
 void HTMLMediaElement::rejectPlayPromises(ExceptionCode code, const String& message)
 {
+    m_playPromiseRejectList.appendVector(m_playPromiseResolvers);
+    m_playPromiseResolvers.clear();
+    rejectPlayPromisesInternal(code, message);
+}
+
+void HTMLMediaElement::rejectPlayPromisesInternal(ExceptionCode code, const String& message)

[whywhat, 2016/06/06 13:08:57]

nit: seems like this method could be inlined

[mlamouri (slow - plz ping), 2016/06/06 16:00:55]

No. I forgot to use it in `rejectScheduledPlayPromises`. Thanks! :)

+{
     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
 {
     if (isHTMLVideoElement()) {
         DEFINE_STATIC_LOCAL(EnumerationHistogram, histogram, ("Media.Controls.Show.Video", MediaControlsShowMax));
         return histogram;
     }
 
     DEFINE_STATIC_LOCAL(EnumerationHistogram, histogram, ("Media.Controls.Show.Audio", MediaControlsShowMax));
@@ skipping 110 matching lines @@
 
 IntRect HTMLMediaElement::AutoplayHelperClientImpl::absoluteBoundingBoxRect() const
 {
     IntRect result;
     if (LayoutObject* object = m_element->layoutObject())
         result = object->absoluteBoundingBoxRect();
     return result;
 }
 
 } // namespace blink