Closed Bug 480819 Opened 15 years ago Closed 15 years ago

Orphan VIDEO elements are Zombies, audio continues after navigating away from page

Categories

(Core :: Audio/Video, defect, P2)

defect

Tracking

()

VERIFIED FIXED

People

(Reporter: BijuMailList, Assigned: roc)

References

Details

(Keywords: verified1.9.1)

Attachments

(7 files, 2 obsolete files)

Attached file zombie_1.html
Orphan VIDEO element are Zombies 
and can be a unexpected annoyance when closing a tab.

ie, orphan VIDEO element which was dynamically created
using document.createElement("video");
Or something which was remove videoParent.removeChild(videoNode);
Or something which was disappeared by videoParent.innerHTML="";
Or something which was wiped out by document.write();


BTW What all should be the capabilities of an orphan VIDEO element
* Should we allow videoNode.play(); on them?
* Can we adopt video node from one document to other while it is still playing?
  That will be a nice to have feature to solve bug 449539.


Steps to reproduce bug

Case 1
1. open attached zombies_1.html
2. you will hear something started playing
3. keep playing few seconds
4. Click "Navigate away to Google" link

Result:
video continues to play
i think till the end of it

Expected:
Video stop immediately




Case 2
1. open attached zombies_1.html
2. you will hear something started playing
3. keep playing few seconds till you hear sound of engine/motor
4. press reload button to refresh page


Result:
both video plays
ie, the first one with sound of engine/motor
i think at some time first one ends

Expected:
the first Video stop immediately
user should hear only current Video 



Case 3
1. open attached zombies_1.html
2. you will hear something started playing
3. keep playing few seconds
4. Close tab


Result:
video continues to play for few seconds


Expected:
Video stop immediately


Repeat same with attached zombie_2.html, zombie_3.html, zombie_4.html
on zombie_4.html the actual results are slightly different


FAIL
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2a1pre) 
Gecko/20090228 Minefield/3.2a1pre

FAIL
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) 
Gecko/20090223 Shiretoko/3.1b3pre
Flags: wanted1.9.1?
Flags: blocking1.9.2?
Flags: blocking1.9.1?
Attached file zombie_2.html
Attached file zombie_3.html
Attached file zombie_4.html
This might be annoying behaviour, but I'm not convinced that it's incorrect behaviour.  According to the HTML5 spec:

"Media elements that are potentially playing while not in a Document must not play any video, but should play any audio component. Media elements must not stop playing just because all references to them have been removed; only once a media element to which no references exist has reached a point where no further audio remains to be played for that element (e.g. because the element is paused, or because the end of the clip has been reached, or because its playbackRate is 0.0) may the element be garbage collected."
(In reply to comment #4)
> "Media elements that are potentially playing while not in a Document must not
> play any video, but should play any audio component.

Good, I can agree with spec.
So that part of issue we are doing as per spec.

But still it should not do what we fixed in bug 470636
ie, at present audio doesn't stop after navigating away from the page.

And Expected Result:-
  Audio should stop immediately after we navigate away from the page.
Summary: Orphan dynamically created VIDEO element are Zombies → Orphan VIDEO elements are Zombies, audio continues after navigating away from page
Attached file RickRolled.html
Warning !!! You will be Rick Roll'D
Bug 470636 is different, the media element is in the document in that case.  In this case, it's not, and the part of the spec I quoted applies.  Navigating away destroys the document, but the media element is not part of the document, so should continue to play per the spec.

I don't think there's a bug here, so resolving as invalid.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → INVALID
(In reply to comment #7)
> Bug 470636 is different, the media element is in the document in that case.

The effect is the same, though. Don't you agree that a site being able to cause audio to continue playing after you've navigated away from it is a bad thing?

It seems to me that the intent of that part of the spec is to ensure that video/audio elements can be used without being inserted into a document. I don't see any valid use cases for them outliving the associated document, or playing while the document is in bfcache, so I think the spec (and our implementation) should be fixed to include that limitation.
Status: RESOLVED → REOPENED
OS: Windows XP → All
Hardware: x86 → All
Resolution: INVALID → ---
This rather long thread goes into the history of how the spec ended up where it is now:

http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2007-October/012798.html

According to the thread it looks like audio objects outside of the document should not be garbage collected until the browsing context goes away.
Reading the spec in the context of that mailing list discussion convinces me that we should stop playing when the document becomes inactive.  I do think that the spec could be clearer on this issue, but this passage seems to cover this case:

"Note: If the media element's Document stops being an active document, then the playback will stop until the document is active again."
I think we actually will stop playing when the document is destroyed via cycle collection. The problem here is that our PresShell::Freeze code iterates through the nodes in the document to decide which ones to freeze ... but that misses the nodes that aren't in the document.

I think we need to add a list of a document's media elements to the document, so we can accurately freeze them all.
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Assignee: nobody → roc
Attached patch fix (obsolete) — Splinter Review
Make each document store the set of elements that it owns that may need to be frozen --- plugins and media elements.

The test code is a little tricky. Hopefully the README file explains what we're doing here. It's important to be able to ignore the destruction of plugin instances that were created before we started tracking them --- because of asynchronous plugin destruction there may be instances hanging around when the test starts that get destroyed later. This is especially a problem when you reload the testcase.
Attachment #375057 - Flags: superreview?(jst)
Attachment #375057 - Flags: review?(jst)
Whiteboard: [needs review]
Comment on attachment 375057 [details] [diff] [review]
fix

Isn't this patch missing code to register plugin elements as freezable elements?
Attached patch fix v2 (obsolete) — Splinter Review
Yes! My test was wrong, the iframe'd plugin was not going into bfcache at all because we don't bfcache iframes by default. So I've revised the test to use a separate window, and the test failed with the old patch. Here's the fixed patch and test.
Attachment #375057 - Attachment is obsolete: true
Attachment #376044 - Flags: superreview?(jst)
Attachment #376044 - Flags: review?(jst)
Attachment #375057 - Flags: superreview?(jst)
Attachment #375057 - Flags: review?(jst)
roc, looks like that last attachment is  a fix for another bug...
Attached patch fix v2Splinter Review
oops
Attachment #376044 - Attachment is obsolete: true
Attachment #376342 - Flags: superreview?(jst)
Attachment #376342 - Flags: review?(jst)
Attachment #376044 - Flags: superreview?(jst)
Attachment #376044 - Flags: review?(jst)
Comment on attachment 376342 [details] [diff] [review]
fix v2

r+sr=jst
Attachment #376342 - Flags: superreview?(jst)
Attachment #376342 - Flags: superreview+
Attachment #376342 - Flags: review?(jst)
Attachment #376342 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/fdd555ade242

There's no obvious way to test the actual sound output, but there is a test that plugins are actually stopped.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Flags: blocking1.9.2? → in-testsuite+
Resolution: --- → FIXED
Whiteboard: [needs review] → [needs 191 landing]
Tanx Roc...

Tested on both
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre)
Gecko/20090509 Minefield/3.6a1pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre)
Gecko/20090509 Minefield/3.6a1pre

