Remove redundant webmediaplayer check

Remove redundant webmediaplayer check

Now that the issue 537013 is fixed, we can safely remove the
webmediaplayer check when the readystate is more than HAVE_NOTHING

[Srirama, 2015-10-21 18:23:37]

Description was changed from

==========
fix

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

to

==========
Remove redundant webmediaplayer check

Now that the issues 541520, 537013 are fixed, we can safely remove
the webmediaplayer check when the readystate is more than HAVE_NOTHING
==========

[Srirama, 2015-10-22 05:12:59]

Description was changed from

==========
Remove redundant webmediaplayer check

Now that the issues 541520, 537013 are fixed, we can safely remove
the webmediaplayer check when the readystate is more than HAVE_NOTHING
==========

to

==========
Remove redundant webmediaplayer check

Now that the issue 537013 is fixed, we can safely remove
the webmediaplayer check when the readystate is more than HAVE_NOTHING
==========

[Srirama, 2015-10-22 05:14:13]

Description was changed from

==========
Remove redundant webmediaplayer check

Now that the issue 537013 is fixed, we can safely remove
the webmediaplayer check when the readystate is more than HAVE_NOTHING
==========

to

==========
Remove redundant webmediaplayer check

Now that the issue 537013 is fixed, we can safely remove the
webmediaplayer check when the readystate is more than HAVE_NOTHING
==========

[Srirama, 2015-10-22 05:15:39]

PTAL.
I think it is fine now to remove these checks. Even if clusterfuzz reports some
issues, we will get a chance to analyze based on the test case instead of
guessing around with the code.

[Srirama, 2015-10-22 05:16:16]

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

[philipj_slow, 2015-10-22 07:00:53]

How certain are you that the there's really no way for m_webMediaPlayer.clear()
to happen without resetting readyState? In
HTMLMediaElement::loadNextSourceChild() it looks possible, I think. A list of
code paths that end up clearing m_webMediaPlayer and why they are also
guaranteed to reset readyState before returning to the event loop would be
great.

A good experiment to run, I think, would be to call
Microtask::enqueueMicrotask() immediately after the clear call, and to
RELEASE_ASSERT in the callback that if m_webMediaPlayer is still null, that both
m_readyState and m_readyStateMaximum have been reset. If that assert is hit, you
know there's something to fix. If it isn't hit, then we'll have a bit more
confidence that this is going to be safe.

[Srirama, 2015-10-23 05:17:42]

On 2015/10/22 07:00:53, philipj wrote:
> How certain are you that the there's really no way for
m_webMediaPlayer.clear()
> to happen without resetting readyState? In
> HTMLMediaElement::loadNextSourceChild() it looks possible, I think. A list of
> code paths that end up clearing m_webMediaPlayer and why they are also
> guaranteed to reset readyState before returning to the event loop would be
> great.
> 
> A good experiment to run, I think, would be to call
> Microtask::enqueueMicrotask() immediately after the clear call, and to
> RELEASE_ASSERT in the callback that if m_webMediaPlayer is still null, that
both
> m_readyState and m_readyStateMaximum have been reset. If that assert is hit,
you
> know there's something to fix. If it isn't hit, then we'll have a bit more
> confidence that this is going to be safe.

I have implemented WebTaskRunner in HTMLMediaElement similar to
ImageLoader::Task and created a microtask with a callback as you said (is there
any simpler way to directly create a micro task other than this?).
I ran all the media tests cases and looks like, one test case
audio-delete-while-slider-thumb-clicked.html is hitting the assert.
It crashes only in release mode and it doesn't crash if i directly open the test
in browser.
I will check it further. Please let me know if we can get logs while running
layout tests in release mode.

[philipj_slow, 2015-10-23 08:48:41]

On 2015/10/23 05:17:42, Srirama wrote:
> On 2015/10/22 07:00:53, philipj wrote:
> > How certain are you that the there's really no way for
> m_webMediaPlayer.clear()
> > to happen without resetting readyState? In
> > HTMLMediaElement::loadNextSourceChild() it looks possible, I think. A list
of
> > code paths that end up clearing m_webMediaPlayer and why they are also
> > guaranteed to reset readyState before returning to the event loop would be
> > great.
> > 
> > A good experiment to run, I think, would be to call
> > Microtask::enqueueMicrotask() immediately after the clear call, and to
> > RELEASE_ASSERT in the callback that if m_webMediaPlayer is still null, that
> both
> > m_readyState and m_readyStateMaximum have been reset. If that assert is hit,
> you
> > know there's something to fix. If it isn't hit, then we'll have a bit more
> > confidence that this is going to be safe.
> 
> I have implemented WebTaskRunner in HTMLMediaElement similar to
> ImageLoader::Task and created a microtask with a callback as you said (is
there
> any simpler way to directly create a micro task other than this?).
> I ran all the media tests cases and looks like, one test case
> audio-delete-while-slider-thumb-clicked.html is hitting the assert.
> It crashes only in release mode and it doesn't crash if i directly open the
test
> in browser.
> I will check it further. Please let me know if we can get logs while running
> layout tests in release mode.

