Reset readystates when webmediaplayer is cleared

Reset readystates when webmediaplayer is cleared

WebMediaPlayer is going out of sync with m_readyState which is causing
crashes. Reset the ready states when WebMediaPlayer is cleared.

BUG=537013, 541520
Committed: https://crrev.com/25778e5a479379a937e9405081f696d1875b73f3
Cr-Commit-Position: refs/heads/master@{#355076}

[Srirama, 2015-10-20 10:37:36]

Patchset #1 (id:1) has been deleted

[Srirama, 2015-10-20 10:40:43]

Patchset #1 (id:20001) has been deleted

[Srirama, 2015-10-20 10:44:02]

Description was changed from

==========
fix

BUG=
==========

to

==========
Reset readystates when webmediaplayer is cleared

WebMediaPlayer is going out of sync with m_readyState which is causing 
crashes. Reset the ready states when WebMediaPlayer is cleared.

BUG=537013, 541520
==========

[Srirama, 2015-10-20 10:44:43]

srirama.m@samsung.com changed reviewers:
+ philipj@opera.com

[Srirama, 2015-10-20 10:44:43]

PTAL.

[philipj_slow, 2015-10-20 11:24:54]

https://codereview.chromium.org/1406183003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/video-move-to-new-document.html
(right):

https://codereview.chromium.org/1406183003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/video-move-to-new-document.html:2:
<title>move video element to new document</title>
Can you put -crash in the filename and perhaps a comment before t.done() to
explain what this is testing?

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

https://codereview.chromium.org/1406183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3048: m_readyState =
HAVE_NOTHING;
Shouldn't the readyState be reset unconditionally? I guess this happens here
merely so that it happens before the function returns, but moving up the "Reset
m_readyState since m_webMediaPlayer is gone" bit to right after the
clearMediaPlayer(-1) call would be more clear I think.

[Srirama, 2015-10-20 11:31:34]

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

https://codereview.chromium.org/1406183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3048: m_readyState =
HAVE_NOTHING;
On 2015/10/20 11:24:53, philipj wrote:
> Shouldn't the readyState be reset unconditionally? I guess this happens here
> merely so that it happens before the function returns, but moving up the
"Reset
> m_readyState since m_webMediaPlayer is gone" bit to right after the
> clearMediaPlayer(-1) call would be more clear I think.

I thought about that, but it will affect step4 in this function. We are
resetting networkstate based on readystate value. So is it fine to move it up?

[philipj_slow, 2015-10-20 11:52:01]

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

https://codereview.chromium.org/1406183003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3048: m_readyState =
HAVE_NOTHING;
On 2015/10/20 11:31:34, Srirama wrote:
> On 2015/10/20 11:24:53, philipj wrote:
> > Shouldn't the readyState be reset unconditionally? I guess this happens here
> > merely so that it happens before the function returns, but moving up the
> "Reset
> > m_readyState since m_webMediaPlayer is gone" bit to right after the
> > clearMediaPlayer(-1) call would be more clear I think.
> 
> I thought about that, but it will affect step4 in this function. We are
> resetting networkstate based on readystate value. So is it fine to move it up?

Oh, that's annoying. I think the problem here is that the spec text is written
in a way that makes sense if you merely stop the network activity but still
allow the data already fetched to be played, which implies keeping the player
alive. However, the first thing we do is to clear the player...

Have you dug into the purpose of this early return? Is it really a good idea to
skip over the `m_currentSourceNode = nullptr` step, for example?

[Srirama, 2015-10-20 13:07:14]

On 2015/10/20 11:52:01, philipj wrote:
>
https://codereview.chromium.org/1406183003/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
> 
>
https://codereview.chromium.org/1406183003/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3048: m_readyState =
> HAVE_NOTHING;
> On 2015/10/20 11:31:34, Srirama wrote:
> > On 2015/10/20 11:24:53, philipj wrote:
> > > Shouldn't the readyState be reset unconditionally? I guess this happens
here
> > > merely so that it happens before the function returns, but moving up the
> > "Reset
> > > m_readyState since m_webMediaPlayer is gone" bit to right after the
> > > clearMediaPlayer(-1) call would be more clear I think.
> > 
> > I thought about that, but it will affect step4 in this function. We are
> > resetting networkstate based on readystate value. So is it fine to move it
up?
> 
> Oh, that's annoying. I think the problem here is that the spec text is written
> in a way that makes sense if you merely stop the network activity but still
> allow the data already fetched to be played, which implies keeping the player
> alive. However, the first thing we do is to clear the player...
> 
> Have you dug into the purpose of this early return? Is it really a good idea
to
> skip over the `m_currentSourceNode = nullptr` step, for example?

Looks like this early return is there from webkit times and it is at the top
initially. Later on clearMediaPlayer was moved up to fix some crashes. Even i
have tried moving down the clearMediaPlayer call initally but there are some
crashes, so i changed the approach. Should i dig a bit more into the old webkit
code?

[philipj_slow, 2015-10-20 13:15:14]

On 2015/10/20 13:07:14, Srirama wrote:
> On 2015/10/20 11:52:01, philipj wrote:
> >
>
https://codereview.chromium.org/1406183003/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1406183003/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3048: m_readyState
=
> > HAVE_NOTHING;
> > On 2015/10/20 11:31:34, Srirama wrote:
> > > On 2015/10/20 11:24:53, philipj wrote:
> > > > Shouldn't the readyState be reset unconditionally? I guess this happens
> here
> > > > merely so that it happens before the function returns, but moving up the
> > > "Reset
> > > > m_readyState since m_webMediaPlayer is gone" bit to right after the
> > > > clearMediaPlayer(-1) call would be more clear I think.
> > > 
> > > I thought about that, but it will affect step4 in this function. We are
> > > resetting networkstate based on readystate value. So is it fine to move it
> up?
> > 
> > Oh, that's annoying. I think the problem here is that the spec text is
written
> > in a way that makes sense if you merely stop the network activity but still
> > allow the data already fetched to be played, which implies keeping the
player
> > alive. However, the first thing we do is to clear the player...
> > 
> > Have you dug into the purpose of this early return? Is it really a good idea
> to
> > skip over the `m_currentSourceNode = nullptr` step, for example?
> 
> Looks like this early return is there from webkit times and it is at the top
> initially. Later on clearMediaPlayer was moved up to fix some crashes. Even i
> have tried moving down the clearMediaPlayer call initally but there are some
> crashes, so i changed the approach. Should i dig a bit more into the old
webkit
> code?

I would be curious to know what bad things might happen if this early return is
removed entirely. The history of it in WebKit might help answer that question,
but just staring at the code and thinking about it could be faster :)

