381239 - Places Leaks

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Robert Sayre]

Probably just missing some cycle collector macros, but Rlk is up to 800k or so again.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is there a reason to believe cycles are the problem?

Comment 2

[Robert Sayre]

(In reply to comment #1)
> Is there a reason to believe cycles are the problem?
> 

Only that the interfaces are crawling with cyclic references (parent->child, child->parent) and there is lots of JS involved. But I suppose time is better spent measuring!  

Comment 3

[Robert Sayre]

Attachment: leak log

Comment 4

[Robert Sayre]

Attachment: nsNavHistoryQuery.balance

Comment 5

[Robert Sayre]

Attachment: leaking queries through resultnodes

Comment 6

[Robert Sayre]

Attachment: nsNavHistoryContainerResultNode.balance

Comment 7

[Robert Sayre]

Comment on attachment 265359 [details]
nsNavHistoryContainerResultNode.balance

this turned out to be leaking from nsNavHistoryResultNode::FillChildren

Comment 8

[Asaf Romano (gone)]

Attachment: collect the cycle on nsNavHistoryQueryResultNode::mQueries

Comment 9

[Robert Sayre]

Attachment: nsNavHistoryFolderResultNode.balance

This shows addrefs in XBL bindings, such as toolbar.xml.

It looks like we're incrementing it by 3, but JS_GC is only collecting one of them.

Comment 10

[Asaf Romano (gone)]

Attachment: break cycles in the xbl views

Comment 11

[Asaf Romano (gone)]

Attachment: break cycles in the xbl views

Comment 12

[Asaf Romano (gone)]

mozilla/browser/components/places/content/menu.xml 1.69
mozilla/browser/components/places/content/toolbar.xml 1.82
mozilla/browser/components/places/content/tree.xml 1.65

Comment 13

[Asaf Romano (gone)]

Attachment: s/view/viewer/

oops.

Also unset the viewer in the toolbar binding, and null out resultNode.

Comment 14

[Asaf Romano (gone)]

mozilla/browser/components/places/content/menu.xml 1.70
mozilla/browser/components/places/content/toolbar.xml 1.83
mozilla/browser/components/places/content/tree.xml 1.67

Comment 15

[Asaf Romano (gone)]

With the last patch, Rlk went down to 5.34k on fxdebug-linux.

Comment 16

[Robert Sayre]

Visiting 1 page and quitting the browser gives numbers that are consistent with tests I conducted prior to places. Here is opening and closing the browser vs. opening the browser and then navigating to yahoo.com.


--------------------------------------------------------------------------------------
Current file:  2doc.txt
Previous file: 1doc.txt
----------------------------------------------leaks------leaks%------bloat------bloat%
TOTAL                                         11264       0.07%   27756500     177.46%
--NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
nsStringBuffer                                  128       6.67%
TOTAL                                           128
--NEW-BLOAT-----------------------------------bloat------bloat%-----------------------
nsRunnable                                    17736     291.01%
nsVoidArray                                  193680     221.04%
nsTArray_base                                 57912     165.16%
XPCWrappedNative                             557760     106.64%
nsHashKey                                    324112      69.20%
nsUnicodeToUTF8                                 160      66.67%
nsStringBuffer                               225408      38.84%
nsCStringKey                                  73152      32.37%
nsXPCWrappedJS                                33072      27.20%
XPCWrappedNativeProto                         28672      19.63%
XPCNativeScriptableShared                    222696       9.33%
nsXPCWrappedJSClass                            2880       8.11%
nsXPCComponents                                9120       4.11%
xptiInterfaceInfo                             55440       3.59%
nsHashtable                                   26280       3.40%
nsLocalFile                                  466336       2.47%
nsJSID                                        23520       2.19%
TOTAL                                       2317936
--ALL-LEAKS-----------------------------------leaks------leaks%-----------------------
TOTAL                                         11264       0.07%
XPCWrappedNative                               3584       0.00%
nsJSContext                                    2080       0.00%
XPCWrappedNativeProto                          1400       0.00%
nsXBLDocGlobalObject                           1120       0.00%
XPCNativeScriptableShared                       432       0.00%
nsXPCWrappedJS                                  416       0.00%
nsCStringKey                                    224       0.00%
nsPrefBranch                                    224       0.00%
nsLocalFile                                     208       0.00%
nsScriptSecurityManager                         168       0.00%
nsHashtable                                     144       0.00%
nsThread                                        144       0.00%
nsStringBuffer                                  128       6.67%
nsXPCComponents                                 120       0.00%
nsHashKey                                       112       0.00%
nsBaseAppShell                                   80       0.00%
nsCollationUnix                                  80       0.00%
nsSystemPrincipal                                72       0.00%
nsXPCWrappedJSClass                              72       0.00%
nsGTKToolkit                                     64       0.00%
nsJSID                                           56       0.00%
nsJSRuntimeServiceImpl                           56       0.00%
BackstagePass                                    48       0.00%
xptiInterfaceInfo                                40       0.00%
nsXPCThreadJSContextStackImpl                    40       0.00%
nsUnicodeToUTF8                                  32       0.00%
nsSecretDecoderRing                              32       0.00%
nsJSRuntime                                      24       0.00%
nsRunnable                                       24       0.00%
nsCollation                                      16       0.00%
nsVoidArray                                      16       0.00%
nsTArray_base                                     8       0.00%
--ALL-BLOAT-----------------------------------bloat------bloat%-----------------------
TOTAL                                      27756500     177.46%
XPCWrappedNative                             557760     106.64%
nsLocalFile                                  466336       2.47%
nsHashKey                                    324112      69.20%
nsStringBuffer                               225408      38.84%
XPCNativeScriptableShared                    222696       9.33%
nsVoidArray                                  193680     221.04%
nsCStringKey                                  73152      32.37%
nsTArray_base                                 57912     165.16%
xptiInterfaceInfo                             55440       3.59%
nsXPCWrappedJS                                33072      27.20%
XPCWrappedNativeProto                         28672      19.63%
nsHashtable                                   26280       3.40%
nsJSID                                        23520       2.19%
nsRunnable                                    17736     291.01%
nsXPCComponents                                9120       4.11%
nsJSContext                                    3120       0.00%
nsPrefBranch                                   2912       0.00%
nsXPCWrappedJSClass                            2880       8.11%
nsXBLDocGlobalObject                           1176       0.00%
nsThread                                        864       0.00%
nsScriptSecurityManager                         168       0.00%
nsUnicodeToUTF8                                 160      66.67%
nsCollationUnix                                 160       0.00%
nsBaseAppShell                                   80       0.00%
nsSystemPrincipal                                72       0.00%
nsGTKToolkit                                     64       0.00%
nsJSRuntimeServiceImpl                           56       0.00%
BackstagePass                                    48       0.00%
nsXPCThreadJSContextStackImpl                    40       0.00%
nsCollation                                      32       0.00%
nsSecretDecoderRing                              32       0.00%
nsJSRuntime                                      24       0.00%
--------------------------------------------------------------------------------------

Comment 17

[Robert Sayre]

Attachment: visit a couple pages, then the tinderbox page (bunch of iframes) vs. visiting two pages

Comment 18

[Robert Sayre]

Attachment: leaking prefs from event handlers

Comment 19

[Robert Sayre]

Attachment: null out all members of PrefHandler

Comment 20

[Dietrich Ayala (:dietrich)]

Comment on attachment 265402 [details] [diff] [review]
null out all members of PrefHandler

r=me, thanks.

Comment 21

[Asaf Romano (gone)]

While this fix doesn't hurt, OptionsFilter is only used in the organizer window. I doubt the later is opened during the Rlk test.

Comment 22

[Robert Sayre]

Attachment: another leak delta

hmm, doing that definitely changed the traces I was seeing, but maybe just a coincidence. Still leaking the prefbranch, but it seems like that is caused by something leaking caps.

Here's another delta, showing us leaking XBL documents.

Comment 23

[Robert Sayre]

(In reply to comment #22)
>
> Here's another delta, showing us leaking XBL documents.

nsXBLDocGlobalObjects, to be more precise. Blockingn on bug 375063.

Comment 24

[(not reading, please use [e-mail] instead)]

Comment on attachment 265370 [details] [diff] [review]
break cycles in the xbl views

r=sspitzer, noting that some of these changes were fixed in follow up patches.

Comment 25

[(not reading, please use [e-mail] instead)]

Comment on attachment 265376 [details] [diff] [review]
s/view/viewer/

r=sspitzer

some questions:

1)  was the change to null out this._resultNode in the menu's destructor to break a particular cycle?  (from looking at menu.xul this._resultNode is just this._result.root).  I'm not seeing any cycles in the menu ownership model.  Am I missing something?

2)  in toolbar.xml, by seting you're breaking the toolbar -> _result.viewer -> _viewer -> toolbar cycle.  can you add a comment to the destructor to indicate the ownership model, like you did in toolbar.xml?