I haven't actually tried it, but it's possible that a Closure involving a
WeakPtr<HTMLMediaElement> would be less code, but as long as you got something
working that seems fine.

Can you reproduce the crash if you run content_shell with --run-layout-test? If
not, that sounds strange, but if the only way you can get it to reproduce is
with the full layout test runner, you still ought to be able to at least printf
things, and probably get it in a debugger using --wrapper.

Have you verified that it fails due to the added assert, or could it be
something else entirely?

[Srirama, 2015-10-23 09:12:15]

On 2015/10/23 08:48:41, philipj wrote:
> On 2015/10/23 05:17:42, Srirama wrote:
> > On 2015/10/22 07:00:53, philipj wrote:
> > > How certain are you that the there's really no way for
> > m_webMediaPlayer.clear()
> > > to happen without resetting readyState? In
> > > HTMLMediaElement::loadNextSourceChild() it looks possible, I think. A list
> of
> > > code paths that end up clearing m_webMediaPlayer and why they are also
> > > guaranteed to reset readyState before returning to the event loop would be
> > > great.
> > > 
> > > A good experiment to run, I think, would be to call
> > > Microtask::enqueueMicrotask() immediately after the clear call, and to
> > > RELEASE_ASSERT in the callback that if m_webMediaPlayer is still null,
that
> > both
> > > m_readyState and m_readyStateMaximum have been reset. If that assert is
hit,
> > you
> > > know there's something to fix. If it isn't hit, then we'll have a bit more
> > > confidence that this is going to be safe.
> > 
> > I have implemented WebTaskRunner in HTMLMediaElement similar to
> > ImageLoader::Task and created a microtask with a callback as you said (is
> there
> > any simpler way to directly create a micro task other than this?).
> > I ran all the media tests cases and looks like, one test case
> > audio-delete-while-slider-thumb-clicked.html is hitting the assert.
> > It crashes only in release mode and it doesn't crash if i directly open the
> test
> > in browser.
> > I will check it further. Please let me know if we can get logs while running
> > layout tests in release mode.
> 
> I haven't actually tried it, but it's possible that a Closure involving a
> WeakPtr<HTMLMediaElement> would be less code, but as long as you got something
> working that seems fine.
> 
> Can you reproduce the crash if you run content_shell with --run-layout-test?
If
> not, that sounds strange, but if the only way you can get it to reproduce is
> with the full layout test runner, you still ought to be able to at least
printf
> things, and probably get it in a debugger using --wrapper.
> 
> Have you verified that it fails due to the added assert, or could it be
> something else entirely?

I am able to simulate using content_shell as a standalone test case.
Issue happens with ./out/Release/content_shell and doesn't happpen with
./out/Debug/content_shell.
And also if i directly execute the file in browser issue doesn't
happen(./out/Release/chrome -> load file).
Below is the crash log.
Received signal 4 ILL_ILLOPN 7f36473a77f1
#0 0x7f36502e4bcb base::debug::(anonymous namespace)::StackDumpSignalHandler()
#1 0x7f364ca5dcb0 <unknown>
#2 0x7f36473a77f1 blink::HTMLMediaElement::Task::run()
#3 0x7f364727af8a blink::microtaskFunctionCallback()
#4 0x7f3650e51b2b v8::internal::Isolate::RunMicrotasks()
#5 0x7f364727af19 blink::Microtask::performCheckpoint()

Below is the code in my microtask callback funtion:
WTF_LOG(Media, "HTMLMediaElement::onMicroTask(: %p) **************",
m_element.get());
WTF_LOG(Media, "HTMLMediaElement::onMicroTask(: player: %p, states: %d, %d",
m_element.get()->webMediaPlayer(),
    m_element.get()->m_readyState, m_element.get()->m_readyStateMaximum);
RELEASE_ASSERT(m_element.get()->webMediaPlayer() ||
(m_element.get()->m_readyState == 0 && m_element.get()->m_readyStateMaximum ==
0));

If i comment the RELEASE_ASSERT then there is no issue. So that is how i
confirmed that there is an issue with readystates.

So you mean i can use --wrapper option for release/content_shell to get WTF_LOG
messages?

[philipj_slow, 2015-10-23 09:39:04]

No, the --wrapper argument I think you could use to get the process into a
debugger, but I now realize that's pointless since it's a release build.

I'm not actually sure how WTF_LOG is supposed to work, whenever I need it I just
edit the WTFLogChannel LogMedia = { WTFLogChannelOff } line in Logging.cpp, but
I've always suspected I'm doing it wrong :)

[philipj_slow, 2015-10-23 09:42:34]

On 2015/10/23 09:39:04, philipj wrote:
> No, the --wrapper argument I think you could use to get the process into a
> debugger, but I now realize that's pointless since it's a release build.
> 
> I'm not actually sure how WTF_LOG is supposed to work, whenever I need it I
just
> edit the WTFLogChannel LogMedia = { WTFLogChannelOff } line in Logging.cpp,
but
> I've always suspected I'm doing it wrong :)

You also have to actually enable logging, blink_logging_always_on might be the
way.

[Srirama, 2015-10-23 09:49:34]

