Closed Bug 381239 Opened 18 years ago Closed 18 years ago

Places Leaks

Categories

(Firefox :: Bookmarks & History, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3 alpha8

People

(Reporter: sayrer, Unassigned)

References

Details

(Keywords: memory-leak)

Attachments

(13 files, 1 obsolete file)

Probably just missing some cycle collector macros, but Rlk is up to 800k or so again.
Is there a reason to believe cycles are the problem?
Keywords: mlk
(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!
Attached file leak log (deleted) —
Attached file nsNavHistoryQuery.balance (deleted) —
Attached file leaking queries through resultnodes (deleted) —
Comment on attachment 265359 [details] nsNavHistoryContainerResultNode.balance this turned out to be leaking from nsNavHistoryResultNode::FillChildren
Attached file nsNavHistoryFolderResultNode.balance (deleted) —
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.
Attached patch break cycles in the xbl views (obsolete) (deleted) — Splinter Review
Attached patch break cycles in the xbl views (deleted) — Splinter Review
Attachment #265369 - Attachment is obsolete: true
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
Attached patch s/view/viewer/ (deleted) — Splinter Review
oops. Also unset the viewer in the toolbar binding, and null out resultNode.
Blocks: 381237
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
With the last patch, Rlk went down to 5.34k on fxdebug-linux.
Attachment #265376 - Flags: review?(sspitzer)
Attachment #265370 - Flags: review?(sspitzer)
Attachment #265363 - Flags: review?(sspitzer)
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% --------------------------------------------------------------------------------------
Attachment #265370 - Flags: review?(sspitzer)
Attachment #265376 - Flags: review?(sspitzer)
Attachment #265363 - Flags: review?(sspitzer)
Attached file leaking prefs from event handlers (deleted) —
Comment on attachment 265402 [details] [diff] [review] null out all members of PrefHandler r=me, thanks.
Attachment #265402 - Flags: review+
While this fix doesn't hurt, OptionsFilter is only used in the organizer window. I doubt the later is opened during the Rlk test.
Attached file another leak delta (deleted) —
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.
(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 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 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 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 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-
I've backed out this patch: mozilla/toolkit/components/places/src/nsNavHistoryResult.cpp 1.100 mozilla/toolkit/components/places/src/nsNavHistoryResult.h 1.40
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
is there anything left to do here?
Target Milestone: --- → Firefox 3 alpha6
Attached file RLk Excel Plot (deleted) —
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.
Oh, and for the record, here are the leaks: --NEW-LEAKS-----------------------------------leaks XPCWrappedNativeProto 756 XPCWrappedNative 1680 TOTAL 2436
moving to b1, we'll need to look at Ryan's comments.
Target Milestone: Firefox 3 alpha6 → Firefox 3 beta1
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.
Target Milestone: Firefox 3 M7 → Firefox 3 M8
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.
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: