Last Comment Bug 720591 - NoSquint 2.1.2 add-on causes zombie compartments
: NoSquint 2.1.2 add-on causes zombie compartments
Status: RESOLVED FIXED
[MemShrink:P3]
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Jason Tackaberry
:
Mentors:
https://addons.mozilla.org/en-US/fire...
Depends on:
Blocks: LeakyAddons ZombieCompartments
  Show dependency treegraph
 
Reported: 2012-01-23 18:28 PST by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2012-06-19 13:00 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2012-01-23 18:28:43 PST
The add-on is at https://addons.mozilla.org/en-US/firefox/addon/nosquint/.

The zombie is filed as an issue on GitHub: https://github.com/jtackaberry/nosquint/issues/1:

> When NoSquint is enabled FireFox is unable to free memory for closed windows. 
> To reproduce:
>
>   1. Disable NoSquint in the FireFox Add-ons Manager
>   2. Restart FireFox
>   3. Open about:memory
>   4. Open a new FireFox window (not a tab)
>   5. In the new window, load http://www.cnn.com
>   6. Close the new window
>   7. Hit F5 in the original window
>   8. See where it says compartment(http://www.cnn.com/)?
>   9. Continue to hit F5 until this goes away
>   10. The memory is now freed
>   11. Enable NoSquint in the FireFox Add-ons Manager
>   12. Repeat steps 2 through 8
>   13. The memory will never be freed
Comment 1 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-01-23 18:35:06 PST
I emailed the author (Jason Tackaberry).

BTW, in comment 0, a better step 7 would be "click the 'Minimize Memory Usage' button in about:memory until the compartment goes away".
Comment 2 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-01-23 18:39:20 PST
Jason (the author) responded very quickly:

> My plan was to dig into this last week but I got pulled on the usual
> work/life issues.  Based on my current schedule it's looking like Sunday
> is my next available hacking day.

So that is promising!
Comment 3 Jason Tackaberry 2012-01-25 18:07:44 PST
I was able to reproduce the problem and have released 2.1.3 on my website at https://urandom.ca/nosquint/ which should hopefully fix it.

I've asked the submitter of the bug on GitHub to confirm.  Once he does, and I give this version a few days to bake on my website, I'll upload it to AMO.
Comment 4 Danial Horton 2012-01-29 22:23:15 PST
Its funny that i reported this very memory leak to the developer more then 5 years ago.

the developer of default full zoom was also perplexed that i'd recommend his plugin over the seemingly superior nosquint because of it :D
Comment 5 Jason Tackaberry 2012-01-30 05:37:08 PST
I did get a report from Danial on 2009-07-08 (not exactly more than 5 years ago, but a while ago anyway) in which he provided a series of steps to reproduce the problem that involved opening 50 youtube tabs and closing them.  But I wasn't able to reproduce the problem.

I'd periodically received vague reports of memory leaks throughout the years.  I never observed the problems myself, so I wasn't able to do anything about it.

Not until now did I receive a very clear description of how to repeat the problem that I could actually reproduce, along with a good technical explanation of what was happening (the zombie compartments wiki).  It turns out the leak reported only applied to multiple windows, not tabs, and since I only ever use tabs, I never ran into this myself.

Of course, the most ideal solution would be for Firefox's GC to handle more cases like forgotten progress listeners. :)
Comment 6 Justin Lebar (not reading bugmail) 2012-01-30 06:38:44 PST
> Of course, the most ideal solution would be for Firefox's GC to handle more cases like forgotten 
> progress listeners. :)

It's a really hard problem in general, but we've been thinking about it.  Would you mind showing us the patch which fixes the problem?
Comment 7 Jason Tackaberry 2012-01-30 06:48:31 PST
The commit that fixed the particular reported leak is at https://github.com/jtackaberry/nosquint/commit/a2ae61fd74375c0903747742173f40ba5c3ed4e2

The new lines in the patch were added to the window unload handler.  This was already being done on TabClose.