On 2015/10/23 09:42:34, philipj wrote:
> On 2015/10/23 09:39:04, philipj wrote:
> > No, the --wrapper argument I think you could use to get the process into a
> > debugger, but I now realize that's pointless since it's a release build.
> > 
> > I'm not actually sure how WTF_LOG is supposed to work, whenever I need it I
> just
> > edit the WTFLogChannel LogMedia = { WTFLogChannelOff } line in Logging.cpp,
> but
> > I've always suspected I'm doing it wrong :)
> 
> You also have to actually enable logging, blink_logging_always_on might be the
> way.

With debug build, i just run chrome with --blink-platform-log-channels=Media and
it works.
But this never used to work with release build, may be the logging needs to be
enabled.
I will try with your options, hopefully will get the logs.

[Srirama, 2015-10-26 10:56:59]

On 2015/10/23 09:49:34, Srirama wrote:
> On 2015/10/23 09:42:34, philipj wrote:
> > On 2015/10/23 09:39:04, philipj wrote:
> > > No, the --wrapper argument I think you could use to get the process into a
> > > debugger, but I now realize that's pointless since it's a release build.
> > > 
> > > I'm not actually sure how WTF_LOG is supposed to work, whenever I need it
I
> > just
> > > edit the WTFLogChannel LogMedia = { WTFLogChannelOff } line in
Logging.cpp,
> > but
> > > I've always suspected I'm doing it wrong :)
> > 
> > You also have to actually enable logging, blink_logging_always_on might be
the
> > way.
> 
> With debug build, i just run chrome with --blink-platform-log-channels=Media
and
> it works.
> But this never used to work with release build, may be the logging needs to be
> enabled.
> I will try with your options, hopefully will get the logs.

I am able to get the logs now and the issue is that, in the test case the audio
element is removed, so the audio element is destroyed which clears the player.
In my test code i am passing the element to micro task and the memory is still
valid when executing the task, so we get those values. Incase of debug build,
the memory is overwritten and it gives garbage values, so it doesn't assert.
So as the element is deleted, we don't need to reset the states.
So the existing layout tests don't hit the assert. Is it good enough to proceed
or should we analyze all code paths?

[philipj_slow, 2015-10-26 11:09:03]

On 2015/10/26 10:56:59, Srirama wrote:
> On 2015/10/23 09:49:34, Srirama wrote:
> > On 2015/10/23 09:42:34, philipj wrote:
> > > On 2015/10/23 09:39:04, philipj wrote:
> > > > No, the --wrapper argument I think you could use to get the process into
a
> > > > debugger, but I now realize that's pointless since it's a release build.
> > > > 
> > > > I'm not actually sure how WTF_LOG is supposed to work, whenever I need
it
> I
> > > just
> > > > edit the WTFLogChannel LogMedia = { WTFLogChannelOff } line in
> Logging.cpp,
> > > but
> > > > I've always suspected I'm doing it wrong :)
> > > 
> > > You also have to actually enable logging, blink_logging_always_on might be
> the
> > > way.
> > 
> > With debug build, i just run chrome with --blink-platform-log-channels=Media
> and
> > it works.
> > But this never used to work with release build, may be the logging needs to
be
> > enabled.
> > I will try with your options, hopefully will get the logs.
> 
> I am able to get the logs now and the issue is that, in the test case the
audio
> element is removed, so the audio element is destroyed which clears the player.
> In my test code i am passing the element to micro task and the memory is still
> valid when executing the task, so we get those values. Incase of debug build,
> the memory is overwritten and it gives garbage values, so it doesn't assert.
> So as the element is deleted, we don't need to reset the states.
> So the existing layout tests don't hit the assert. Is it good enough to
proceed
> or should we analyze all code paths?

Unless it turns out to be crazy hard, I still think that "A list of code paths
that end up clearing m_webMediaPlayer and why they are also guaranteed to reset
readyState before returning to the event loop would be great."

The experiment you have run gives us confidence that everything is as it seems,
but actually looking at the code and seeing why would be nice.

[Srirama, 2015-10-26 11:13:09]

On 2015/10/26 11:09:03, philipj wrote:
> On 2015/10/26 10:56:59, Srirama wrote:
> > On 2015/10/23 09:49:34, Srirama wrote:
> > > On 2015/10/23 09:42:34, philipj wrote:
> > > > On 2015/10/23 09:39:04, philipj wrote:
> > > > > No, the --wrapper argument I think you could use to get the process
into
> a
> > > > > debugger, but I now realize that's pointless since it's a release
build.
> > > > > 
> > > > > I'm not actually sure how WTF_LOG is supposed to work, whenever I need
> it
> > I
> > > > just
> > > > > edit the WTFLogChannel LogMedia = { WTFLogChannelOff } line in
> > Logging.cpp,
> > > > but
> > > > > I've always suspected I'm doing it wrong :)
> > > > 
> > > > You also have to actually enable logging, blink_logging_always_on might
be
> > the
> > > > way.
> > > 
> > > With debug build, i just run chrome with
--blink-platform-log-channels=Media
> > and
> > > it works.
> > > But this never used to work with release build, may be the logging needs
to
> be
> > > enabled.
> > > I will try with your options, hopefully will get the logs.
> > 
> > I am able to get the logs now and the issue is that, in the test case the
> audio
> > element is removed, so the audio element is destroyed which clears the
player.
> > In my test code i am passing the element to micro task and the memory is
still
> > valid when executing the task, so we get those values. Incase of debug
build,
> > the memory is overwritten and it gives garbage values, so it doesn't assert.
> > So as the element is deleted, we don't need to reset the states.
> > So the existing layout tests don't hit the assert. Is it good enough to
> proceed
> > or should we analyze all code paths?
> 
> Unless it turns out to be crazy hard, I still think that "A list of code paths
> that end up clearing m_webMediaPlayer and why they are also guaranteed to
reset
> readyState before returning to the event loop would be great."
> 
> The experiment you have run gives us confidence that everything is as it
seems,
> but actually looking at the code and seeing why would be nice.

