Closed Bug 720591 Opened 12 years ago Closed 12 years ago

NoSquint 2.1.2 add-on causes zombie compartments

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: n.nethercote, Assigned: tack-bugzilla)

References

()

Details

(Whiteboard: [MemShrink:P3])

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
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".
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!
Whiteboard: [MemShrink]
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.
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
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. :)
> 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?
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.
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.
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?
But the browser is still alive, right?  So this isn't an orphaned cycle.
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.
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?
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
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.
The browser should detect all garbage cycles.  I'll try to reproduce this and see what is keeping it alive.
So is this NoSquint's fault or Firefox's fault?

I guess we can close this once a new version of NoSquint is released.
Assignee: nobody → tack-bugzilla
> 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...
(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.
Status: NEW → ASSIGNED
Whiteboard: [MemShrink] → [MemShrink:P3]
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.
Great!  Thanks for the quick action, Jason.
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.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → INVALID
Thanks so much for working with us, Jason!
Resolution: INVALID → FIXED
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
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/
(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
You need to log in before you can comment on or make changes to this bug.