I also noticed that I'm adding observers at window load time, but not removing them on unload.  This didn't seem to manifest as a zombie compartment, but I'll make sure I clean that up as well just in case.
Comment 8 Justin Lebar (not reading bugmail) 2012-01-30 06:54:34 PST
I think actually progress listeners may be weak references -- that is, you don't need to explicitly remove them.  But you do need to browser.setUserData('nosquint', null, null), because that gets rid of a reference to the progress listener!

Not that the code which removes the progress listener is bad, but would you mind seeing if it still leaks with just the setUserData() call?

We might be able make the progress listeners themselves hold a weak reference to the listened object, so this doesn't happen.  Also, it seems pretty nuts that TabClose doesn't fire for each closed tab.
Comment 9 Jason Tackaberry 2012-01-30 07:00:37 PST
Sure, I'll test that out tonight and report back.

Even if the listeners hold strong references, I wouldn't have expected this to be a problem since (as I understand it), Firefox's GC can collect objects involved in orphaned reference cycles.  Or is this just one of a handful of special cases where it fails?
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-01-30 07:04:10 PST
But the browser is still alive, right?  So this isn't an orphaned cycle.
Comment 11 Justin Lebar (not reading bugmail) 2012-01-30 07:07:45 PST
To be clear, I think the refs are:

 (1) browser -s-> userData -s-> progress listener -s-> browser

and

 (2)  browser -w-> progress listener -s-> browser

where -s-> is a strong ref and -w-> is a weak ref.  We can't break the cycle in (1) automagically, but we may be able to change the final strong ref in (1) into a weak ref.
Comment 12 Jason Tackaberry 2012-01-30 07:10:12 PST
I understand the browser is kept alive because of the refcycle, but isn't it orphaned?  What external objects are holding a reference to either the browser or the progress listener?
Comment 13 Justin Lebar (not reading bugmail) 2012-01-30 07:24:14 PST
I'm not sure about comment 12, but Andrew might be able to help us.

I now understand that ProgressListener is NoSqint's own class.  You could use a weak ref there, I think.  (Maybe you don't need a ref to the browser at all, if you can go from the |progress| arg to a browser, but you know chrome JS better than I do.)

