Clear m_readyStateMaximum when there is no source.

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

Index: third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
diff --git a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
index ececc0032f57cb4c49c5cadaef855936f65caa32..e7f93c269f6ad39df5ce7782aa3cd6227c93eb3f 100644
--- a/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
+++ b/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
@@ -950,6 +950,9 @@ void HTMLMediaElement::selectMediaResource() {
     m_loadState = WaitingForSource;
     setShouldDelayLoadEvent(false);
     setNetworkState(kNetworkEmpty);
+    // Also reset the maximum ready state, since load() will not do that when
+    // the network state is kNetworkEmpty.
+    m_readyStateMaximum = kHaveNothing;

[foolip, 2017/01/26 05:06:56]

At this point, why has m_readyState and m_readyStateMaximum progressed beyond
HAVE_NOTHING? At least in
https://html.spec.whatwg.org/multipage/embedded-content.html#concept-media-lo...,
this is where we end up if there is no src or source, and it should make sense
to return to precisely the state before invoking the resource selection
algorithm. Can you check what m_readyStateMaximum was in
HTMLMediaElement::invokeResourceSelectionAlgorithm()? If it wasn't HAVE_NOTHING,
then the problem needs to be addressed at an earlier point when the strange
state is entered. IIRC, that was when a previous load failed, but clue me in on
the details of that load again?

[sandersd (OOO until July 31), 2017/01/26 19:17:16]

What we're seeing here is basically a race condition. sourceWasAdded() triggers
the load behavior, but all the sources are removed by the time the timer fires.

This is exactly the branch that should be clearing the load state in that case,
as far as I can understand it. (There are some implications about what the state
must have been before this triggered, but it's load() that should do that
clearing. If we reset properly here, load() will do that.)

[foolip, 2017/01/27 06:23:07]

Right, adding and removing a source element is how you end up in this branch.
What I'm wondering is why m_readyStateMaximum wasn't already kHaveNothing at the
time that the source element was added. Something must have happened to the
media element before that to change m_readyStateMaximum from its initial state.

I guess we had a WebMediaPlayer that got slightly off the ground before the
error? Did we already discuss resetting m_readyState and m_readyStateMaximum
when the WebMediaPlayer is destroyed? The spec doesn't reset readyState on
errors, but I wonder if it's assuming that one can't have reached beyond
HAVE_NOTHING. This is interesting, if there is a way to fail fatally after going
beyond HAVE_NOTHING, then perhaps we need to change the spec as well.

[sandersd (OOO until July 31), 2017/01/27 19:19:47]

I don't think it makes sense to clear these values in an error state; it's
reasonable for applications to scrape and report them in that case.

The next load() will clear everything, *unless* m_networkState == kNetworkEmpty.
And that's only possible along this path; we set m_networkState to kNetworkEmpty
here without triggering the other cleanup.

(Technically a WebMediaPlayer can set the networkState to kNetworkEmpty, but I
checked and none of them ever do.)

If we just flagged that cleanup was still needed here (a special networkState? A
bool? Use kNoSource?), everything would be fine. Should we even be changing the
networkState on this path?

[foolip, 2017/02/21 06:59:14]

I'm still not clear on how exactly we end up in this state, and if not dealing
with it is a spec omission or not. Can you describe the sequence of events?

[sandersd (OOO until July 31), 2017/04/01 00:17:28]

Here is a complete reproduction sequence that I have tested against HEAD. There
may be other sequences.

  - Create a video element with <source src="corrupt.mp4">
  - WebMediaPlayerImpl gets as far as setReadyState(kHaveEnoughData) before
decoding fails.
  - mediaEngineError() executes. We now have:
    - m_loadState = WaitingForSource
    - m_networkState = kNetworkIdle
    - m_readyState = kHaveEnoughData
    - m_readyStateMaximum = kHaveEnoughData
  - Remove the source child.
  - Append and remove an empty <source> child.
  - sourceWasAdded() executes and schedules arms the loadTimer.
  - loadTimer fires, selectMediaResource() sets m_networkState = kNetworkEmpty.
  - Now load any media you want, because invokeLoadAlgorithm() won't clear
m_readyStateMaximum when m_networkState == kNetworkEmpty.

With this full sequence to consider, I think either of these solutions sound
reasonable:
  - networkState ends up being NoSource rather than Empty.
  - loadTimer effect is no-op in this case.

[foolip, 2017/04/04 04:09:50]

After this point, is it possible to play anything? If the error was really
fatal, it seems like we should destroy the WebMediaPlayer, set networkState to
kNetworkEmpty and readyState to kHaveNothing.

If this isn't the point where the old WebMediaPlayer is destroyed, when is it?
That seems like the point where we should reset at least readyState.
https://html.spec.whatwg.org/multipage/embedded-content.html#ready-states claims
that "Media elements whose networkState attribute are set to NETWORK_EMPTY are
always in the HAVE_NOTHING state." Clearly that is not true here. If this is the
point where the old WebMediaPlayer is destroyed, I feel like this is also the
point where we should reset readyState.

[sandersd (OOO until July 31), 2017/04/04 19:03:52]

No, playback is not possible at this point.

The current code does not destroy the player until the next load().

I'm not sure that it is correct to destroy it here; there are likely to be
getters that call through to the player. (eg. currentTime() or the soon-to-be
added accessor for detailed error information.)
Probably true, which this CL in its current state implements half of. I think
the real problem is that the selectMediaResource() implementation assumes that a
player does not exist, and I suspect that trying to patch it up incrementally is
a path to madness.

[foolip, 2017/04/05 04:46:40]

Good points. One could reset just the readyState at this point, which might be
more truthful, but perhaps this isn't the most robust fix.
Based on what the spec says about NETWORK_EMPTY, I looked for cases where set
networkState to kNetworkEmpty:
 - invokeLoadAlgorithm()
 - selectMediaResource()
 - setNetworkState(WebMediaPlayer::NetworkState) (seems like this should never
happen)
 - contextDestroyed()

So, I'm thinking that if we ensure that the WebMediaPlayer is destroyed in
invokeLoadAlgorithm() and selectMediaResource() as networkState is reset, that
should cover it?

[mlamouri (slow - plz ping), 2017/04/18 14:00:13]

I would be happy with this "not most robust fix" if there is a test case
covering it.
If it should never happen, we should add a DCHECK.
Sounds good to me too.

Can we pick one solution and land it this week? We just branched so it will
offer us a longer period to make sure the fix doesn't regress in Canary/Dev
builds.

[foolip, 2017/04/21 16:03:47]

sandersd@, do you want to try the fix outlined above, and also add the DCHECK if
setNetworkState(WebMediaPlayer::NetworkState) in fact never happens. (I said it
shouldn't because it would be quite strange, not because I looked at what
WebMediaPlayer implementations do.)

     updateDisplayState();
 
     BLINK_MEDIA_LOG << "selectMediaResource(" << (void*)this