ok, i will check that.

[Srirama, 2015-11-01 17:51:30]

On 2015/10/26 11:13:09, Srirama wrote:
> On 2015/10/26 11:09:03, philipj wrote:
> > On 2015/10/26 10:56:59, Srirama wrote:
> > > On 2015/10/23 09:49:34, Srirama wrote:
> > > > On 2015/10/23 09:42:34, philipj wrote:
> > > > > On 2015/10/23 09:39:04, philipj wrote:
> > > > > > No, the --wrapper argument I think you could use to get the process
> into
> > a
> > > > > > debugger, but I now realize that's pointless since it's a release
> build.
> > > > > > 
> > > > > > I'm not actually sure how WTF_LOG is supposed to work, whenever I
need
> > it
> > > I
> > > > > just
> > > > > > edit the WTFLogChannel LogMedia = { WTFLogChannelOff } line in
> > > Logging.cpp,
> > > > > but
> > > > > > I've always suspected I'm doing it wrong :)
> > > > > 
> > > > > You also have to actually enable logging, blink_logging_always_on
might
> be
> > > the
> > > > > way.
> > > > 
> > > > With debug build, i just run chrome with
> --blink-platform-log-channels=Media
> > > and
> > > > it works.
> > > > But this never used to work with release build, may be the logging needs
> to
> > be
> > > > enabled.
> > > > I will try with your options, hopefully will get the logs.
> > > 
> > > I am able to get the logs now and the issue is that, in the test case the
> > audio
> > > element is removed, so the audio element is destroyed which clears the
> player.
> > > In my test code i am passing the element to micro task and the memory is
> still
> > > valid when executing the task, so we get those values. Incase of debug
> build,
> > > the memory is overwritten and it gives garbage values, so it doesn't
assert.
> > > So as the element is deleted, we don't need to reset the states.
> > > So the existing layout tests don't hit the assert. Is it good enough to
> > proceed
> > > or should we analyze all code paths?
> > 
> > Unless it turns out to be crazy hard, I still think that "A list of code
paths
> > that end up clearing m_webMediaPlayer and why they are also guaranteed to
> reset
> > readyState before returning to the event loop would be great."
> > 
> > The experiment you have run gives us confidence that everything is as it
> seems,
> > but actually looking at the code and seeing why would be nice.
> 
> ok, i will check that.

Hi Philip, Please find the detailed code analysis for clearing media player.
ClearMediaPlayer() scenarios:
1) parseAttribute -> calls clearMediaPlayer with LoadMediaResource flag which
resets LoadMediaResource bit in m_pendingActionFlags. Then it calls
scheduleDelayedAction with LoadMediaResource, so the above two actions make sure
that it calls prepareForLoad which inturn resets readystates (here either
webmediaplayer will be NULL or network state will not be empty which makes sure
that readystates will be reset)
2) userCancelledLoad -> clears mediaplayer and reset readystates as well

clearMediaPlayerAndAudioSourceProviderClientWithoutLocking() scenarios:
1) Invoked in clearMediaPlayer() -> covered above
2) HTMLMediaElement::~HTMLMediaElement() -> no need to worry about states
3) void HTMLMediaElement::dispose() -> no need to worry about states
4) HTMLMediaElement::resetMediaPlayerAndMediaSource -> covered below

resetMediaPlayerAndMediaSource() scenarios:
1) HTMLMediaElement::prepareForLoad() -> Either mediaplayer will be null or
network state will not be empty which makes sure that readystates are reset
2) HTMLMediaElement::loadNextSourceChild() -> this is the only case which can
cause trouble. So i did various experiments based on the code which can lead to
a situation where player is cleared but states are not reset. Finally made a
test case which can crash though with a bit of hack (forcefully called
mediaEngineError(MediaError::create(MediaError::MEDIA_ERR_NETWORK)); at the end
of setReadyState on HAVE_METADATA state). Please find below the test case for
your reference.

<script src=video-test.js></script>
<script src=media-file.js></script>
<script>
    function load()
    {
        findMediaElement();
        video.removeChild(video.firstChild);
        var source = document.createElement("source");
        source.src = findMediaFile("video", "content/test");
	video.preload = "none";
        video.appendChild(source);
	
	setTimeout(function() {video.currentTime = 1.0}, 1000);
    }
    
    waitForEventOnce("loadedmetadata", load);
</script>
<video controls><source src="content/counting.ogv"></video>

