Closed Bug 296474 Opened 19 years ago Closed 11 years ago

Memory leak when customizing toolbars

Categories

(Firefox :: Toolbars and Customization, defect)

x86
All
defect
Not set
major

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....
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).
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
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>.
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.
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).
Attached patch Patch per discussion with bz and mconnor (obsolete) (deleted) — Splinter Review
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #188508 - Flags: review?(mconnor)
Assignee: gavin.sharp → nobody
Status: ASSIGNED → NEW
Attachment #188508 - Flags: review?(mconnor) → review+
Attachment #188508 - Flags: approval-aviary1.1a2?
Attachment #188508 - Flags: approval-aviary1.1a2? → approval-aviary1.1a2+
Whiteboard: [checkin needed][a+]
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+]
Has this bug now been fixed?
Yes, I no longer see the leak I was seeing with Gavin's patch in the tree...
--> Resolved FIXED as per comment 9
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Flags: blocking-aviary1.1?
Depends on: 301283
OK, reopening. This has reappeared again, not sure when.
Status: RESOLVED → REOPENED
Flags: blocking-aviary1.5?
Resolution: FIXED → ---
Flags: blocking-aviary1.5? → blocking1.8b4?
Flags: blocking1.8b4? → blocking1.8b4+
after reconsidering the general user impact, this is no longer a blocker
Flags: blocking1.8b4+ → blocking1.8b4-
*** Bug 323433 has been marked as a duplicate of this bug. ***
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
*** Bug 325989 has been marked as a duplicate of this bug. ***
*** Bug 338468 has been marked as a duplicate of this bug. ***
Attachment #188508 - Attachment is obsolete: true
Attached file More recent leak-log (deleted) —
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.
Blocks: 402335
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-
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?
Attached file new leak log (deleted) —
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
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
I can reproduce massively on OS X; debug build leaks 868851 bytes right-clicking on toolbar, hitting Done, and exiting.
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre) Gecko/2008031905 Minefield/3.0b5pre Can still reproduce as well.
The fix in bug 426236 fixes all the leaks, but it doesn't touch controllers...
Boris, does this bug still need qa attention (qawanted)?
Probably not.
Thanks, so removing the flag for now.
Keywords: qawanted
Removing "mlk" as the memory leaks have disappeared.
Keywords: mlk
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
Whiteboard: [MemShrink]
Whiteboard: [MemShrink] → [MemShrink:P3]
> 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.
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.)
Er, s/refcount balancer/nsTraceRefcnt/
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 ago11 years ago
Resolution: --- → WORKSFORME
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.

Attachment

General

Created:
Updated:
Size: