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 773 matching lines @@
     m_displayMode = Unknown;
 
     // 1 - Abort any already-running instance of the resource selection algorithm for this element.
     m_loadState = WaitingForSource;
     m_currentSourceNode = nullptr;
 
     // 2 - If there are any tasks from the media element's media element event task source in
     // one of the task queues, then remove those tasks.
     cancelPendingEventsAndCallbacks();
 
+    for (auto& resolver : m_playPromiseResolvers)
+        m_playPromiseRejectList.append(resolver);

[philipj_slow, 2016/04/06 14:12:59]

m_playPromiseRejectList.append(m_playPromiseResolvers) instead of loop?

[mlamouri (slow - plz ping), 2016/04/12 14:36:20]

Done. Here and other places.

+    m_playPromiseResolvers.clear();
     rejectPlayPromises(AbortError, "The play() request was interrupted by a new load request.");
 
     // 3 - If the media element's networkState is set to NETWORK_LOADING or NETWORK_IDLE, queue
     // a task to fire a simple event named abort at the media element.
     if (m_networkState == NETWORK_LOADING || m_networkState == NETWORK_IDLE)
         scheduleEvent(EventTypeNames::abort);
 
     resetMediaPlayerAndMediaSource();
 
     // 4 - If the media element's networkState is not set to NETWORK_EMPTY, then run these substeps
@@ skipping 1200 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 do

[philipj_slow, 2016/04/06 14:12:59]

s/do then/does/

[mlamouri (slow - plz ping), 2016/04/12 14:36:20]

Done.

+    // then 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);

[philipj_slow, 2016/04/06 14:12:59]

I don't follow why this change was needed. Does it have to do with the order
relative to something inside play()?

[mlamouri (slow - plz ping), 2016/04/12 14:36:20]

Assume the media is already playing. It will resolve all the promises in a task.
Obviously, that include the promise linked to the current play() request.
However, without this change, we add the promise *after* the task is created. It
was fine before because we would check the promise list when running the task.
Given that we keep it aside when creating the task now, we would populate an
empty list of promises and never resolve the promise.

+
     Nullable<ExceptionCode> code = play();
     if (!code.isNull()) {
+        ASSERT(!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:
             ASSERT_NOT_REACHED();
         }
         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()
 {
     WTF_LOG(Media, "HTMLMediaElement::play(%p)", this);
 
     m_autoplayHelper->playMethodCalled();
 
     if (!UserGestureIndicator::processingUserGesture()) {
@@ skipping 1582 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->template registerWeakMembers<HTMLMediaElement, &HTMLMediaElement::clearWeakMembers>(this);
     Supplementable<HTMLMediaElement>::trace(visitor);
     HTMLElement::trace(visitor);
     ActiveDOMObject::trace(visitor);
 }
 
 void HTMLMediaElement::createPlaceholderTracksIfNecessary()
@@ skipping 61 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

[philipj_slow, 2016/04/06 14:12:59]

Hmm, so these are two decidedly not great options. I guess the easiest way to
see the problem is with a play();pause();enqueueTask();play();pause() sequence,
where both promises will be rejected by either the first or the second task, but
there should be one before and one after the unrelated task, which could perhaps
be from setting volume.

Can you write a failing test for this, and any other scenarios I haven't thought
of here? Those tests might indicate which option is the safest. FWIW, the
behavior with events is to just bundle them all up with the first task. That's
per spec because a media element has its own task source, but it still makes me
slightly inclined to favor a similar behavior here, simply adding to the list of
promises without posting a new task. Definitely on the fence, what risks do you
see?

[mlamouri (slow - plz ping), 2016/04/12 14:36:20]

I believe I'm doing what you are suggesting here or I misunderstood your
comment: when play(); pause(); stuf(); play(); pause(); happen, if the second
pause() rejects before the first is run, both of them will be rejected when the
first one should have been rejected.

The reason why I went with this solution, as explained is because it is likely
less observable: the second task will be resolved after pause() is called, just
maybe quicker than expected. Creating a new task would delay the first event
which could have side effects.

WDYT?

+    // former approach is prefered because it might be the less observable
+    // change.
+    ASSERT(m_playPromiseResolveList.isEmpty() || m_playPromiseResolveTask->isPending());
+    if (m_playPromiseResolvers.isEmpty())
+        return;
+
+    for (auto& resolver : m_playPromiseResolvers)
+        m_playPromiseResolveList.append(resolver);
+    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
+    // change.
+    ASSERT(m_playPromiseRejectList.isEmpty() || m_playPromiseRejectTask->isPending());
+    if (m_playPromiseResolvers.isEmpty())
+        return;
+
+    for (auto& resolver : m_playPromiseResolvers)
+        m_playPromiseRejectList.append(resolver);
+    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;
     Platform::current()->currentThread()->getWebTaskRunner()->postTask(BLINK_FROM_HERE, m_playPromiseRejectTask->cancelAndCreate());
 }
 
 void HTMLMediaElement::scheduleNotifyPlaying()
 {
     scheduleEvent(EventTypeNames::playing);
     scheduleResolvePlayPromises();
 }
 
 void HTMLMediaElement::resolvePlayPromises()
 {
-    for (auto& resolver: m_playResolvers)
+    for (auto& resolver: m_playPromiseResolveList)
         resolver->resolve();
 
-    m_playResolvers.clear();
+    m_playPromiseResolveList.clear();
 }
 
 void HTMLMediaElement::rejectPlayPromises()
 {
     // 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);
     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)
 {
     ASSERT(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();
 }
 
 void HTMLMediaElement::clearWeakMembers(Visitor* visitor)
 {
     if (!Heap::isHeapObjectAlive(m_audioSourceNode))
         getAudioSourceProvider().setClient(nullptr);
 }
 
 void HTMLMediaElement::AudioSourceProviderImpl::wrap(WebAudioSourceProvider* provider)
 {
@@ skipping 95 matching lines @@
 }
 
 #if !ENABLE(OILPAN)
 WeakPtr<HTMLMediaElement> HTMLMediaElement::createWeakPtr()
 {
     return m_weakPtrFactory.createWeakPtr();
 }
 #endif
 
 } // namespace blink