[Srirama, 2015-10-20 13:59:20]

On 2015/10/20 13:15:14, philipj wrote:
> On 2015/10/20 13:07:14, Srirama wrote:
> > On 2015/10/20 11:52:01, philipj wrote:
> > >
> >
>
https://codereview.chromium.org/1406183003/diff/40001/third_party/WebKit/Sour...
> > > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1406183003/diff/40001/third_party/WebKit/Sour...
> > > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3048:
m_readyState
> =
> > > HAVE_NOTHING;
> > > On 2015/10/20 11:31:34, Srirama wrote:
> > > > On 2015/10/20 11:24:53, philipj wrote:
> > > > > Shouldn't the readyState be reset unconditionally? I guess this
happens
> > here
> > > > > merely so that it happens before the function returns, but moving up
the
> > > > "Reset
> > > > > m_readyState since m_webMediaPlayer is gone" bit to right after the
> > > > > clearMediaPlayer(-1) call would be more clear I think.
> > > > 
> > > > I thought about that, but it will affect step4 in this function. We are
> > > > resetting networkstate based on readystate value. So is it fine to move
it
> > up?
> > > 
> > > Oh, that's annoying. I think the problem here is that the spec text is
> written
> > > in a way that makes sense if you merely stop the network activity but
still
> > > allow the data already fetched to be played, which implies keeping the
> player
> > > alive. However, the first thing we do is to clear the player...
> > > 
> > > Have you dug into the purpose of this early return? Is it really a good
idea
> > to
> > > skip over the `m_currentSourceNode = nullptr` step, for example?
> > 
> > Looks like this early return is there from webkit times and it is at the top
> > initially. Later on clearMediaPlayer was moved up to fix some crashes. Even
i
> > have tried moving down the clearMediaPlayer call initally but there are some
> > crashes, so i changed the approach. Should i dig a bit more into the old
> webkit
> > code?
> 
> I would be curious to know what bad things might happen if this early return
is
> removed entirely. The history of it in WebKit might help answer that question,
> but just staring at the code and thinking about it could be faster :)

