474022 - sound from Flash continues to play when the containing tab/window is closed (with Flashblock)

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[[e-mail]]

Flash/Flashblock is broken on recent trunk builds.
Enable Flashblock in Camino or install Flashblock in Minefield, then play back a flash movie. Close the tab or window containing the flash movie and notice how the sound still play on.

Verified using todays Camino and Minefield (Flashblock 1.5.7.1 installed).
Regression date seem to be January 7th. As the Camino build from January 7th. works as expected (silence), while the January 8th. fail (sound continue).
Hence I suspect the checkins for bug 445520 or 473930 to be the guilty one.

Comment 2

[Chris Lawson (gone)]

(In reply to comment #1)
> 
> *** This bug has been marked as a duplicate of bug 437925 ***

I'm not so sure; this appears to be a recent regression, at least for Camino. (I suppose it's possible one of those checkins exposed something to embeddors that wasn't before.)

Kai, were you able to confirm the same regression window for Minefield?

Comment 3

[[e-mail]]

Don't seem to be the same regression window in Minefield.
Tested Minefield builds from January 6, 7 and 8. All of them continue to play sound after closing the tab/window.

While on Camino trunk there were no issues (afaik) up to the January 8. build. The January 7 build cut audio instantly when I close the tab/window, as expected.
Hence I think this Camino regression is sort of unrelated to bug 437925, but rather caused by checkins for bug 438830 or bug 445520 ...Who both landed within the regression window (January 7).

Perhaps this bug should be re-opened and moved to Product -> Camino?

Comment 4

[[e-mail]]

For your information.
Backing out the patch for bug 438830 took care of it on Camino trunk. The sound now stop instantly when the tab/window is closed.

Comment 5

[Chris Lawson (gone)]

Reopening per comment 4, then, since this is definitely not a dupe (although it's possible that it's another Camino-only Core bug).

cl

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

This is a recent regression on 1.9.0, with the causing bug already identified by a backout, so requesting blocking (though I'm aware more investigation may be needed than can be done before 1.9.0.6).

Comment 7

[Matthias Versen [:Matti]]

I don't think that this is a camino only bug, see bug 471956 which may be a dupe of this one while bug 437925 is a diffrent older different issue

Comment 8

[Samuel Sidler (old account; do not CC)]

Johnny, do you have any thoughts on this? We're technically code frozen for 1.9.0.6, but if your bug 438830 has another follow up (this bug), it might be worth taking...

Philip, any ideas on your end?

(fwiw, bug 437925 is definitely different since it was filed with Firefox 3.0 which didn't have that patch landed. Bug 437925 is probably not valid as filed, but the comments and dupes *are* valid and belong in this bug. i.e., that bug morphed.)

Comment 9

[Philip Chee]

According to Bug 474153 somehow Flashblock keeps the flash plugin instances alive even after all the tabs and windows are closed. Based on other reports this happens even in whitelist mode (where we bind to flash objects but don't actually remove them from the DOM). I'd guess that something is keeping a reference open but I don't see how as our XBL goes away when the content window closes.

Boris wasn't there an earlier case where there was a double frame constructed causing two flash movies to appear on the screen although there was only one object in the DOM?

Comment 10

[Boris Zbarsky [:bzbarsky]]

Sure, but in general any time that happens it's a really serious problem and we aim to fix it ASAP.

Is that happening here?

Comment 11

[Samuel Sidler (old account; do not CC)]

Dan and I talked this out and decided that we *won't* block 1.9.0.6 for this issue. That being said, we *will* block 1.9.0.7 on this issue and I'm assigning this bug to Johnny for now. Johnny, please re-assign as needed, but this bug is a top priority to fix for 1.9.0.7.

In Firefox, the issue isn't as bad as Camino. Audio doesn't continue playing indefinitely, only for a couple/three seconds on Mac and about half a second on Windows.

For reference, full STR:
  1. Install Flashblock
  2. Restart
  3. (Flashblock will be enabled by default)
  4. Open a new tab with a YouTube video (making sure you have a total of 2
     or more tabs open)
  5. Un-block the Flash video in YouTube
  6. Close the tab with the YouTube video

Note also that I can reproduce both in a GranParadiso nightly and a Shiretoko nightly. This should block 1.9.1 and is a regression from 3.0 (but, obviously, not 3.0.6 because we're going to ship with this bug).

Apologies to Camino because their nightlies will still show this bug until it's fixed but since final releases aren't occurring yet for them off of 1.9.0, we think we can fix it in the next 1.9.0.x release.

Comment 12

[Johnny Stenback (:jst)]

This sounds like a flashblock bug to me. Trev, thoughts?

Comment 13

[Philip Chee]

The same version flashblock doesn't cause this problem on previous versions of Firefox, so what changed?

Comment 14

[Wladimir Palant]

Sorry, Johnny - I just don't see how this can be a Flashblock bug. I wouldn't know how to keep a plugin instance alive from an extension even if I wanted. FWIW I cannot reproduce that issue in Minefield 20080118 build on Windows, is it Mac only? For a minimal testcase you can try a document with an embed, dynamically remove the "src" attribute of this embed then add it back (maybe also remove it from document and reinsert in between) - that's more or less what Flashblock does.

Comment 15

[Samuel Sidler (old account; do not CC)]

(In reply to comment #14)
> FWIW I cannot reproduce that issue in Minefield 20080118 build on Windows, is
> it Mac only?

No, but I tested on a Windows VM which is clearly resource-constrained. I only heard about a half-second (or less) of the audio after closing the tab, as opposed to Mac where I heard it for 2-3 seconds.

Comment 16

[Johnny Stenback (:jst)]

Josh and I are investigating...

Comment 17

[Josh Aas]

Attachment: stack trace from delayed stop #1

Installed Flashblock in a trunk debug build, started playing a YouTube music video and closed the YouTube tab, broke on "nsPluginHostImpl::StopPluginInstance".

Comment 18

[Boris Zbarsky [:bzbarsky]]

You probably want to see where we post that runnable...

Comment 19

[Josh Aas]

The runnable from that stack trace is created in the DoDelayedStop call from DoStopPlugin's !doCache and !doCallSetWindowAfterDestroy code path.

Comment 20

[Josh Aas]

Attachment: where we generate delayed stop runnable #1

This is the stack trace for where we generate the runnable that eventually stops the YouTube plugin instance.

Comment 21

[Johnny Stenback (:jst)]

How come it takes us up to 2 or so seconds to process that runnable? Assuming that's what's happening...

Comment 22

[Johnny Stenback (:jst)]

It's the event loop nesting level code in nsStopPluginRunnable::Run() that doesn't work right here. We end up in this case with a last even loop nesting level of 0, and a current one of 1, and it seems like the plugin is *never* stopped here. IOW, we keep re-firing a 100ms timer in that code, over and over. And if I load youtube again etc and close it, I get one more of those never ending repeating 100ms timers.

Josh, nsPluginInstanceOwner's mLastEventloopNestingLevel is initialized to 0 in this case, and that never goes up, only down, which leads to trouble here since once we get into nsStopPluginRunnable::Run(), the current nesting level is always 1 here, so we're stuck in an endless loop firing timers.

Comment 23

[Johnny Stenback (:jst)]

(In reply to comment #11)
> In Firefox, the issue isn't as bad as Camino. Audio doesn't continue playing
> indefinitely, only for a couple/three seconds on Mac and about half a second on
> Windows.

Sam, you're making stuff up here, this is not an issue on Windows :)

Comment 24

[Johnny Stenback (:jst)]

Attachment: Fix.

This fixes this problem by making sure we never end up with a mLastEventloopNestingLevel of 0, which means we'll never stop a plugin. The reason this is happening here is probably because we come in an initiate the plugin from a UI event (by clicking on the flashblock icon), which are apparently treated special on the Mac (see the code at http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp#434).

Comment 25

[Boris Zbarsky [:bzbarsky]]

I'd rather we refactor the "get current event nesting level" function so that this code and ConsiderNewEventloopNestingLevel just run the same code.

Comment 26

[Johnny Stenback (:jst)]

Attachment: Better fix.

Per bz's suggestion.

Comment 27

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 359792 [details] [diff] [review]
Better fix.

I don't understand the DoDelayedStop() and nsStopPluginRunnable code
very well.  But on the assumption that it "just works", this patch
looks fine to me.

I've also tested it on the current mozilla-central tree, and it does
fix the bug (as described in comment #11).

Comment 28

[Samuel Sidler (old account; do not CC)]

Johnny: Any chance of getting this landed on mozilla-central and 1.9.1 today?

Comment 29

[Johnny Stenback (:jst)]

Thanks for the review!

Fixed on trunk and 1.9.1

Comment 30

[Samuel Sidler (old account; do not CC)]

Johnny, I'm guessing this patch applies cleanly to 1.9.0.7? Please request approval if so.

Comment 31

[Johnny Stenback (:jst)]

Comment on attachment 359792 [details] [diff] [review]
Better fix.

Seem to apply cleanly (with fuzz), still needs testing tho.

Comment 32

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #31)
> (From update of attachment 359792 [details] [diff] [review])
> Seem to apply cleanly (with fuzz), still needs testing tho.

I tested this with Camino as soon as it appeared, and it did fix the bug (as well as the likely-related bugs about persistent high CPU usage even after closing  pages with Flashblock-viewed Flash).

Comment 33

[Samuel Sidler (old account; do not CC)]

Comment on attachment 359792 [details] [diff] [review]
Better fix.

Johnny, I'm going to approve this based on Smokey's testing. Please land it ASAP since code freeze is in ~26 hours. QA will come by and verify it after it's landed.

Approved for 1.9.0.7. a=ss

Comment 37

[[e-mail]]

Verified fixed, using Camino 2.0b2pre (1.9.0.7pre 2009020807)

Comment 38

[Al Billings [:abillings - ex-MoCo]]

Verified fixed for 1.9.0.7 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7pre) Gecko/2009021704 GranParadiso/3.0.7pre.

Comment 39

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.1 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090213 Shiretoko/3.1b3pre.

Comment 40

[Boris Zbarsky [:bzbarsky]]

This bug# was incorrectly listed in the checkin message for the fix for bug 475299.

Comment 41

[Kshitij Chawla]

using firefox 15 aurora, with flashblock. I had loaded a video containing page, with the video not clicked on and hence not loaded. I was surfing other pages when suddenly the audio of the video started playing without any user intervention. The video is still not loaded but the audio is still playing even after i closed the tab.

http://gigaom.com/mobile/video-why-im-enjoying-googles-newest-chromebook/

Comment 42

[u52928]

I'm in the wrong place here because my problem is with Win and not Macintosh, but I think this is a bad bug, and it seems to be cross platform.

Anyway, a new Flash version has just come out (11.4.402.265), but this old FF problem still plagues the Win7 Nightly, at least the 64 bit. The audio even keeps playing when the tab in which the video is playing is closed. Looking at the third bug mentioned below, it appears to be related to the FlashBlock addon.

By the way, it works fine in good old Namoroka and Aurora on 32-bit Win XP-SP3.

There are several bugs that seem related 588101, 437925, perhaps most importantly 474022, and others.

Since - given the limits of my testing abilities - I can only verify it in the 64-bit Nightly. On the FF Forum, I've opined that I think it'd be good if other people checked it out to see how wide spread it actually is.

Comment 43

[Ingo Rucker]

Well, I don't think it's fixed, or it has re-appeared in Win 8.1 FF 25.0.1

Comment 44

[Georg Fritzsche [:gfritzsche]]

(In reply to Ingo Rucker from comment #43)
> Well, I don't think it's fixed, or it has re-appeared in Win 8.1 FF 25.0.1

Can you please file a new bug on this and add more details on the problem you're seeing and how to trigger it?