Fix races between tasks resolving/rejecting play promises and them being added

[mounirlamouri]

For example, if pause() is called, it creates a task to reject all
promises even if there is none around. If play() is called just after,
this task would reject the newly created promise.

This is fixing the issue by creating tasks rejecting/resolving a given
list of promises, copied for the task.

Closes #996.

[mounirlamouri]

Creating special code before queueing makes the editorial work a bit tedious sometimes.

For resolve, it's fine apart that there are some places queuing for two things one after each other. I guess implementations could be smart about it if needed.

However, for reject, it's a bit different. First of all, the load algorithm reject is now asynchronous. It's not entirely obvious because I didn't change the prose but the algorithm is now async. Second of all, the source error algorithm is run asynchronously and it's a very long list of tasks with rejecting in the middle of it. At the moment, the spec says to asynchronously reject inside an asynchronous task. I'm not sure we want to do that. Though, I'm not sure what to do. Any editorial advice would be welcome :)

[mounirlamouri]

Replying to myselfy, Blink doesn't actually run the dedicated source failure steps asynchronously but run some parts of it asynchronously. Do we know if this part of the spec is web compatible? Would it be worth changing it?

@padenot do you know how Gecko implements this?

[padenot]

I think gecko does it synchronously. @cpearce, did I read the code right ?

[mounirlamouri]

Indeed, it seems that Gecko run the algorithm synchronously.
Also WebKit has a similar code than Blink here so they both run the algorithm synchronously.

@foolip, is there a reason I'm missing why this is run asynchronously per-spec?

[foolip]

@mounirlamouri, do you mean why the spec says "Queue a task to run the dedicated media source failure steps" instead of just running the steps synchronously at the call sites in the resource selection algorithm?

I'm pretty sure that the original reason was to ensure that events are delivered in the correct order. Since all events are fired inside tasks, this has to be as well or it might be fired before something that actually happened before it. And the reason that all of the steps are run in a task instead of just queueing the event is so that the error and networkState attributes cannot be observed to change before the event is fired. (I don't think this kind of guarantee is universally upheld, but think it ought to be.)

[foolip]

I will review the spec change itself tomorrow.

[foolip]

This change I think could become a problem, because it means that the promise will be resolved before the event is fired, and the order relative to other events could change. For the most part, the spec currently makes sure that there's a single task where observable state (error) is changed, the event is fired and then promises are rejected or resolved. At least I find it easier to reason about things when that is upheld.

Will comment separately on other options.

[foolip]

OK, so roughly speaking, I think things need to work like this:

• For all of the places that currently say "queue a task to notify about playing", they need to make a copy of the pending play events, empty the original list, and then queue a task to fire the playing event and reject the promises on that copied list.
• The one place that says "queue a task to resolve pending play promises" needs to do the same, except it shouldn't fire the playing event. Maybe a shared algorithm with a flag would make it nicer.
• The two places that say "reject pending play promises" inside a task (one to run the dedicated media source failure steps and one queued by the internal pause steps) need to be arranged such that the list of promises is copied and emptied before those tasks are queued.
• The "reject pending play promises" in the media element load algorithm should definitely empty the list of pending play promises synchronously. Whether they are rejected synchronously as well doesn't seem terribly important. If it is done in a tasks, it would be rather natural if it's done in the same task that fires the abort event, if any.

I realize that this is a bit convoluted and will not be entirely straightforward to implement until events are fired by normal tasks in Blink, but WDYT?

[mounirlamouri]

CC @jernoble

[mounirlamouri]

@foolip PTAL.

I've added a new algorithm to "move the list" which clear the list then returns it. I use this in order to get the list of promises before queue tasks. I didn't add a flag to "notify about playing" because the overhead isn't worth for the one caller that can simply call "resolve pending play promises".

The spec is a bit heavier but I believe the issue should be fixed without changing any other ordering behaviour.

[foolip]

There's an extra "the" here

[foolip]

In terms of behavior, this LGTM, precisely what I had in mind I think.

Editorial nit: The phrasing around move pending play promises is cumbersome in places, do you think that take pending play promises would read better? Example: "Take pending play promises, and let promises be the result." or "Take pending play promises, then queue a task to run the dedicated media source failure steps with the result."

It's nice when one can avoid wordy things like "Invoke the foo algorithm" if foo is a verb phrase already.

[mounirlamouri]

Comments applied. @foolip, can you merge?

[foolip]

On it.

[foolip]

Pushed as 18be384. I added a missing space and wordsmithed a little bit, editorial things only.