GM_registerMenuCommand is causing zombie compartments #1578

Closed
matachi opened this Issue Jul 2, 2012 · 5 comments

Comments

Projects
None yet
4 participants
@matachi

matachi commented Jul 2, 2012

Hi!

I noticed that a userscript was causing zombie compartments to appear in my Firefox, and together with the script author we managed to pinpoint the cause to GM_registerMenuCommand.
Here is the thread about the issue: http://userscripts.org/topics/113805

Basically, do the following steps to get zombie compartments:

  1. Install http://userscripts.org/scripts/show/109262
  2. Visit a random site, such as google.com.
  3. Click the Greasemonkey icon in the add-on bar.
  4. Click outside the menu that appears to close it.
  5. Close google.com.
  6. Open about:compartments and the compartment for google is still left.

This error is present in Firefox 13. In Firefox Aurora 15 you don't even need to open the Greasemonkey menu to get zombie compartments from the open page - you get them from every page visit. Here is a video that I recorded of the process: http://www.flickr.com/photos/81272265@N04/7473372344/in/photostream
Here is how it looks like after opening and closing a few sites: http://imgur.com/a/gLf0i

To prevent the zombie compartments from appearing with this script, comment out line 113 with:

GM_registerMenuCommand('Set up Mouseover Popup Image Viewer', setup);

Would be nice if you could take a look at this, since it is in the end causing a major memory leak.

(We have only tried Greasemonkey 0.9.20)

@arantius

This comment has been minimized.

Show comment
Hide comment
@arantius

arantius Jul 2, 2012

Collaborator

Thanks for the detailed report. That's a huge script, and flickr isn't a simple page. Reducing to a simple test case will help expedite a fix.

Collaborator

arantius commented Jul 2, 2012

Thanks for the detailed report. That's a huge script, and flickr isn't a simple page. Reducing to a simple test case will help expedite a fix.

@matachi

This comment has been minimized.

Show comment
Hide comment
@matachi

matachi Jul 3, 2012

So you think that the problem might originate from something else?

matachi commented Jul 3, 2012

So you think that the problem might originate from something else?

@Ventero

This comment has been minimized.

Show comment
Hide comment
@Ventero

Ventero Jul 3, 2012

Contributor

First of all, here's a gist containing a reduced testcase. The steps to reproduce are the same as in the initial report.

There are actually 2 zombie compartment leaks in Greasemonkey: The first occurs (as described in the initial report) simply when opening the monkey menu when a menu commander is registered for the current tab. This can be fixed by adding an "popuphiding" event listener to the menupopup, and clearing out the "User Script Commands" menupopup when the outer menupopup closes. A patch can be found in this commit. After applying this patch, the initial bug report seems to be fixed for me.

The second leak occurs when actually hovering over the commands menupopup. When this happens, GM_MenuCommander.onPopupShowing adds a menuitem with an event listener to the popup. The problem is that this event listener never gets removed. It can be verified that after applying this patch, the leak seems to be fixed. I'm unsure though if attaching the command function as an expando property and calling it directly has any unwanted side effects (@arantius, if you want me to push that patch to my fork so you can pull it, just let me know).

Contributor

Ventero commented Jul 3, 2012

First of all, here's a gist containing a reduced testcase. The steps to reproduce are the same as in the initial report.

There are actually 2 zombie compartment leaks in Greasemonkey: The first occurs (as described in the initial report) simply when opening the monkey menu when a menu commander is registered for the current tab. This can be fixed by adding an "popuphiding" event listener to the menupopup, and clearing out the "User Script Commands" menupopup when the outer menupopup closes. A patch can be found in this commit. After applying this patch, the initial bug report seems to be fixed for me.

The second leak occurs when actually hovering over the commands menupopup. When this happens, GM_MenuCommander.onPopupShowing adds a menuitem with an event listener to the popup. The problem is that this event listener never gets removed. It can be verified that after applying this patch, the leak seems to be fixed. I'm unsure though if attaching the command function as an expando property and calling it directly has any unwanted side effects (@arantius, if you want me to push that patch to my fork so you can pull it, just let me know).

@sxe

This comment has been minimized.

Show comment
Hide comment
@sxe

sxe Jul 5, 2012

I can confirm this bug.
Figured it out by myself, while reducing the memory consumption of firefox (aurora).
Right now i'm removing the GM_registerMenuCommand() calls, as a workaround.

sxe commented Jul 5, 2012

I can confirm this bug.
Figured it out by myself, while reducing the memory consumption of firefox (aurora).
Right now i'm removing the GM_registerMenuCommand() calls, as a workaround.

arantius added a commit to arantius/greasemonkey that referenced this issue Aug 1, 2012

arantius added a commit to arantius/greasemonkey that referenced this issue Aug 1, 2012

Remove script command items when the monkey menu closes to prevent zo…
…mbie compartments.

Refs #1578

Conflicts:

	content/browser.js

@arantius arantius closed this in b89b0a5 Aug 1, 2012

@arantius

This comment has been minimized.

Show comment
Hide comment

arantius added a commit to arantius/greasemonkey that referenced this issue Aug 1, 2012

Fix isDeadWrapper detection failure.
Can't assign to Components.utils; define a helper instead.

Refs #1578
Refs #1594

arantius added a commit to arantius/greasemonkey that referenced this issue Aug 1, 2012

Do not attempt to assign to a property of Components.utils.
Like 912cd95 for the 0.9 branch, but cleaner syntax given a night to sleep on it.

Refs #1578
Refs #1594

arantius added a commit to arantius/greasemonkey that referenced this issue Aug 1, 2012

Rewrite .contentDestroyed() to be more paranoid.
This version is more aggressive and explicit about removing stale menu command objects, which hold references to content windows, and thus can leak memory if never cleared.

Refs #1578
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment