296474 - Memory leak when customizing toolbars

Status: RESOLVED WORKSFORME

(Firefox :: Toolbars and Customization, defect)

Description

[Boris Zbarsky [:bzbarsky]]

BUILD: Current trunk firefox:

STEPS TO REPRODUCE:
1)  env XPCOM_MEM_LEAK_LOG=/tmp/log.txt firefox
2)  Right-click on "Home" button and choose the "Customize..." menu option
3)  Click "Done" in the resulting dialog
4)  Quit.

EXPECTED RESULTS: no leaks

ACTUAL RESULTS: leaked 25 nsDocument objects, 2 nsGlobalWindow objects, etc, etc.

OTHER INFORMATION:  The problem is somehow tied to the wrapToolbarItems()
function in customizeToolbars.js.  To be precise, the insertBefore() calls in
this function are needed for the leak -- if I always make it take the
appendChild path, the leak goes away.  It also doesn't matter whether I leave the 

  var wrapper = wrapToolbarItem(item);

line as-is or replace it with:

  var wrapper = item;

I haven't been able to reproduce this with an HTML testcase using insertBefore,
so far....

Comment 1

[Martijn Wargers (dead)]

Maybe bug 291781 has something to do with this? That bug describes a situation
where wrapToolbarItem() removes an element that has controllers attached to it
(there is a patch in bug 246158 that could fix that).

Comment 2

[Boris Zbarsky [:bzbarsky]]

Possible...  XUL elements do release their controllers when removed from the
tree to prevent leaks through them, though which is what bug 291781 is really
about... So I'm a little doubtful that that's the issue, but maybe...

Comment 3

[Boris Zbarsky [:bzbarsky]]

So I did a bit more testing, and I can get this to leak if all
wrapToolbarItems() does is call |toolbar.insertBefore(item, item.nextSibling);|
on a single |item| (without even wrapping it).  The item needs to be the
<toolbaritem> that contains the <searchbar>.

Comment 4

[Boris Zbarsky [:bzbarsky]]

In particular, we leak if I just remove the searchbar from the toolbaritem. 
Again, in a minimal testcase I don't get that to happen... Further, if I comment
out both <field>s in the searchbar binding, I also stop leaking.

The JS GC output has things like:

08610360 object 0x86749b8 XULDocument via
nsXPCWrappedJS::mJSObj[nsIController,0x87a6258,0x8485df0](Object @
0x08485df0).mOuter(XULElement @ 0x08485a00).__parent__(XULElement @
0x084859d8).nsDOMClassInfo::sPreservedWrapperTable(XULElement @
0x08455ea8).__parent__(XULElement @ 0x08455e38).__parent__(XULElement @
0x08610380).__parent__(XULDocument @ 0x08610360).

Which would indicate that someone somewhere is holding a reference to that
JS-implemented nsIController in C++, and leaking the document through that.

The searchbar-textbox binding destructor has some silliness with not removing
the controller if it's a specific descendant of a toolbarpaletteitem.  If I
remove that check and always call removeController() in that destructor, the
leak goes away.  So it just sounds like someone is neglecting to tear down a
reference cycle when toolbarpaletteitems are involved.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, and I figured out why using appendChild instead of insertBefore fixed the
bug in comment 0.  If I appendChild, then when I increment the index I end up
skipping over a node... So instead of wrapping all nodes, I only wrap the 1st,
3rd, etc, but wrap them twice or more.  As it happens, the searchbar toolbaritem
is the 8th node in the toolbar, so it was getting skipped over and never got
removed from the DOM (which is the operation that leaks).

Comment 6

[:Gavin Sharp [email: [e-mail]]]

Attachment: Patch per discussion with bz and mconnor

Comment 7

[Asaf Romano (gone)]

Checking in search.xml;
/cvsroot/mozilla/browser/base/content/search.xml,v  <--  search.xml
new revision: 1.24; previous revision: 1.23
done

Comment 8

[Steve England [:stevee]]

