425289 - Mousing over plugins on Mac/Linux leaks and prevents plugin from shutting down

Status: VERIFIED FIXED

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

Description

[Johnny Stenback (:jst)]

This is a regression from bug 420886. The problem, as Ben found out, is that UI events sent to an nsObjectFrame on the Mac doesn't go through the appshell event loop code, and thus the event loop nesting logic in nsPluginInstanceOwner doesn't work as expected. Specifically the event loop nesting level is typically 0 on the Mac when we enter nsObjectFrame::HandleEvent(), which means that the event loop nesting depth we remember as the last event loop nesting level in the nsPluginInstanceOwner is 0. This causes us to leak the nsPluginInstanceOwner since by the time we get to doing a delayed stop of a plugin, we're at event loop nesting level 1 (because we're processing a runnable event or a timer event), but the remembered event loop nesting level is 0, so we fire a timer to stop the plugin later, once that timer fires, we're still at the same level, and we repeat forever.

This has the unfortunate side effect of not only leaking the plugin instance owner, but also plugins that play audio can continue to do so until the app is shut down enve if the page with the plugin was closed.

Comment 1

[Johnny Stenback (:jst)]

This is a beta5 blocker.

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch

This fixes us by always incrementing the nesting level for events on mac and ensuring that we always have a level > 0 for other platforms.

Comment 3

[Mike Schroepfer]

The comment mentions linux for the else clause - is this case meant to be for
all non mac platforms?

Comment 4

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

What's the timer change for?

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch with fixed comments

We do want to do this for all non-mac platforms to be safe, fixed that comment.

The timer change is to tear down the plugin as quickly as possible - the 3000 that was there was really too long.

Comment 6

[Johnny Stenback (:jst)]

(In reply to comment #4)
> What's the timer change for?
> 

As Ben said, that's so that we don't wait 3 seconds in some cases to stop a plugin. Initially that didn't seem bad as we weren't aware of this being triggered except when using modal dialogs in plugins etc, but given that it can happen, a shorter timeout seems more appropriate so that plugins die faster even in those edge cases.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 311924 [details] [diff] [review]
Patch with fixed comments

r+sr=jst

Comment 8

[Mike Schroepfer]

How do we manually test this for possible regressions?  The build cycle is pretty long - so if we can suss out any issues in advance that would save us a ton of time.

Comment 9

[Mike Schroepfer]

Comment on attachment 311924 [details] [diff] [review]
Patch with fixed comments

a+ schrep - please land it.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Fixed on trunk.

Comment 11

[Carsten Book [:Tomcat]]

(In reply to comment #8)
> How do we manually test this for possible regressions?  The build cycle is
> pretty long - so if we can suss out any issues in advance that would save us a
> ton of time.
> 

If that helps:

I can run (leak) regression tests with my own debug builds and this patch applied into the build. Will test for this leak and also Bug 424719. Will test media/plugin related sites with priority.

Comment 12

[Tony Chung [:tchung]]

(In reply to comment #8)
> How do we manually test this for possible regressions?  The build cycle is
> pretty long - so if we can suss out any issues in advance that would save us a
> ton of time.
> 

Ben, is the testcase from 424719 comment 1 sufficient enough to verify this fix?

Comment 13

[Johnny Stenback (:jst)]

Fix checked in on GECKO19b5_20080326_RELBRANCH.

The easiest way to verify this is probably to load

  http://freearcade.com/SeaDiver.flash/SeaDiver.html

Wait for it to load and start playing the background music, make sure you move your mouse over the plugin, play the game a bit etc. The close the tab with this game loaded. If the background music continues to play, you're seeing this bug, if the background music stops, the bug is fixed.

Comment 14

[Carsten Book [:Tomcat]]

(In reply to comment #13)
> Fix checked in on GECKO19b5_20080326_RELBRANCH.
> 
> The easiest way to verify this is probably to load
> 
>   http://freearcade.com/SeaDiver.flash/SeaDiver.html
> 
> Wait for it to load and start playing the background music, make sure you move
> your mouse over the plugin, play the game a bit etc. The close the tab with
> this game loaded. If the background music continues to play, you're seeing this
> bug, if the background music stops, the bug is fixed.
> 

Hi Johnny, QA has done tests over the last hours and looks good on Mac Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008032701 Firefox/3.0pre ID:2008032701 and Windows.

And also so far no leaks found. Will do final tests with Linux and will set this bug then to verified fixed.

Comment 15

[Carsten Book [:Tomcat]]

also working on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b5) Gecko/2008032619 Firefox/3.0b5 

--> Verified fixed