3)  changes to tree.xml destructor have already landed as part of another patch.

Comment 26

[(not reading, please use [e-mail] instead)]

Comment on attachment 265363 [details] [diff] [review]
collect the cycle on nsNavHistoryQueryResultNode::mQueries

Can you explain why you use the Traverse and Unlink methods for mQueries?  

I don't see any potential cycles from a nsNavHistoryQuery back to a nsNavHistoryQueryResultNode.

Also, are we sure that we need the cycle collector here?

perhaps we can get some review help from peterv (or graydon)?

Comment 27

[Peter Van der Beken [:peterv]]

Comment on attachment 265363 [details] [diff] [review]
collect the cycle on nsNavHistoryQueryResultNode::mQueries

mQueries is a nsCOMArray<nsNavHistoryQuery>. This patch does nothing until you make nsNavHistoryQuery participate in cycle collection, but I don't see how nsNavHistoryQuery can create cycles.

Comment 28

[Asaf Romano (gone)]

I've backed out this patch:
mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp 1.100
mozilla/toolkit/components/places/src/nsNavHistoryResult.h 1.40

Comment 29

[Dietrich Ayala (:dietrich)]

is there anything left to do here?

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Attachment: RLk Excel Plot

OK, I'm pretty sure now that the random 84 byte leak on the RLk test is due to Places Bookmarks being turned on. The attachment is an Excel spreadsheet plotting the raw RLk data over that time frame. You can see on the plot that prior to Places Bookmarks being turned on, it's nice and flat. Next, it skyrockets after they're turned on. Finally, Mano checked in a fix for that and after that, we can see the leak now apparent from there out.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Oh, and for the record, here are the leaks:

