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

VERIFIED FIXED in mozilla1.9beta5

Status

()

Core
Plug-ins
P1
normal
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: jst, Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

({memory-leak})

Trunk
mozilla1.9beta5
memory-leak
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
This is a beta5 blocker.
Flags: blocking1.9+
Priority: -- → P1
Target Milestone: --- → mozilla1.9beta5
(Reporter)

Updated

10 years ago
Depends on: 420886
Created attachment 311921 [details] [diff] [review]
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.
Attachment #311921 - Flags: review?(jst)
Version: unspecified → Trunk

Comment 3

10 years ago
The comment mentions linux for the else clause - is this case meant to be for
all non mac platforms?
What's the timer change for?
Created attachment 311924 [details] [diff] [review]
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.
Attachment #311921 - Attachment is obsolete: true
Attachment #311924 - Flags: review?
Attachment #311921 - Flags: review?(jst)
Attachment #311924 - Flags: review? → review?(jst)
(Reporter)

Comment 6

10 years ago
(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.
(Reporter)

Comment 7

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

r+sr=jst
Attachment #311924 - Flags: superreview+
Attachment #311924 - Flags: review?(jst)
Attachment #311924 - Flags: review+

Comment 8

10 years ago
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

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

a+ schrep - please land it.
Attachment #311924 - Flags: approval1.9+
Fixed on trunk.
(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

10 years ago
(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?

Updated

10 years ago
Keywords: mlk
(Reporter)

Comment 13

10 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: mlk
Resolution: --- → FIXED
(Reporter)

Updated

10 years ago
Keywords: mlk
(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.

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

--> Verified fixed 
Status: RESOLVED → VERIFIED
OS: Linux → All
Hardware: PC → All
You need to log in before you can comment on or make changes to this bug.