http://stackoverflow.com/questions/5119123/weak-reference-in-firefox-javascript-chrome-code
Comment 14 Jason Tackaberry 2012-01-30 07:47:11 PST
Yeah, I actually I have it as a TODO in the code to use gBrowser.addTabsProgressListener() to hook a global progress listener (which passes the browser as an argument to the handlers) rather than per-tab listeners, which I plan to do once I drop support for Firefox 3.x (which I'll be doing in the next major release).

As it is now, with per-tab listeners, I'm not sure it's possible to get the browser object from the progress arg.

Anyway, this is somewhat academic.  I think I'd rather use strong refs and explicitly clean up the references than introduce weakrefs in strategic places to avoid refcycles.  The latter feels more volatile to me in the sense that I may forget how that works after a few months or years and end up breaking it again.

The best option of course is to detect and collect garbage objects involved in these kinds of cycles, since it means other add-ons benefit without having to do anything.  I think it must be the case that the progress listener is kept alive in some internal machinery, which is keeping the browser alive due to the ProgressListener -s-> browser reference.  This could be an area to look into.
Comment 15 Andrew McCreight [:mccr8] 2012-01-30 07:48:53 PST
The browser should detect all garbage cycles.  I'll try to reproduce this and see what is keeping it alive.
Comment 16 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-01-30 14:39:21 PST
So is this NoSquint's fault or Firefox's fault?

I guess we can close this once a new version of NoSquint is released.
Comment 17 Justin Lebar (not reading bugmail) 2012-01-30 14:47:30 PST
> So is this NoSquint's fault or Firefox's fault?

Unclear pending investigation in comment 15.  But that should be a separate bug, if it pans out.

The fact that we don't send TabClosed events when the window closes is pretty redonculous, tho.  I'm sure there's a good reason for it in our chrome...
Comment 18 Jason Tackaberry 2012-01-30 16:06:54 PST
(In reply to Justin Lebar [:jlebar] from comment #8)
> Not that the code which removes the progress listener is bad, but would you
> mind seeing if it still leaks with just the setUserData() call?

As you expected, calling browser.removeProgressListener() wasn't necessary.
Comment 19 Jason Tackaberry 2012-02-01 14:43:36 PST
I uploaded version 2.1.4 to AMO which fixes the leaks reported here and also a possible leak due to not removing a DOMFrameContentLoaded listener on tab close (not sure if this is necessary, but it won't hurt to do).

It's now pending approval.
Comment 20 Nicholas Nethercote [:njn] (on vacation until July 11) 2012-02-01 15:35:01 PST
Great!  Thanks for the quick action, Jason.
Comment 21 Jason Tackaberry 2012-02-01 19:57:13 PST
It's now approved on AMO, so this bug can be closed.

Although I can resolve this bug, I can't seem to resolve it as fixed.  The only resolution codes available are INVALID, WONTFIX, DUPLICATE, WORKSFORME, and INCOMPLETE, none of which are appropriate.

So I'll let Nicholas close the bug.
Comment 22 Justin Lebar (not reading bugmail) 2012-02-01 20:15:51 PST
Thanks so much for working with us, Jason!
Comment 23 R.N. (Roger) Folsom 2012-06-19 11:25:02 PDT
Jason Tackaberry:

The SeaMonkey bug section of Bugzilla (https://bugzilla.mozilla.org/enter_bug.cgi#h=dupes|SeaMonkey) says "Please summarise your issue or request in one sentence:" I replied "NoSquint not available for SeaMonkey 2.1 and up"

But this thread is all about Firefox.

My laptop has a high resolution UXGA (1600x1200 pixels) 15" screen, and the ability to increase (or occasionally decrease) font size without altering images by using Ctrl++ or Ctrl+- is extremely useful.  Also useful is SeaMonkey NoSquint's ability to remember settings for frequently used website.

So I am stuck on using SeaMonkey 2.0.14 instead of the latest version.

Is there any chance that your NoSquint, or at least its font size change capability, will ever be ported to SeaMonkey?

I would do it myself, but I'm not a programmer, although I did use Fortran in the 1960s while in graduate school.  But nothing in this thread looks anything like ancient Fortran!

Roger Folsom
Comment 24 Justin Lebar (not reading bugmail) 2012-06-19 11:33:57 PDT
Roger, this isn't the right forum for your request.  Please redirect your conversation to the AMO page [1], or direct correspondence with the add-on author.

[1] https://addons.mozilla.org/en-US/firefox/addon/nosquint/
Comment 25 R.N. (Roger) Folsom 2012-06-19 13:00:22 PDT
(In reply to Justin Lebar [:jlebar] from comment #24)
> Roger, this isn't the right forum for your request.  Please redirect your
> conversation to the AMO page [1], or direct correspondence with the add-on
> author.
> 
> [1] https://addons.mozilla.org/en-US/firefox/addon/nosquint/

Justin:

Thanks for the advice.  Before getting your suggestion, I filed a bug report at
https://bugzilla.mozilla.org/show_bug.cgi?id=766254

Other SeaMonkey users have tried direct correspondence with Jason Tackaberry, with no results.  Apparently he's focused on Firefox, which admittedly has more users than SeaMonkey does.

My posts requesting a NoSquint for SeaMonkey > 2.0.14 on MozillaZine also have not worked.

But I am certainly willing to post a request for a NoSquint for SeaMonkey > 2.0.14 to the "AMO page [1]," but I will need to know what AMO means and where it is located.

I really don't want to have to switch to a Firefox/Thunderbird combination.  I've been using a combined browser-email package beginning with early Netscape versions in 1995, then the Mozilla Suite, then SeaMonkey.

Thanks again for your response.

R.N. (Roger) Folsom

Note You need to log in before you can comment on or make changes to this bug.