--NEW-LEAKS-----------------------------------leaks
XPCWrappedNativeProto                           756
XPCWrappedNative                               1680
TOTAL                                          2436

Comment 32

[Mike Connor [:mconnor]]

moving to b1, we'll need to look at Ryan's comments.

Comment 33

[Myk Melez [:myk] [@mykmelez]]

Over in bug 385237, I figured out that the cause of a content pref service leak is the nsIFactory that creates the service.  Some testing leads me to conclude that the factories in every JS component are likely to be leaking, which may be the cause of (some of) the leaks reported in the bugs to which I am adding this comment (bug 337050, bug 378618, bug 381239, and bug 380873/bug 380931).

Take a look at bug 385237, comment 2 for more details, and note that the fix for bug 180380 makes all the XPCWrappedNative and XPCWrappedNativeProto leaks (which were what the content pref service was leaking) go away and may similarly fix (some of) the leaks reported in this bug.

Comment 34

[(not reading, please use [e-mail] instead)]

I don't think there is anything left to do here due to bug #180380, as myk points out.

see bug #387203 comment #16 (the "before" part, which is the current state, not the "after" part.)  

But note, for bug #387203, I need to figure out the leaks that patch causes, othewise places will leak for sure.

Comment 35

[Robert Sayre]

I agree, we got this down to one XPCWrappedNative, which was solved by the XPConnect patch. Marking fixed.

Comment 36

[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