426236 - Customize toolbar dialog leaks multiple dom windows and documents

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

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

Attachment: 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.

Comment 1

[Seth Spitzer]

Comment on attachment 312769 [details] [diff] [review]
Patch

r=sspitzer

two comments:

1)  bent rules
2)  see comment #1

Comment 2

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

BTW, steps to reproduce:

1. Right click the toolbar, select 'Customize'.
2. Do whatever, dismiss the dialog.
3. Close the browser.

Comment 3

[Seth Spitzer]

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

Comment 4

[Seth Spitzer]

keeping peterv in the loop (who helped me with the original places cycle collector monkey junk), and mano (killer of places leaks).

Comment 5

[Dietrich Ayala (:dietrich)]

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.

Comment 6

[Jesse Ruderman]

Does this fix bug 296474?

Comment 7

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

(In reply to comment #6)

I don't leak anything anymore with this patch.

Comment 8

[Carsten Book [:Tomcat]]

(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 9

[Damon Sicore (:damons)]

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 10

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

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 11

[Damon Sicore (:damons)]

Comment on attachment 312769 [details] [diff] [review]
Patch

Thanks for the answer.

a1.9+=damons, fixes a leak, low risk.

Comment 12

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

Fixed on trunk.

Comment 13

[Jeff Walden [:Waldo]]

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.

Comment 14

[Carsten Book [:Tomcat]]

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)

Comment 15

[Gervase Markham [:gerv]]

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