Created attachment 312769 [details] [diff] [review] Patch Here's the basic problem: nsCycleCollector: nsXPCWrappedJS (nsINavHistoryResultViewer) 0x1ca89458 was not collected due to 1 external references (2 total - 1 known) An object expected to be garbage could be reached from it by the path: nsXPCWrappedJS (nsINavHistoryResultViewer) 0x1ca89458 via <unknown edge> JS Object (Object) (global=1bdf55a0) 0x1c9bd5a0 via __parent__ JS Object (ChromeWindow) (global=1bdf55a0) 0x1bdf55a0 via <unknown edge> nsGlobalWindow 0x1be8c0ac The 1 known references to it were from: nsXPCWrappedJS (nsINavHistoryResultViewer) 0x1ca89458 Somehow the places xbl bindings (toolbar.xml, tree.xml, menu.xml) are failing to release a result->viewer->result cycle properly and a bunch of stuff lives on because of it. I poked around a bit and wasn't able to find the leak as quickly as I wanted, so I propose that we cheat and let the CC handle this case (since that's what it's made for, anyway). All that we have to do is tell the CC that the viewer is an edge that needs to be handled. See the patch.
Attachment #312769 - Flags: review?(dietrich)
Comment on attachment 312769 [details] [diff] [review] Patch r=sspitzer two comments: 1) bent rules 2) see comment #1
Attachment #312769 - Flags: review+
BTW, steps to reproduce: 1. Right click the toolbar, select 'Customize'. 2. Do whatever, dismiss the dialog. 3. Close the browser.
Assignee: nobody → bent.mozilla
cc'ing tomcat, in case he has an automated or manual list of leak tests.
keeping peterv in the loop (who helped me with the original places cycle collector monkey junk), and mano (killer of places leaks).
Comment on attachment 312769 [details] [diff] [review] Patch r=me, thanks mucho. however, i'll cc mano as he might have more insight into when/where we should properly break the cycle.
Attachment #312769 - Flags: review?(dietrich) → review+
Does this fix bug 296474?
(In reply to comment #6) I don't leak anything anymore with this patch.
(In reply to comment #3) > cc'ing tomcat, in case he has an automated or manual list of leak tests. > will do some leak tests around this patch when its landed on trunk and will verify this bug then :-)
Comment on attachment 312769 [details] [diff] [review] Patch What's the regression risk here? While this is a good leak fix, need to know what the risk is. re-request approval once answered. Good job, bent.
Comment on attachment 312769 [details] [diff] [review] Patch I don't think there is really any regression risk here. Code in js land already tries to break this cycle manually so the case where the result's view is null is definitely expected.
Comment on attachment 312769 [details] [diff] [review] Patch Thanks for the answer. a1.9+=damons, fixes a leak, low risk.
Attachment #312769 - Flags: approval1.9? → approval1.9+
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3
A browser-chrome or chrome Mochitest could test customizing toolbars at the surface level and verify the absence of this leak, once leaks become fatal when running Mochitests.
verified fixed using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008041521 Minefield/3.0pre (Leak Debug Build)
Status: RESOLVED → VERIFIED
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.