Customize toolbar dialog leaks multiple dom windows and documents

VERIFIED FIXED in Firefox 3

Status

()

Firefox
Bookmarks & History
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Ben Turner (not reading bugmail, use the needinfo flag!), Assigned: Ben Turner (not reading bugmail, use the needinfo flag!))

Tracking

({memory-leak})

Trunk
Firefox 3
memory-leak
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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)
Attachment #312769 - Flags: review?(seth)

Comment 1

10 years ago
Comment on attachment 312769 [details] [diff] [review]
Patch

r=sspitzer

two comments:

1)  bent rules
2)  see comment #1
Attachment #312769 - Flags: review+

Updated

10 years ago
Attachment #312769 - Flags: review?(seth)
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

Comment 3

10 years ago
cc'ing tomcat, in case he has an automated or manual list of leak tests.

Comment 4

10 years ago
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+

Comment 6

10 years ago
Does this fix bug 296474?
Keywords: mlk
(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.
Attachment #312769 - Flags: approval1.9?
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.
Attachment #312769 - Flags: approval1.9?
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+
Keywords: checkin-needed
Fixed on trunk.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3

Comment 13

10 years ago
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.
Flags: in-testsuite?
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.