Has this bug now been fixed?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Yes, I no longer see the leak I was seeing with Gavin's patch in the tree...

Comment 10

[Steve England [:stevee]]

--> Resolved FIXED as per comment 9

Comment 11

[Boris Zbarsky [:bzbarsky]]

OK, reopening.  This has reappeared again, not sure when.

Comment 12

[Asa Dotzler [:asa]]

after reconsidering the general user impact, this is no longer a blocker

Comment 13

[Steve England [:stevee]]

*** Bug 323433 has been marked as a duplicate of this bug. ***

Comment 14

[u88484]

Using leak-gauge.pl I get this with new profile, no extensions, default theme, about:blank as homepage so it was only thing loaded on startup. right clicked to customize, then closed the pallet and firefox.

Leaked inner window 1512028 (outer 1512130) at address 1512028.
 ... with URI "chrome://browser/content/browser.xul".
Leaked inner window 14561d8 (outer 140b498) at address 14561d8.
 ... with URI "about:blank".
Leaked outer window 140b498 at address 140b498.
Leaked outer window 1512130 at address 1512130.
Leaked document at address 1d8dfb8.
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/browser.j
ar!/content/browser/search.xml".
 ... with URI "chrome://browser/content/search.xml".
Leaked document at address 1e01b08.
 ... with URI "chrome://global/content/bindings/button.xml".
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/bindings/button.xml".
Leaked document at address 1d602e8.
 ... with URI "chrome://global/content/platformHTMLBindings.xml".
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/platformHTMLBindings.xml".
Leaked document at address 14b1610.
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/bindings/stringbundle.xml".
 ... with URI "chrome://global/content/bindings/stringbundle.xml".
Leaked document at address 1e38470.
 ... with URI "chrome://global/content/bindings/browser.xml".
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/bindings/browser.xml".
Leaked document at address 1cfe7d8.
 ... with URI "chrome://global/content/bindings/text.xml".
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/bindings/text.xml".
Leaked document at address 1eb8308.
 ... with URI "chrome://global/content/bindings/tree.xml".
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/bindings/tree.xml".
Leaked document at address 1e2c388.
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/bindings/tabbox.xml".
 ... with URI "chrome://global/content/bindings/tabbox.xml".
Leaked document at address 1dd3620.
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/bindings/splitter.xml".
 ... with URI "chrome://global/content/bindings/splitter.xml".
Leaked document at address 14c8c68.
 ... with URI "about:blank".
Leaked document at address 1e47f50.
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/bindings/scrollbox.xml".
 ... with URI "chrome://global/content/bindings/scrollbox.xml".
Leaked document at address 1ea0438.
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/bindings/general.xml".
 ... with URI "chrome://global/content/bindings/general.xml".
Leaked document at address 1d8e938.
 ... with URI "chrome://global/content/bindings/popup.xml".
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/bindings/popup.xml".
Leaked document at address 1dda7c0.
 ... with URI "chrome://global/content/bindings/tabbrowser.xml".
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/bindings/tabbrowser.xml".
Leaked document at address 1eb5068.
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/bindings/menu.xml".
 ... with URI "chrome://global/content/bindings/menu.xml".
Leaked document at address 14e0dc8.
 ... with URI "chrome://global/content/bindings/autocomplete.xml".
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/bindings/autocomplete.xml".
Leaked document at address 1e29538.
 ... with URI "chrome://global/content/bindings/toolbarbutton.xml".
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/bindings/toolbarbutton.xml".
Leaked document at address 1e3d948.
 ... with URI "chrome://global/content/bindings/progressmeter.xml".
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/bindings/progressmeter.xml".
Leaked document at address 1dfc360.
 ... with URI "chrome://global/content/bindings/textbox.xml".
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/bindings/textbox.xml".
Leaked document at address 1e73b10.
 ... with URI "chrome://global/content/bindings/toolbar.xml".
 ... with URI "jar:file:///C:/Program%20Files/Mozilla%20Firefox/chrome/toolkit.j
