Closed
Bug 296474
Opened 19 years ago
Closed 11 years ago
Memory leak when customizing toolbars
Categories
(Firefox :: Toolbars and Customization, defect)
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: bzbarsky, Unassigned)
References
Details
(Keywords: memory-leak, Whiteboard: [MemShrink:P3])
Attachments
(2 files, 1 obsolete file)
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•19 years ago
|
||
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).
Reporter | ||
Comment 2•19 years ago
|
||
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...
Flags: blocking-aviary1.1?
Keywords: qawanted
Reporter | ||
Comment 3•19 years ago
|
||
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>.
Reporter | ||
Comment 4•19 years ago
|
||
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.
Reporter | ||
Comment 5•19 years ago
|
||
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•19 years ago
|
||
Updated•19 years ago
|
Assignee: gavin.sharp → nobody
Status: ASSIGNED → NEW
Updated•19 years ago
|
Attachment #188508 -
Flags: review?(mconnor) → review+
Updated•19 years ago
|
Attachment #188508 -
Flags: approval-aviary1.1a2?
Updated•19 years ago
|
Attachment #188508 -
Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
Updated•19 years ago
|
Whiteboard: [checkin needed][a+]
Comment 7•19 years ago
|
||
Checking in search.xml;
/cvsroot/mozilla/browser/base/content/search.xml,v <-- search.xml
new revision: 1.24; previous revision: 1.23
done
Whiteboard: [checkin needed][a+]
Comment 8•19 years ago
|
||
Has this bug now been fixed?
Reporter | ||
Comment 9•19 years ago
|
||
Yes, I no longer see the leak I was seeing with Gavin's patch in the tree...
Comment 10•19 years ago
|
||
--> Resolved FIXED as per comment 9
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•19 years ago
|
Flags: blocking-aviary1.1?
Reporter | ||
Comment 11•19 years ago
|
||
OK, reopening. This has reappeared again, not sure when.
Status: RESOLVED → REOPENED
Flags: blocking-aviary1.5?
Resolution: FIXED → ---
Updated•19 years ago
|
Flags: blocking-aviary1.5? → blocking1.8b4?
Updated•19 years ago
|
Flags: blocking1.8b4? → blocking1.8b4+
Comment 12•19 years ago
|
||
after reconsidering the general user impact, this is no longer a blocker
Flags: blocking1.8b4+ → blocking1.8b4-
Comment 13•19 years ago
|
||
*** Bug 323433 has been marked as a duplicate of this bug. ***
Comment 14•19 years ago
|
||
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
OS: Linux → All
Comment 15•19 years ago
|
||
*** Bug 325989 has been marked as a duplicate of this bug. ***
Updated•18 years ago
|
Comment 16•18 years ago
|
||
*** Bug 338468 has been marked as a duplicate of this bug. ***
Updated•18 years ago
|
Attachment #188508 -
Attachment is obsolete: true
Comment 17•17 years ago
|
||
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.
Flags: blocking-firefox3?
Comment 18•17 years ago
|
||
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.
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Reporter | ||
Comment 19•17 years ago
|
||
fwiw, fixing this might be as simple as adding some cycle collection to controllers. Assuming this still happens, of course.
Hmm.. all controllers should get unhooked from nsDocument::Destroy though, so they really shouldn't be causing cycles involving documents. No?
Reporter | ||
Comment 21•17 years ago
|
||
See comment 4.
Comment 22•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
I can reproduce massively on OS X; debug build leaks 868851 bytes right-clicking on toolbar, hitting Done, and exiting.
Comment 25•17 years ago
|
||
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031905 Minefield/3.0b5pre
Can still reproduce as well.
Depends on: 426236
The fix in bug 426236 fixes all the leaks, but it doesn't touch controllers...
Comment 27•15 years ago
|
||
Boris, does this bug still need qa attention (qawanted)?
Reporter | ||
Comment 28•15 years ago
|
||
Probably not.
Comment 31•13 years ago
|
||
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.
Keywords: mlk
Updated•13 years ago
|
Whiteboard: [MemShrink]
Updated•13 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P3]
Comment 32•11 years ago
|
||
> 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•11 years ago
|
||
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•11 years ago
|
||
Er, s/refcount balancer/nsTraceRefcnt/
Comment 35•11 years ago
|
||
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.
Status: REOPENED → RESOLVED
Closed: 19 years ago → 11 years ago
Resolution: --- → WORKSFORME
Reporter | ||
Comment 36•11 years ago
|
||
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)...
You need to log in
before you can comment on or make changes to this bug.
Description
•