a PASS for case 1/2/3 on zombie_1/2/3/4.html and RickRolled.html

PS: instead of stopping immediately, it will be after few (say 3) seconds
I disabled the test because it was causing focus problems: baf06abf0af4

(In reply to comment #19)
> PS: instead of stopping immediately, it will be after few (say 3) seconds

That's probably just the time required to drain the audio buffers. 3 seconds sounds a bit long though.
Flags: in-testsuite+ → in-testsuite?
(In reply to comment #20)
> 3 seconds sounds a bit long though.

on my PC usually it is a minimum of 3sec, 
Some times it even went to 10sec, I think we can live that for now.
Attached patch 1.9.1 patchSplinter Review
The test doesn't work on 1.9.1 because the test plugin stuff I need is not there, so I've removed that from the patch.

One thing I'm not sure about is the change to nsIDocument. Are we still allowed to rev its IID on the 1.9.1 branch? Or can we avoid revving the IID since I'm not changing any virtual methods?
I think I should just land this without revving the nsIDocument IID.
I've backed this out from 1.9.1 as it was causing crashes on the linux reftests and crashtests: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1242803401.1242808896.30194.gz&fulltext=1
The landing was
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8d98bfd5645a
The backout was
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/acd2d4638228
(thanks Dave)

We crashed after this test:
REFTEST TEST-PASS | file:///builds/slave/mozilla-1.9.1-linux-unittest/build/layout/reftests/bugs/84400-1.html |

Thread 0 (crashed)
 0  libxul.so!FreezeElement(nsIContent*, void*) [nsISupportsUtils.h : 203 + 0x3]
    eip = 0x40368b51   esp = 0xbfe372ec   ebp = 0xbfe37308   ebx = 0x40dbf5a0
    esi = 0x429e18f0   edi = 0x42958db4   eax = 0x00000000   ecx = 0x429e18f0
    edx = 0xbfe372f8   efl = 0x00010282
 1  libxul.so!EnumerateFreezables(nsPtrHashKey<nsIContent>*, void*) [nsDocument.cpp:c7babba8df56 : 7580 + 0xa]
    eip = 0x404b1936   esp = 0xbfe37310   ebp = 0xbfe37328
When merging to branch I turned do_QueryFrame into CallQueryInterface, but the former is null safe while the latter is not, which lead to this crash. So I added an "if (frame)", tests were OK, and I relanded.

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8c7f6dd7990b
Keywords: fixed1.9.1
Whiteboard: [needs 191 landing]
Verified FIXED on latest 1.9.1 branch and trunk, as well tested in b99

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090608 Shiretoko/3.5pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090608 Minefield/3.6a1pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b99) Gecko/20090604 Firefox/3.5b99
Status: RESOLVED → VERIFIED
(In reply to comment #22)
> Created an attachment (id=377621) [details]
> 1.9.1 patch
> 
> The test doesn't work on 1.9.1 because the test plugin stuff I need is not
> there, so I've removed that from the patch.

FYI, there's a blanket approval for landing tests and test harness changes on branches, so feel free to land test plugin changes on the branch.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: