Clear m_readyStateMaximum when there is no source.

Clear m_readyStateMaximum when there is no source.

BUG=674716

[sandersd (OOO until July 31), 2017-01-05 01:26:06]

sandersd@chromium.org changed reviewers:
+ foolip@chromium.org

[sandersd (OOO until July 31), 2017-01-05 01:26:06]

Is there any other state we should be clearing as well? load() would otherwise
also:
  - emit 'emptied'
  - clear m_readyState
  - set m_paused
  - clear m_seeking
  - etc.

Alternatively, perhaps there is somewhere else that this clearing should go that
would be more robust.

[sandersd (OOO until July 31), 2017-01-05 18:56:07]

Oh, or we could set the network state to kNetworkNoSource. I don't know if that
matches the spec, but it does simplify reasoning about HTMLMediaElement.

[sandersd (OOO until July 31), 2017-01-23 23:58:37]

Description was changed from

==========
Clear m_readyStateMaximum when there is no source.

BUG=674716
==========

to

==========
Clear m_readyStateMaximum when there is no source.

BUG=674716
==========

[sandersd (OOO until July 31), 2017-01-23 23:58:37]

sandersd@chromium.org changed reviewers:
+ mlamouri@chromium.org
- foolip@chromium.org

[sandersd (OOO until July 31), 2017-01-23 23:59:02]

On 2017/01/05 18:56:07, sandersd wrote:
> Oh, or we could set the network state to kNetworkNoSource. I don't know if
that
> matches the spec, but it does simplify reasoning about HTMLMediaElement.

=> mlamouri@ to decide.

[foolip, 2017-01-26 05:00:50]

What's the status of this now?

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

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

https://codereview.chromium.org/2616803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:955:
m_readyStateMaximum = kHaveNothing;
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]

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

https://codereview.chromium.org/2616803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:955:
m_readyStateMaximum = kHaveNothing;
On 2017/01/26 05:06:56, foolip_UTC7 wrote:
> 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?

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]

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

https://codereview.chromium.org/2616803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:955:
m_readyStateMaximum = kHaveNothing;
On 2017/01/26 19:17:16, sandersd wrote:
> On 2017/01/26 05:06:56, foolip_UTC7 wrote:
> > 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?
> 
> 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.)

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]

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

https://codereview.chromium.org/2616803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:955:
m_readyStateMaximum = kHaveNothing;
> 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.

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?

[mlamouri (slow - plz ping), 2017-02-10 14:51:52]

That's not a trivial issue :)

It would be great sandersd@ if you could write a LayoutTest or a unit test for
this. It would be useful for the future in case of your fix doesn't quite work
and would help better grasp the issue.

Otherwise, wouldn't it make sense to only reset the m_readyStateMaximum to
kHaveNothing when the m_webMediaPlayer is reset?

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

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

https://codereview.chromium.org/2616803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:955:
m_readyStateMaximum = kHaveNothing;
On 2017/01/27 19:19:47, sandersd wrote:
> > 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.
> 
> 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?

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:29]

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

https://codereview.chromium.org/2616803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:955:
m_readyStateMaximum = kHaveNothing;
On 2017/02/21 06:59:14, foolip_UTC9 wrote:
> On 2017/01/27 19:19:47, sandersd wrote:
> > > 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.
> > 
> > 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?
> 
> 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?

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]

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

https://codereview.chromium.org/2616803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:955:
m_readyStateMaximum = kHaveNothing;
On 2017/04/01 00:17:28, sandersd wrote:
> On 2017/02/21 06:59:14, foolip_UTC9 wrote:
> > On 2017/01/27 19:19:47, sandersd wrote:
> > > > 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.
> > > 
> > > 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?
> > 
> > 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?
> 
> 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

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.

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

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.

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

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

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

https://codereview.chromium.org/2616803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:955:
m_readyStateMaximum = kHaveNothing;
> 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.

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.)

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

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:41]

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

https://codereview.chromium.org/2616803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:955:
m_readyStateMaximum = kHaveNothing;
On 2017/04/04 19:03:52, sandersd wrote:
> > 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.
> 
> 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.)

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.

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

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]

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

https://codereview.chromium.org/2616803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:955:
m_readyStateMaximum = kHaveNothing;
On 2017/04/05 at 04:46:40, foolip OOO on Wednesday wrote:
> On 2017/04/04 19:03:52, sandersd wrote:
> > > 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.
> > 
> > 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.)
> 
> 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.

