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

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
Committed: https://crrev.com/3a82d82079ca79cddd932eb77f924db5428885b9
Cr-Commit-Position: refs/heads/master@{#398370}

[mlamouri (slow - plz ping), 2016-04-06 13:52:44]

mlamouri@chromium.org changed reviewers:
+ philipj@opera.com

[mlamouri (slow - plz ping), 2016-04-06 13:52:44]

Philip, PTAL.

Two things to note:
- things are becoming a bit messy. It would be really nice if we could create N
cancelable tasks and be able to pass arguments to them.
- it seems that one test that was added added was broken. I will investigate
that in a different CL instead of trying to fix everything here.

[mlamouri (slow - plz ping), 2016-04-06 13:53:04]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-06 13:53:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1865933002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1865933002/1

[mlamouri (slow - plz ping), 2016-04-06 13:54:00]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-06 13:54:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1865933002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1865933002/20001

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

https://codereview.chromium.org/1865933002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1865933002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:795:
m_playPromiseRejectList.append(resolver);
m_playPromiseRejectList.append(m_playPromiseResolvers) instead of loop?

https://codereview.chromium.org/1865933002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2019: // is called,
|m_playPromiseResolvers| needs to be populated. What this code do
s/do then/does/

https://codereview.chromium.org/1865933002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2024:
m_playPromiseResolvers.append(resolver);
I don't follow why this change was needed. Does it have to do with the order
relative to something inside play()?

https://codereview.chromium.org/1865933002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3734: // appended
promise list or append the new promise to the current list. The
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?

[commit-bot: I haz the power, 2016-04-06 14:55:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-06 14:55:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

PTAL.

https://codereview.chromium.org/1865933002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1865933002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:795:
m_playPromiseRejectList.append(resolver);
On 2016/04/06 at 14:12:59, philipj wrote:
> m_playPromiseRejectList.append(m_playPromiseResolvers) instead of loop?

Done. Here and other places.

https://codereview.chromium.org/1865933002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2019: // is called,
|m_playPromiseResolvers| needs to be populated. What this code do
On 2016/04/06 at 14:12:59, philipj wrote:
> s/do then/does/

Done.

https://codereview.chromium.org/1865933002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2024:
m_playPromiseResolvers.append(resolver);
On 2016/04/06 at 14:12:59, philipj wrote:
> I don't follow why this change was needed. Does it have to do with the order
relative to something inside play()?

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.

https://codereview.chromium.org/1865933002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3734: // appended
promise list or append the new promise to the current list. The
On 2016/04/06 at 14:12:59, philipj wrote:
> 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?

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?

[mlamouri (slow - plz ping), 2016-04-12 14:40:16]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-04-12 14:40:40]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1865933002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1865933002/60001

[commit-bot: I haz the power, 2016-04-12 15:59:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-04-12 15:59:01]

Dry run: This issue passed the CQ dry run.

[mlamouri (slow - plz ping), 2016-04-12 17:00:22]

I've updated the CL to asynchronously reject play promises from the load
algorithm too. It matches the WIP spec change.

[philipj_slow, 2016-04-27 13:47:41]

I won't be able to see this through to the end, I suggest fs@opera.com as a
reviewer when the spec change is done. I will be able to give feedback on the
spec PR, however, on evenings and such.

[mlamouri (slow - plz ping), 2016-06-05 16:07:33]

mlamouri@chromium.org changed reviewers:
+ avayvod@chromium.org, fs@opera.com
- philipj@opera.com

[mlamouri (slow - plz ping), 2016-06-05 16:07:34]

avayvod@ and fs@, can you PTAL? It should now match the spec (which has been
agreed on).

[mlamouri (slow - plz ping), 2016-06-05 16:07:40]

The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-06-05 16:07:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1865933002/100001

[commit-bot: I haz the power, 2016-06-05 17:23:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-05 17:23:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

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

LGTM w/ nits

https://codereview.chromium.org/1865933002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/media/media-play-promise.html (right):

https://codereview.chromium.org/1865933002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/media-play-promise.html:109: // promise
that resolves successfuly.
Nit: successfully (also below)

https://codereview.chromium.org/1865933002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1865933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3716: // former
approach is prefered because it might be the less observable
Nit: preferred (also below)

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

lgtm with nits

https://codereview.chromium.org/1865933002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1865933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3737: // former
approach is prefered because it might be the less observable
nit: the code seems to be doing the latter, not former. same above

https://codereview.chromium.org/1865933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3751:
m_playPromiseErrorCode = code;
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?

https://codereview.chromium.org/1865933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3788: void
HTMLMediaElement::rejectPlayPromisesInternal(ExceptionCode code, const String&
message)
nit: seems like this method could be inlined

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

The CQ bit was checked by mlamouri@chromium.org

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

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, fs@opera.com
Link to the patchset: https://codereview.chromium.org/1865933002/#ps120001
(title: "review comments")

[commit-bot: I haz the power, 2016-06-06 16:00:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1865933002/120001

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

https://codereview.chromium.org/1865933002/diff/100001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/media/media-play-promise.html (right):

https://codereview.chromium.org/1865933002/diff/100001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/media/media-play-promise.html:109: // promise
that resolves successfuly.
On 2016/06/06 at 09:13:24, fs (OoO) wrote:
> Nit: successfully (also below)

Changed other places in the file.

https://codereview.chromium.org/1865933002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):

https://codereview.chromium.org/1865933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3716: // former
approach is prefered because it might be the less observable
On 2016/06/06 at 09:13:24, fs (OoO) wrote:
> Nit: preferred (also below)

Done.

https://codereview.chromium.org/1865933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3737: // former
approach is prefered because it might be the less observable
On 2016/06/06 at 13:08:57, whywhat wrote:
> nit: the code seems to be doing the latter, not former. same above

Done.

https://codereview.chromium.org/1865933002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3788: void
HTMLMediaElement::rejectPlayPromisesInternal(ExceptionCode code, const String&
message)
On 2016/06/06 at 13:08:57, whywhat wrote:
> nit: seems like this method could be inlined

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

[commit-bot: I haz the power, 2016-06-06 17:30:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-06-06 17:30:06]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mlamouri (slow - plz ping), 2016-06-07 19:18:57]

The CQ bit was checked by mlamouri@chromium.org

[mlamouri (slow - plz ping), 2016-06-07 19:18:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
avayvod@chromium.org, fs@opera.com
Link to the patchset: https://codereview.chromium.org/1865933002/#ps140001
(title: "fix")

[commit-bot: I haz the power, 2016-06-07 19:31:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1865933002/140001

[commit-bot: I haz the power, 2016-06-07 21:25:54]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-06-07 21:27:07]

Description was changed from

==========
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
==========

to

==========
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

Committed: https://crrev.com/3a82d82079ca79cddd932eb77f924db5428885b9
Cr-Commit-Position: refs/heads/master@{#398370}
==========

[commit-bot: I haz the power, 2016-06-07 21:27:08]

Patchset 8 (id:??) landed as
https://crrev.com/3a82d82079ca79cddd932eb77f924db5428885b9
Cr-Commit-Position: refs/heads/master@{#398370}