2.15 KB, patch
|Details | Diff | Splinter Review|
13.58 KB, text/plain
2.89 KB, patch
Josh Aas: review+
Samuel Sidler (old account; do not CC): approval126.96.36.199+
|Details | Diff | Splinter Review|
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 188.8.131.52 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.
(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?
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?
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.
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
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 184.108.40.206).
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
Johnny, do you have any thoughts on this? We're technically code frozen for 220.127.116.11, 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.)
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?
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?
Dan and I talked this out and decided that we *won't* block 18.104.22.168 for this issue. That being said, we *will* block 22.214.171.124 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 126.96.36.199. 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.
This sounds like a flashblock bug to me. Trev, thoughts?
The same version flashblock doesn't cause this problem on previous versions of Firefox, so what changed?
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.
(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.
Josh and I are investigating...
Created attachment 359338 [details] [diff] [review] 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".
You probably want to see where we post that runnable...
The runnable from that stack trace is created in the DoDelayedStop call from DoStopPlugin's !doCache and !doCallSetWindowAfterDestroy code path.
Created attachment 359349 [details] 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.
How come it takes us up to 2 or so seconds to process that runnable? Assuming that's what's happening...
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.
(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 :)
Created attachment 359551 [details] [diff] [review] 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).
I'd rather we refactor the "get current event nesting level" function so that this code and ConsiderNewEventloopNestingLevel just run the same code.
Created attachment 359792 [details] [diff] [review] Better fix. Per bz's suggestion.
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).
Johnny: Any chance of getting this landed on mozilla-central and 1.9.1 today?
Thanks for the review! Fixed on trunk and 1.9.1
Johnny, I'm guessing this patch applies cleanly to 188.8.131.52? Please request approval if so.
Comment on attachment 359792 [details] [diff] [review] Better fix. Seem to apply cleanly (with fuzz), still needs testing tho.
(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 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 184.108.40.206. a=ss
Verified fixed, using Camino 2.0b2pre (220.127.116.11pre 2009020807)
Verified fixed for 18.104.22.168 with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:22.214.171.124pre) Gecko/2009021704 GranParadiso/3.0.7pre.
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.
This bug# was incorrectly listed in the checkin message for the fix for bug 475299.
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/
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.
Well, I don't think it's fixed, or it has re-appeared in Win 8.1 FF 25.0.1
(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?