So probably reset states in resetMediaPlayerAndMediaSource to fix the issue?

[Srirama, 2015-11-16 14:28:13]

On 2015/11/01 17:51:30, Srirama wrote:
> On 2015/10/26 11:13:09, Srirama wrote:
> > On 2015/10/26 11:09:03, philipj wrote:
> > > On 2015/10/26 10:56:59, Srirama wrote:
> > > > On 2015/10/23 09:49:34, Srirama wrote:
> > > > > On 2015/10/23 09:42:34, philipj wrote:
> > > > > > On 2015/10/23 09:39:04, philipj wrote:
> > > > > > > No, the --wrapper argument I think you could use to get the
process
> > into
> > > a
> > > > > > > debugger, but I now realize that's pointless since it's a release
> > build.
> > > > > > > 
> > > > > > > I'm not actually sure how WTF_LOG is supposed to work, whenever I
> need
> > > it
> > > > I
> > > > > > just
> > > > > > > edit the WTFLogChannel LogMedia = { WTFLogChannelOff } line in
> > > > Logging.cpp,
> > > > > > but
> > > > > > > I've always suspected I'm doing it wrong :)
> > > > > > 
> > > > > > You also have to actually enable logging, blink_logging_always_on
> might
> > be
> > > > the
> > > > > > way.
> > > > > 
> > > > > With debug build, i just run chrome with
> > --blink-platform-log-channels=Media
> > > > and
> > > > > it works.
> > > > > But this never used to work with release build, may be the logging
needs
> > to
> > > be
> > > > > enabled.
> > > > > I will try with your options, hopefully will get the logs.
> > > > 
> > > > I am able to get the logs now and the issue is that, in the test case
the
> > > audio
> > > > element is removed, so the audio element is destroyed which clears the
> > player.
> > > > In my test code i am passing the element to micro task and the memory is
> > still
> > > > valid when executing the task, so we get those values. Incase of debug
> > build,
> > > > the memory is overwritten and it gives garbage values, so it doesn't
> assert.
> > > > So as the element is deleted, we don't need to reset the states.
> > > > So the existing layout tests don't hit the assert. Is it good enough to
> > > proceed
> > > > or should we analyze all code paths?
> > > 
> > > Unless it turns out to be crazy hard, I still think that "A list of code
> paths
> > > that end up clearing m_webMediaPlayer and why they are also guaranteed to
> > reset
> > > readyState before returning to the event loop would be great."
> > > 
> > > The experiment you have run gives us confidence that everything is as it
> > seems,
> > > but actually looking at the code and seeing why would be nice.
> > 
> > ok, i will check that.
> 
> Hi Philip, Please find the detailed code analysis for clearing media player.
> ClearMediaPlayer() scenarios:
> 1) parseAttribute -> calls clearMediaPlayer with LoadMediaResource flag which
> resets LoadMediaResource bit in m_pendingActionFlags. Then it calls
> scheduleDelayedAction with LoadMediaResource, so the above two actions make
sure
> that it calls prepareForLoad which inturn resets readystates (here either
> webmediaplayer will be NULL or network state will not be empty which makes
sure
> that readystates will be reset)
> 2) userCancelledLoad -> clears mediaplayer and reset readystates as well
> 
> clearMediaPlayerAndAudioSourceProviderClientWithoutLocking() scenarios:
> 1) Invoked in clearMediaPlayer() -> covered above
> 2) HTMLMediaElement::~HTMLMediaElement() -> no need to worry about states
> 3) void HTMLMediaElement::dispose() -> no need to worry about states
> 4) HTMLMediaElement::resetMediaPlayerAndMediaSource -> covered below
> 
> resetMediaPlayerAndMediaSource() scenarios:
> 1) HTMLMediaElement::prepareForLoad() -> Either mediaplayer will be null or
> network state will not be empty which makes sure that readystates are reset
> 2) HTMLMediaElement::loadNextSourceChild() -> this is the only case which can
> cause trouble. So i did various experiments based on the code which can lead
to
> a situation where player is cleared but states are not reset. Finally made a
> test case which can crash though with a bit of hack (forcefully called
> mediaEngineError(MediaError::create(MediaError::MEDIA_ERR_NETWORK)); at the
end
> of setReadyState on HAVE_METADATA state). Please find below the test case for
> your reference.
> 
> <script src=video-test.js></script>
> <script src=media-file.js></script>
> <script>
>     function load()
>     {
>         findMediaElement();
>         video.removeChild(video.firstChild);
>         var source = document.createElement("source");
>         source.src = findMediaFile("video", "content/test");
> 	video.preload = "none";
>         video.appendChild(source);
> 	
> 	setTimeout(function() {video.currentTime = 1.0}, 1000);
>     }
>     
>     waitForEventOnce("loadedmetadata", load);
> </script>
> <video controls><source src="content/counting.ogv"></video>
> 
> So probably reset states in resetMediaPlayerAndMediaSource to fix the issue?

Can you have a look at this Philip?
I am trying to write a http test for this, but looks like the above scenario may
not happen. I will investigate more and confirm. But please have a look at the
above clearmediaplayer scenarios and lets me know if i missed something.

[philipj_slow, 2015-11-16 14:58:44]