I would be happy with this "not most robust fix" if there is a test case
covering it.

> > > 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.
> > 
> > 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.
> 
> 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)

If it should never happen, we should add a DCHECK.

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

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:49]

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

https://codereview.chromium.org/2616803002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:955:
m_readyStateMaximum = kHaveNothing;
On 2017/04/18 14:00:13, mlamouri wrote:
> On 2017/04/05 at 04:46:40, foolip OOO on Wednesday wrote:
> > On 2017/04/04 19:03:52, sandersd wrote:
> > > > 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.
> > > 
> > > 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.)
> > 
> > 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.
> 
> I would be happy with this "not most robust fix" if there is a test case
> covering it.
> 
> > > >
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.
> > > 
> > > 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.
> > 
> > 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)
> 
> If it should never happen, we should add a DCHECK.
> 
> >  - 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?
> 
> 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.

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.)

[sandersd (OOO until July 31), 2017-04-28 22:04:54]

> >  - setNetworkState(WebMediaPlayer::NetworkState) (seems like this
> > should never happen)
>
> If it should never happen, we should add a DCHECK.

Agreed, but in practice HTMLMediaElement uses the same setter for internal use
as well, so there is no obvious place for the DCHECK to go.

> sandersd@, do you want to try the fix outlined above

Each time I attempt this, I run into the problem of not being sure exactly what
state I should be resetting when.

Simply destroying the player does nothing; the only normal path that clears
|ready_state_maximum_| is InvokeLoadAlgorithm() with |network_state_ ==
kNetworkEmpty|.

There are several different methods for clearing state:
  - ClearMediaPlayer(): Doesn't affect ready state.
  - ClearMediaPlayerAndAudioSourceProviderClientWithoutLocking(): Doesn't affect
ready state.
  - ResetMediaPlayerAndMediaSource(): Doesn't affect ready state.
  - Dispose(): Doesn't affect ready state.
  - ContextDestroyed(): Probably actually does what we want, but clearly wasn't
meant for this.

If it's as simple as factoring ContextDestroyed() out so that I can call the
same implementation, that is easy, but I have little confidence in the
correctness of my attempts so far.

[foolip, 2017-05-03 15:13:58]

On 2017/04/28 22:04:54, sandersd wrote:
> > >  - setNetworkState(WebMediaPlayer::NetworkState) (seems like this
> > > should never happen)
> >
> > If it should never happen, we should add a DCHECK.
> 
> Agreed, but in practice HTMLMediaElement uses the same setter for internal use
> as well, so there is no obvious place for the DCHECK to go.
> 
> > sandersd@, do you want to try the fix outlined above
> 
> Each time I attempt this, I run into the problem of not being sure exactly
what
> state I should be resetting when.
> 
> Simply destroying the player does nothing; the only normal path that clears
> |ready_state_maximum_| is InvokeLoadAlgorithm() with |network_state_ ==
> kNetworkEmpty|.
> 
> There are several different methods for clearing state:
>   - ClearMediaPlayer(): Doesn't affect ready state.
>   - ClearMediaPlayerAndAudioSourceProviderClientWithoutLocking(): Doesn't
affect
> ready state.
>   - ResetMediaPlayerAndMediaSource(): Doesn't affect ready state.
>   - Dispose(): Doesn't affect ready state.
>   - ContextDestroyed(): Probably actually does what we want, but clearly
wasn't
> meant for this.
> 
> If it's as simple as factoring ContextDestroyed() out so that I can call the
> same implementation, that is easy, but I have little confidence in the
> correctness of my attempts so far.

There are indeed too many reset method. ResetMediaPlayerAndMediaSource() even
has a TODO to merge it into something that doesn't exist anymore. Refactoring
the three first methods in your list to make more sense at a glance would be
nice. I suspect that ClearMediaPlayer() and ResetMediaPlayerAndMediaSource()
could be merged, they already share some bits. Or at least all of the clearing
methods should form some kind of hierarchy where it's clear at which level to
call in which context.

Lacking such a cleanup, adding stuff to both ClearMediaPlayer() and
ResetMediaPlayerAndMediaSource() might just have the intended effect, but it's
hard to be confident without trying it.