Closed
Bug 381239
Opened 18 years ago
Closed 18 years ago
Places Leaks
Categories
(Firefox :: Bookmarks & History, defect)
Tracking
()
RESOLVED
FIXED
Firefox 3 alpha8
People
(Reporter: sayrer, Unassigned)
References
Details
(Keywords: memory-leak)
Attachments
(13 files, 1 obsolete file)
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
peterv
:
review-
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
moco
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
moco
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
dietrich
:
review+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
application/vnd.ms-excel
|
Details |
Probably just missing some cycle collector macros, but Rlk is up to 800k or so again.
Comment 1•18 years ago
|
||
Is there a reason to believe cycles are the problem?
Reporter | ||
Comment 2•18 years ago
|
||
(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!
Reporter | ||
Comment 3•18 years ago
|
||
Reporter | ||
Comment 4•18 years ago
|
||
Reporter | ||
Comment 5•18 years ago
|
||
Reporter | ||
Comment 6•18 years ago
|
||
Reporter | ||
Comment 7•18 years ago
|
||
Comment on attachment 265359 [details]
nsNavHistoryContainerResultNode.balance
this turned out to be leaking from nsNavHistoryResultNode::FillChildren
Comment 8•18 years ago
|
||
Reporter | ||
Comment 9•18 years ago
|
||
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•18 years ago
|
||
Comment 11•18 years ago
|
||
Attachment #265369 -
Attachment is obsolete: true
Comment 12•18 years ago
|
||
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•18 years ago
|
||
oops.
Also unset the viewer in the toolbar binding, and null out resultNode.
Comment 14•18 years ago
|
||
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•18 years ago
|
||
With the last patch, Rlk went down to 5.34k on fxdebug-linux.
Updated•18 years ago
|
Attachment #265376 -
Flags: review?(sspitzer)
Updated•18 years ago
|
Attachment #265370 -
Flags: review?(sspitzer)
Updated•18 years ago
|
Attachment #265363 -
Flags: review?(sspitzer)
Updated•18 years ago
|
Attachment #265376 -
Flags: review?(sspitzer)
Updated•18 years ago
|
Attachment #265370 -
Flags: review?(sspitzer)
Updated•18 years ago
|
Attachment #265363 -
Flags: review?(sspitzer)
Reporter | ||
Comment 16•18 years ago
|
||
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%
--------------------------------------------------------------------------------------
Reporter | ||
Comment 17•18 years ago
|
||
Updated•18 years ago
|
Attachment #265370 -
Flags: review?(sspitzer)
Updated•18 years ago
|
Attachment #265376 -
Flags: review?(sspitzer)
Updated•18 years ago
|
Attachment #265363 -
Flags: review?(sspitzer)
Reporter | ||
Comment 18•18 years ago
|
||
Reporter | ||
Comment 19•18 years ago
|
||
Comment 20•18 years ago
|
||
Comment on attachment 265402 [details] [diff] [review]
null out all members of PrefHandler
r=me, thanks.
Attachment #265402 -
Flags: review+
Comment 21•18 years ago
|
||
While this fix doesn't hurt, OptionsFilter is only used in the organizer window. I doubt the later is opened during the Rlk test.
Reporter | ||
Comment 22•18 years ago
|
||
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.
Reporter | ||
Comment 23•18 years ago
|
||
(In reply to comment #22)
>
> Here's another delta, showing us leaking XBL documents.
nsXBLDocGlobalObjects, to be more precise. Blockingn on bug 375063.
Depends on: 375063
Comment 24•18 years ago
|
||
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.
Attachment #265370 -
Flags: review?(sspitzer) → review+
Comment 25•18 years ago
|
||
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.
Attachment #265376 -
Flags: review?(sspitzer) → review+
Comment 26•18 years ago
|
||
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)?
Attachment #265363 -
Flags: review?(sspitzer) → review?(peterv)
Comment 27•18 years ago
|
||
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.
Attachment #265363 -
Flags: review?(peterv) → review-
Comment 28•18 years ago
|
||
I've backed out this patch:
mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp 1.100
mozilla/toolkit/components/places/src/nsNavHistoryResult.h 1.40
Updated•18 years ago
|
Flags: blocking-firefox3?
Updated•18 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Comment 29•18 years ago
|
||
is there anything left to do here?
Updated•18 years ago
|
Target Milestone: --- → Firefox 3 alpha6
Comment 30•18 years ago
|
||
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•18 years ago
|
||
Oh, and for the record, here are the leaks:
--NEW-LEAKS-----------------------------------leaks
XPCWrappedNativeProto 756
XPCWrappedNative 1680
TOTAL 2436
Comment 32•18 years ago
|
||
moving to b1, we'll need to look at Ryan's comments.
Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1
Comment 33•18 years ago
|
||
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.
Updated•18 years ago
|
Target Milestone: Firefox 3 M7 → Firefox 3 M8
Comment 34•18 years ago
|
||
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.
Reporter | ||
Comment 35•18 years ago
|
||
I agree, we got this down to one XPCWrappedNative, which was solved by the XPConnect patch. Marking fixed.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Comment 36•15 years ago
|
||
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.
Description
•