On 2015/11/01 17:51:30, Srirama wrote:

> Hi Philip, Please find the detailed code analysis for clearing media player.
> ClearMediaPlayer() scenarios:
> 1) parseAttribute -> calls clearMediaPlayer with LoadMediaResource flag which
> resets LoadMediaResource bit in m_pendingActionFlags. Then it calls
> scheduleDelayedAction with LoadMediaResource, so the above two actions make
sure
> that it calls prepareForLoad which inturn resets readystates (here either
> webmediaplayer will be NULL or network state will not be empty which makes
sure
> that readystates will be reset)

How strong is the guarantee that networkState==NETWORK_EMPTY implies
readyState==HAVE_NOTHING?

> 2) userCancelledLoad -> clears mediaplayer and reset readystates as well

Acknowledged.

> clearMediaPlayerAndAudioSourceProviderClientWithoutLocking() scenarios:
> 1) Invoked in clearMediaPlayer() -> covered above
> 2) HTMLMediaElement::~HTMLMediaElement() -> no need to worry about states
> 3) void HTMLMediaElement::dispose() -> no need to worry about states
> 4) HTMLMediaElement::resetMediaPlayerAndMediaSource -> covered below

Acknowledged.

> resetMediaPlayerAndMediaSource() scenarios:
> 1) HTMLMediaElement::prepareForLoad() -> Either mediaplayer will be null or
> network state will not be empty which makes sure that readystates are reset

This is again assuming that m_networkState is in perfect sync with
m_webMediaPlayer, so same question.

> 2) HTMLMediaElement::loadNextSourceChild() -> this is the only case which can
> cause trouble. So i did various experiments based on the code which can lead
to
> a situation where player is cleared but states are not reset. Finally made a
> test case which can crash though with a bit of hack (forcefully called
> mediaEngineError(MediaError::create(MediaError::MEDIA_ERR_NETWORK)); at the
end
> of setReadyState on HAVE_METADATA state). Please find below the test case for
> your reference.
> 
> <script src=video-test.js></script>
> <script src=media-file.js></script>
> <script>
>     function load()
>     {
>         findMediaElement();
>         video.removeChild(video.firstChild);
>         var source = document.createElement("source");
>         source.src = findMediaFile("video", "content/test");
> 	video.preload = "none";
>         video.appendChild(source);
> 	
> 	setTimeout(function() {video.currentTime = 1.0}, 1000);
>     }
>     
>     waitForEventOnce("loadedmetadata", load);
> </script>
> <video controls><source src="content/counting.ogv"></video>
> 
> So probably reset states in resetMediaPlayerAndMediaSource to fix the issue?

OK, interesting. It might be worth checking if it's possible to reset both
readyState and networkState in
HTMLMediaElement::clearMediaPlayerAndAudioSourceProviderClientWithoutLocking(),
and in the cases where later steps depend on one of them, extract that before
resetting. Then it just cannot be out of sync after being cleared.

[Srirama, 2015-11-17 14:10:06]

On 2015/11/16 14:58:44, philipj wrote:
> On 2015/11/01 17:51:30, Srirama wrote:
> 
> > Hi Philip, Please find the detailed code analysis for clearing media player.
> > ClearMediaPlayer() scenarios:
> > 1) parseAttribute -> calls clearMediaPlayer with LoadMediaResource flag
which
> > resets LoadMediaResource bit in m_pendingActionFlags. Then it calls
> > scheduleDelayedAction with LoadMediaResource, so the above two actions make
> sure
> > that it calls prepareForLoad which inturn resets readystates (here either
> > webmediaplayer will be NULL or network state will not be empty which makes
> sure
> > that readystates will be reset)
> 
> How strong is the guarantee that networkState==NETWORK_EMPTY implies
> readyState==HAVE_NOTHING?

When networkState is NETWORK_EMPTY doesn't it mean that it just started a load
and there is no player created, or if player is created networkState will be set
from chromium side to some other state in fail or success case. Only problem is
if we get NETWORK_EMPTY from chromium side after creating player, which i
thought is unlikely.
> > 2) userCancelledLoad -> clears mediaplayer and reset readystates as well
> 
> Acknowledged.
> 
> > clearMediaPlayerAndAudioSourceProviderClientWithoutLocking() scenarios:
> > 1) Invoked in clearMediaPlayer() -> covered above
> > 2) HTMLMediaElement::~HTMLMediaElement() -> no need to worry about states
> > 3) void HTMLMediaElement::dispose() -> no need to worry about states
> > 4) HTMLMediaElement::resetMediaPlayerAndMediaSource -> covered below
> 
> Acknowledged.
> 
> > resetMediaPlayerAndMediaSource() scenarios:
> > 1) HTMLMediaElement::prepareForLoad() -> Either mediaplayer will be null or
> > network state will not be empty which makes sure that readystates are reset
> 
> This is again assuming that m_networkState is in perfect sync with
> m_webMediaPlayer, so same question.

ditto.