By removing that entire condition, there are no test failures. From the webkit
history(avoid reload on page cache, page cache anyway is not there in chrome)
and code review, I think the first two conditions are just to avoid unnecessary
execution of the below code. Is it safe enough to drop m_isFinalizing as well,
though there are no test failures? Even that was added to solve some clusterfuzz
issue :)

[philipj_slow, 2015-10-20 14:16:19]

On 2015/10/20 13:59:20, Srirama wrote:
> On 2015/10/20 13:15:14, philipj wrote:
> > On 2015/10/20 13:07:14, Srirama wrote:
> > > On 2015/10/20 11:52:01, philipj wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1406183003/diff/40001/third_party/WebKit/Sour...
> > > > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1406183003/diff/40001/third_party/WebKit/Sour...
> > > > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:3048:
> m_readyState
> > =
> > > > HAVE_NOTHING;
> > > > On 2015/10/20 11:31:34, Srirama wrote:
> > > > > On 2015/10/20 11:24:53, philipj wrote:
> > > > > > Shouldn't the readyState be reset unconditionally? I guess this
> happens
> > > here
> > > > > > merely so that it happens before the function returns, but moving up
> the
> > > > > "Reset
> > > > > > m_readyState since m_webMediaPlayer is gone" bit to right after the
> > > > > > clearMediaPlayer(-1) call would be more clear I think.
> > > > > 
> > > > > I thought about that, but it will affect step4 in this function. We
are
> > > > > resetting networkstate based on readystate value. So is it fine to
move
> it
> > > up?
> > > > 
> > > > Oh, that's annoying. I think the problem here is that the spec text is
> > written
> > > > in a way that makes sense if you merely stop the network activity but
> still
> > > > allow the data already fetched to be played, which implies keeping the
> > player
> > > > alive. However, the first thing we do is to clear the player...
> > > > 
> > > > Have you dug into the purpose of this early return? Is it really a good
> idea
> > > to
> > > > skip over the `m_currentSourceNode = nullptr` step, for example?
> > > 
> > > Looks like this early return is there from webkit times and it is at the
top
> > > initially. Later on clearMediaPlayer was moved up to fix some crashes.
Even
> i
> > > have tried moving down the clearMediaPlayer call initally but there are
some
> > > crashes, so i changed the approach. Should i dig a bit more into the old
> > webkit
> > > code?
> > 
> > I would be curious to know what bad things might happen if this early return
> is
> > removed entirely. The history of it in WebKit might help answer that
question,
> > but just staring at the code and thinking about it could be faster :)
> 
> By removing that entire condition, there are no test failures. From the webkit
> history(avoid reload on page cache, page cache anyway is not there in chrome)
> and code review, I think the first two conditions are just to avoid
unnecessary
> execution of the below code. Is it safe enough to drop m_isFinalizing as well,
> though there are no test failures? Even that was added to solve some
clusterfuzz
> issue :)

Was no test case added together with the m_isFinalizing fix?

In order to not risk re-introducing a new crash, perhaps the very most
conservative thing would be to set a ReadyState readyState = m_readyState;
before resetting the readyState, so that there's just one place resetting, and
step 4 can use the old state.

A TODO to get rid of that early return would also be nice.

[Srirama, 2015-10-20 14:32:32]

Modified as suggested.
PTAL. Hope you are not having hard time reviewing my patches.

https://codereview.chromium.org/1406183003/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/media/video-move-to-new-document.html
(right):

https://codereview.chromium.org/1406183003/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/media/video-move-to-new-document.html:2:
<title>move video element to new document</title>
On 2015/10/20 11:24:53, philipj wrote:
> Can you put -crash in the filename and perhaps a comment before t.done() to
> explain what this is testing?

Done.

[philipj_slow, 2015-10-20 14:45:25]

LGTM, I've also verified that the test crashes for me locally without the fix.

No trouble reviewing at all, the hard part is just figuring out what to do with
this mess :)

[Srirama, 2015-10-20 16:23:46]

The CQ bit was checked by srirama.m@samsung.com

[commit-bot: I haz the power, 2015-10-20 16:24:44]

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

[commit-bot: I haz the power, 2015-10-20 16:35:05]

Committed patchset #2 (id:60001)

[commit-bot: I haz the power, 2015-10-20 16:36:26]

Patchset 2 (id:??) landed as
https://crrev.com/25778e5a479379a937e9405081f696d1875b73f3
Cr-Commit-Position: refs/heads/master@{#355076}