Michael Gauthier [:gauthierm] Reporter
	Description • 15 years ago

According the the HTML5 draft specification, the user-agent should act like the pause() method is called if a media element is removed from the document and the networkState has a value other than NETWORK_EMPTY (0).

When a media element is removed in Firefox and the networkState is NETWORK_LOADING (2) the media keeps buffering, and then starts playing in the background.

See test case at http://labs.silverorange.com/files/mozilla/video-testcase/test-case.html

	Julien Levesy
	Comment 1 • 11 years ago

I'm interested in working on this bug.
I've got experience in c++ but, i'm new to firefox developpement.
So, i don't know really where to start.
Could someone help me to get in ?
Thanks

	Paul Adenot (:padenot)
	Comment 2 • 11 years ago

The code that gets called when the element is removed from the DOM is at [1].

After a quick look, it seems that the code is correct, but for some reason, |mPaused| is |true|, so we don't call the |Pause| method. We need to understand why |mPaused| it |true| at that point.

The testcase in comment 0 is still valid. What should happen is that it should not produce any sound (that is, the element should be paused when removed from the DOM).

[1]: http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/HTMLMediaElement.cpp#2277

	Paul Adenot (:padenot)
	Comment 3 • 11 years ago

Bastien is going to work on that. He is an ENSIMAG [1] student, and is trying to fix bugs in open-source software for a school project.

[1]: http://ensimag.grenoble-inp.fr/

	bastien42
	Comment 4 • 11 years ago

This patch corrects the bug 533840. There is also a test in content/media/test.

	Paul Adenot (:padenot)
	Comment 5 • 11 years ago

Chris, this is a patch by a french student I was mentoring a couple weeks back.
I cleaned it up and reworked the logic a bit, so I'd rather have someone else to
review it.

What is happening here is that we were starting the playback after the
::UnbindFromTree (using the autoplay logic), whereas per spec, we should have
paused the media.

	Paul Adenot (:padenot)
	Comment 6 • 11 years ago

try push: https://tbpl.mozilla.org/?tree=Try&rev=6ad23e2aeb21

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 7 • 11 years ago

Comment on attachment 767225 [details] [diff] [review]
Don't start playback when removed from a document and autoplaying.  r=

Review of attachment 767225 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/media/test/test_paused_after_removed.html
@@ +24,5 @@
> +  video.parentNode.removeChild(video);
> +  ok(video.paused, "Media should be paused.");
> +  // wait a bit to check we won't receive the a "play" event.
> +  setTimeout(function() {
> +    manager.finished(video.token);

Please set video.src = ""; to force the decoder to be shut down immediately.

@@ +25,5 @@
> +  ok(video.paused, "Media should be paused.");
> +  // wait a bit to check we won't receive the a "play" event.
> +  setTimeout(function() {
> +    manager.finished(video.token);
> +  }, 5000);

Maybe make the timeout smaller? Over enough runs we'll still catch failures.

	Paul Adenot (:padenot)
	Comment 8 • 11 years ago

Addressed changes and pushed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4f84868850f

	Ryan VanderMeulen [:RyanVM]
	Comment 9 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/a4f84868850f

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 10 • 11 years ago

So loadstart and play are both dispatched asynchronously.
It is possible that when we handle loadstart, the runnable to dispatch play is already in the
event loop and we end up dispatching the event even though the element isn't in document anymore.
That seems to cause bug 887421.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 11 years ago

I may back this out in order to get other stuff (which happens to cause the test failure to happen more often) landed.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 12 • 11 years ago

Rather than back this out, I think that we should change test_paused_after_removed.html to listen for "pause" rather than "play" events, and end the test in the "pause" handler. We can't be sure that the play event will never be in the event queue by the time that "loadstart" fires, so it's the best that we can do I think. That also removes the timeout in the test, making it faster.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 13 • 11 years ago

sounds good.

	Paul Adenot (:padenot)
	Comment 14 • 11 years ago

It won't work, we don't fire "pause" in this test.

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 15 • 11 years ago

We should fire "pause", shouldn't we?

	Paul Adenot (:padenot)
	Comment 16 • 11 years ago

Per spec, we should fire pause only if we are not paused. And we are paused when removing the element from the document, here (because we remove it right after "loadstart" has fired).

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 17 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6bd848b916c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc7af847469c

I decided to back this out, since I've had enough problems with broken tests.
I'll look at this once I know bug 789919 sticks in.

	Ryan VanderMeulen [:RyanVM]
	Comment 18 • 11 years ago

(In reply to Olli Pettay [:smaug] from comment #17)
> I decided to back this out, since I've had enough problems with broken tests.
> I'll look at this once I know bug 789919 sticks in.

I think it stuck :)

	Maire Reavy [:mreavy]
	Comment 19 • 9 years ago

Smaug -- noticed this when I was doing bug clean up.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 20 • 9 years ago

Comment on attachment 767225 [details] [diff] [review]
Don't start playback when removed from a document and autoplaying.  r=

So I tried to look at it again, but I don't understand
mWasInDocument && IsInDoc(). (Also, I'm not at all familiar with the media code.)

If we are in document, we sure should have mWasInDocument == true.
Also, looks like HTMLMediaElements have now mElementInTreeState.
Should that be reused here? It was added in bug 1095438.
Though, it is not clear to me what ELEMENT_INTREE actually means.
document.createElement("div").innerHTML = "<audio>"; will set it, but should it be set only when media element is inserted into document?
Similar with ELEMENT_NOT_INTREE_HAD_INTREE.

Perhaps Benjamin could take a look at this?
(I assume this bug is still valid.)