> > 2) HTMLMediaElement::loadNextSourceChild() -> this is the only case which
can
> > cause trouble. So i did various experiments based on the code which can lead
> to
> > a situation where player is cleared but states are not reset. Finally made a
> > test case which can crash though with a bit of hack (forcefully called
> > mediaEngineError(MediaError::create(MediaError::MEDIA_ERR_NETWORK)); at the
> end
> > of setReadyState on HAVE_METADATA state). Please find below the test case
for
> > your reference.
> > 
> > <script src=video-test.js></script>
> > <script src=media-file.js></script>
> > <script>
> >     function load()
> >     {
> >         findMediaElement();
> >         video.removeChild(video.firstChild);
> >         var source = document.createElement("source");
> >         source.src = findMediaFile("video", "content/test");
> > 	video.preload = "none";
> >         video.appendChild(source);
> > 	
> > 	setTimeout(function() {video.currentTime = 1.0}, 1000);
> >     }
> >     
> >     waitForEventOnce("loadedmetadata", load);
> > </script>
> > <video controls><source src="content/counting.ogv"></video>
> > 
> > So probably reset states in resetMediaPlayerAndMediaSource to fix the issue?
> 
> OK, interesting. It might be worth checking if it's possible to reset both
> readyState and networkState in
>
HTMLMediaElement::clearMediaPlayerAndAudioSourceProviderClientWithoutLocking(),
> and in the cases where later steps depend on one of them, extract that before
> resetting. Then it just cannot be out of sync after being cleared.

Ok, i will try this.

[philipj_slow, 2015-11-17 14:49:04]

On 2015/11/17 14:10:06, Srirama wrote:
> On 2015/11/16 14:58:44, philipj wrote:
> > On 2015/11/01 17:51:30, Srirama wrote:
> > 
> > > Hi Philip, Please find the detailed code analysis for clearing media
player.
> > > ClearMediaPlayer() scenarios:
> > > 1) parseAttribute -> calls clearMediaPlayer with LoadMediaResource flag
> which
> > > resets LoadMediaResource bit in m_pendingActionFlags. Then it calls
> > > scheduleDelayedAction with LoadMediaResource, so the above two actions
make
> > sure
> > > that it calls prepareForLoad which inturn resets readystates (here either
> > > webmediaplayer will be NULL or network state will not be empty which makes
> > sure
> > > that readystates will be reset)
> > 
> > How strong is the guarantee that networkState==NETWORK_EMPTY implies
> > readyState==HAVE_NOTHING?
> 
> When networkState is NETWORK_EMPTY doesn't it mean that it just started a load
> and there is no player created, or if player is created networkState will be
set
> from chromium side to some other state in fail or success case. Only problem
is
> if we get NETWORK_EMPTY from chromium side after creating player, which i
> thought is unlikely.

It looks like the WebMediaPlayer implementation starts out with
WebMediaPlayer::NetworkStateEmpty, but it looks like the only place that
WebMediaPlayer::networkState() is called is inside the networkStateChanged()
callback, and I can't find a way for that callback to be invoked while
networkState is EMPTY. Still, it would be nice to have some invariants,
something like assertShadowRootChildren(), that is called any time that
m_readyState, m_networkState or m_webMediaPlayer changes, to ensure that things
are precisely as we hope that they are.

[Srirama, 2015-11-18 08:35:13]

On 2015/11/17 14:49:04, philipj wrote:
> On 2015/11/17 14:10:06, Srirama wrote:
> > On 2015/11/16 14:58:44, philipj wrote:
> > > On 2015/11/01 17:51:30, Srirama wrote:
> > > 
> > > > Hi Philip, Please find the detailed code analysis for clearing media
> player.
> > > > ClearMediaPlayer() scenarios:
> > > > 1) parseAttribute -> calls clearMediaPlayer with LoadMediaResource flag
> > which
> > > > resets LoadMediaResource bit in m_pendingActionFlags. Then it calls
> > > > scheduleDelayedAction with LoadMediaResource, so the above two actions
> make
> > > sure
> > > > that it calls prepareForLoad which inturn resets readystates (here
either
> > > > webmediaplayer will be NULL or network state will not be empty which
makes
> > > sure
> > > > that readystates will be reset)
> > > 
> > > How strong is the guarantee that networkState==NETWORK_EMPTY implies
> > > readyState==HAVE_NOTHING?
> > 
> > When networkState is NETWORK_EMPTY doesn't it mean that it just started a
load
> > and there is no player created, or if player is created networkState will be
> set
> > from chromium side to some other state in fail or success case. Only problem
> is
> > if we get NETWORK_EMPTY from chromium side after creating player, which i
> > thought is unlikely.
> 
> It looks like the WebMediaPlayer implementation starts out with
> WebMediaPlayer::NetworkStateEmpty, but it looks like the only place that
> WebMediaPlayer::networkState() is called is inside the networkStateChanged()
> callback, and I can't find a way for that callback to be invoked while
> networkState is EMPTY. Still, it would be nice to have some invariants,
> something like assertShadowRootChildren(), that is called any time that
> m_readyState, m_networkState or m_webMediaPlayer changes, to ensure that
things
> are precisely as we hope that they are.