ar!/content/global/bindings/toolbar.xml".
Leaked document at address 15764f8.
Summary:
Leaked 4 out of 8 DOM Windows
Leaked 21 out of 30 documents
Leaked 0 out of 4 docshells

Comment 15

[Steve England [:stevee]]

*** Bug 325989 has been marked as a duplicate of this bug. ***

Comment 16

[:Gavin Sharp [email: [e-mail]]]

*** Bug 338468 has been marked as a duplicate of this bug. ***

Comment 17

[Steve England [:stevee]]

Attachment: More recent leak-log

Leaklog from Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a7pre) Gecko/2007071506 Minefield/3.0a7pre ID:2007071506

New profile, start firefox with logging enabled, right click on toolbar area, customize toolbars, done, close firefox, analyse log.

Summary:
Leaked 6 out of 9 DOM Windows
Leaked 27 out of 43 documents
Leaked 0 out of 4 docshells

which is the same number of DOM Windows and documents leaked as the recent bug 388297.

Comment 18

[Mike Connor [:mconnor]]

We'd really like to get a fix for this, but as it happens in a generally rare case not going to block at this time.

Comment 19

[Boris Zbarsky [:bzbarsky]]

fwiw, fixing this might be as simple as adding some cycle collection to controllers.  Assuming this still happens, of course.

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hmm.. all controllers should get unhooked from nsDocument::Destroy though, so they really shouldn't be causing cycles involving documents. No?

Comment 21

[Boris Zbarsky [:bzbarsky]]

See comment 4.

Comment 22

[Carsten Book [:Tomcat]]

Attachment: new leak log

leak log generated with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b3pre) Gecko/2008012310 Firefox/3.0b3pre

Comment 23

[Carsten Book [:Tomcat]]

not leaking for me anymore with the steps to reproduce from comment #0 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008030619
Minefield/3.0b5pre

Comment 24

[Jeff Walden [:Waldo]]

I can reproduce massively on OS X; debug build leaks 868851 bytes right-clicking on toolbar, hitting Done, and exiting.

Comment 25

[u278084]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031905 Minefield/3.0b5pre
Can still reproduce as well.

Comment 26

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

The fix in bug 426236 fixes all the leaks, but it doesn't touch controllers...

Comment 27

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Boris, does this bug still need qa attention (qawanted)?

Comment 28

[Boris Zbarsky [:bzbarsky]]

Probably not.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks, so removing the flag for now.

Comment 30

[Marco Castelluccio [:marco]]

Removing "mlk" as the memory leaks have disappeared.

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

If there aren't any leaks currently, then either this bug should be marked fixed or it should remain open for issues that can potentially cause leaks but currently aren't; either way, it is a leak bug.

Comment 32

[Nicholas Nethercote [inactive]]

> STEPS TO REPRODUCE:
> 1)  env XPCOM_MEM_LEAK_LOG=/tmp/log.txt firefox
> 2)  Right-click on "Home" button and choose the "Customize..." menu option
> 3)  Click "Done" in the resulting dialog
> 4)  Quit.

Setting XPCOM_MEM_LEAK_LOG doesn't seem to do anything. Does that feature still work? It appears to require NS_IMPL_REFCNT_LOGGING to be defined for the relevant code to execute, but I can't see anything that defines that constant.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It should work in either:
 * builds with --enable-debug
 * builds with --enable-logrefcnt

(It's part of the refcount balancer, along with XPCOM_MEM_REFCNT_LOG, XPCOM_MEM_COMPTR_LOG, etc.)

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Er, s/refcount balancer/nsTraceRefcnt/

Comment 35

[Nicholas Nethercote [inactive]]

The only output I got was this:

> == BloatView: ALL (cumulative) LEAK STATISTICS, default process 11459
> nsTraceRefcntImpl::DumpStatistics: 1006 entries

So I think this is fixed. Please reopen if I'm wrong.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Yeah.  Even if we ignore bug 426236, the customization UI is totally different now (e.g. it's an in-tab UI, not a dialog)...