"Still, it would be nice to have some invariants, something like
assertShadowRootChildren()" sounds good. I will add it.
And what about this, should i check and do this as well?
"Reset both readyState and networkState in 
HTMLMediaElement::clearMediaPlayerAndAudioSourceProviderClientWithoutLocking(),
and in the cases where later steps depend on one of them, extract that before
resetting. Then it just cannot be out of sync after being cleared."

[philipj_slow, 2015-11-18 10:05:58]

On 2015/11/18 08:35:13, Srirama wrote:
> "Still, it would be nice to have some invariants, something like
> assertShadowRootChildren()" sounds good. I will add it.
> And what about this, should i check and do this as well?
> "Reset both readyState and networkState in 
>
HTMLMediaElement::clearMediaPlayerAndAudioSourceProviderClientWithoutLocking(),
> and in the cases where later steps depend on one of them, extract that before
> resetting. Then it just cannot be out of sync after being cleared."

Yeah, that's the combination I mean. An assert helper that can be a pre- and
post-condition for the places where m_readyState, m_networkState and
m_webMediaPlayer change. Exactly where to put the readyState and networkState
reset might need some testing, it's quite possible that the structure of the
code right now makes it impossible to simply add the asserts without also moving
things around.

[Srirama, 2015-11-18 14:07:36]

On 2015/11/18 10:05:58, philipj wrote:
> On 2015/11/18 08:35:13, Srirama wrote:
> > "Still, it would be nice to have some invariants, something like
> > assertShadowRootChildren()" sounds good. I will add it.
> > And what about this, should i check and do this as well?
> > "Reset both readyState and networkState in 
> >
>
HTMLMediaElement::clearMediaPlayerAndAudioSourceProviderClientWithoutLocking(),
> > and in the cases where later steps depend on one of them, extract that
before
> > resetting. Then it just cannot be out of sync after being cleared."
> 
> Yeah, that's the combination I mean. An assert helper that can be a pre- and
> post-condition for the places where m_readyState, m_networkState and
> m_webMediaPlayer change. Exactly where to put the readyState and networkState
> reset might need some testing, it's quite possible that the structure of the
> code right now makes it impossible to simply add the asserts without also
moving
> things around.

Should we reset network state as well, because when m_webMediaPlayer is null,
networkstate can either be EMPTY or IDLE.
For ex: in userCancelledLoad and loadResource(if it goes to mediaLoadingFailed()
path), m_webMediaPlayer can be null and networkstate will be IDLE.

[Srirama, 2015-11-18 14:09:54]

On 2015/11/18 14:07:36, Srirama wrote:
> On 2015/11/18 10:05:58, philipj wrote:
> > On 2015/11/18 08:35:13, Srirama wrote:
> > > "Still, it would be nice to have some invariants, something like
> > > assertShadowRootChildren()" sounds good. I will add it.
> > > And what about this, should i check and do this as well?
> > > "Reset both readyState and networkState in 
> > >
> >
>
HTMLMediaElement::clearMediaPlayerAndAudioSourceProviderClientWithoutLocking(),
> > > and in the cases where later steps depend on one of them, extract that
> before
> > > resetting. Then it just cannot be out of sync after being cleared."
> > 
> > Yeah, that's the combination I mean. An assert helper that can be a pre- and
> > post-condition for the places where m_readyState, m_networkState and
> > m_webMediaPlayer change. Exactly where to put the readyState and
networkState
> > reset might need some testing, it's quite possible that the structure of the
> > code right now makes it impossible to simply add the asserts without also
> moving
> > things around.
> 
> Should we reset network state as well, because when m_webMediaPlayer is null,
> networkstate can either be EMPTY or IDLE.
> For ex: in userCancelledLoad and loadResource(if it goes to
mediaLoadingFailed()
> path), m_webMediaPlayer can be null and networkstate will be IDLE.

Sorry, only in userCancelledLoad it can be IDLE.

[philipj_slow, 2015-11-18 14:23:29]

So, userCancelledLoad is a terrible name for what this actually does. It isn't
called when the user cancels load (there's no way to do that) and yet it
implements that bit of the spec, but clears the media player to actually
accomplish this.

Ideally userCancelledLoad should be reduced to just the bare minimum needed in
the destructor and in stop(), but perhaps not in this CL. Since we clear the
media player unconditionally, I think it would be good to also fire the emptied
event unconditionally, removing the branch that sets networkState to IDLE.

[philipj_slow, 2015-11-18 14:24:33]

On 2015/11/18 14:23:29, philipj wrote:
> So, userCancelledLoad is a terrible name for what this actually does. It isn't
> called when the user cancels load (there's no way to do that) and yet it
> implements that bit of the spec, but clears the media player to actually
> accomplish this.
> 
> Ideally userCancelledLoad should be reduced to just the bare minimum needed in
> the destructor and in stop(), but perhaps not in this CL. Since we clear the
> media player unconditionally, I think it would be good to also fire the
emptied
> event unconditionally, removing the branch that sets networkState to IDLE.

Sorry, it's called in the destructor and didMoveToNewDocument(), not stop(). It
still doesn't make much sense in either context, though :)

[foolip, 2016-05-25 10:23:19]

foolip@chromium.org changed reviewers:
+ foolip@chromium.org
- philipj@opera.com

[foolip, 2016-05-25 10:23:32]

Should this be closed?