Closed
Bug 732495
Opened 12 years ago
Closed 12 years ago
shutdown leaks on a fresh profile due to XBL cache
Categories
(Core :: XBL, defect)
Core
XBL
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: mccr8, Assigned: mccr8)
References
Details
(Whiteboard: [MemShrink:P2])
Attachments
(3 files, 6 obsolete files)
Steps to reproduce: 1. create a new profile (profile manager has a shutdown leak, see bug 732493) 2. start browser with new profile. exit. no shutdown leak. 3. start browser with new profile. exit. no shutdown leak. 4. start browser with new profile. exit. shutdown leak. Subsequent start and exits also have shutdown leaks. It looks like it leaks documents, but no nsGlobalWindow.
Assignee | ||
Comment 1•12 years ago
|
||
looks like the nsDocuments are being held alive by nsXBLDocumentInfo with missing edges, plus some marked JS.
Assignee | ||
Comment 2•12 years ago
|
||
The various marked JS are being held alive by the GC as follows: via B mScopeObject : 0x1185c5ba0 [B ChromeWindow 119e84be0] --[B BrowserOffline]-> 0x11a009f40 [B Object <no private>] --[B _uiElement]-> 0x11af1c130 [B XULElement 11a1ed4c0] (the B's indicate the object in question has been marked black by the GC)
Assignee | ||
Comment 3•12 years ago
|
||
The GC path is bogus. I have to fix my scripts to deal with the B and G stuff.
Assignee | ||
Comment 4•12 years ago
|
||
I tried this again, and the nsDocuments were only being held live by nsXBLDocumentInfo.
Attachment #602415 -
Attachment is obsolete: true
Assignee | ||
Comment 5•12 years ago
|
||
3/2 Aurora also exhibits the same behavior.
Assignee | ||
Comment 6•12 years ago
|
||
Setting nglayout.debug.disable_xul_cache to true stops the shutdown leak.
Component: General → XBL
QA Contact: general → xbl
Assignee | ||
Comment 7•12 years ago
|
||
With this patch applied, I no longer see the leak. All the patch does is set startupCache to false.
Assignee | ||
Comment 8•12 years ago
|
||
I guess it could be some cycle collector optimization not shutting down properly on shutdown or something like that.
Assignee | ||
Comment 9•12 years ago
|
||
3/3 nightly Beta Debug is also affected, which suggests it isn't anything from recent cycle collector optimizations.
Assignee | ||
Updated•12 years ago
|
Summary: shutdown leaks on a fresh profile → shutdown leaks on a fresh profile due to XBL cache
Updated•12 years ago
|
Assignee: nobody → continuation
Whiteboard: [MemShrink] → [MemShrink:P2]
Assignee | ||
Comment 10•12 years ago
|
||
After poking around with some ref count logging, I came up with this, which in some light testing seems to fix the leak. I think the problem is that LoadBindingDocumentInfo does a call with getter_AddRefs(info), so the callee does an addref on info, but then before returning, LoadBindingDocumentInfo also does an addref? This is a very hacky fix. I'm not sure what the idiomatic way to do this is.
Attachment #602481 -
Attachment is obsolete: true
Comment 11•12 years ago
|
||
Yes, the existing behaviour looks incorrect. It should be using |info.forget(*aResult)| instead of the NS_IF_ADDREF call.
(In reply to Andrew McCreight [:mccr8] from comment #10) > Created attachment 608027 [details] [diff] [review] > hacky fix > > After poking around with some ref count logging, I came up with this, which > in some light testing seems to fix the leak. I think the problem is that > LoadBindingDocumentInfo does a call with getter_AddRefs(info), so the callee > does an addref on info, but then before returning, LoadBindingDocumentInfo > also does an addref? Except that the refptr dtor will do a Release.
Comment 13•12 years ago
|
||
Whoops, info.forget(aResult).
Assignee | ||
Comment 14•12 years ago
|
||
(In reply to Josh Matthews [:jdm] from comment #13) > Whoops, info.forget(aResult). That still leaks (though possibly in a different way). (In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #12) > Except that the refptr dtor will do a Release. Yeah, that's true. I guess for the other places info is assigned to in the normal way the underlying object will also get addrefed? With something like this: info = xblDocBindingManager->GetXBLDocumentInfo(documentURI) So that info is getting the same number of addrefs along all paths in the function? Maybe the problem isn't in nsXBLService, but maybe ReadPrototypeBindings or one of the PutXBLDocumentInfo calls is somehow addreffing info an extra time. Most likely ReadPrototypeBindings, as this is the only place it is called.
Can you post a refcount log for the relevant object?
Assignee | ||
Comment 16•12 years ago
|
||
This is the ref count log for nsXBLDocumentInfo with id 1 (chosen arbitrarily). 61k compressed, 20 megs uncompressed.
Assignee | ||
Comment 17•12 years ago
|
||
Big thanks to jdm, who found the extra addref and suggested this fix.
Attachment #608027 -
Attachment is obsolete: true
Assignee | ||
Comment 18•12 years ago
|
||
The NS_NewXBLDocumentInfo function that manually addrefs an object seems a little weird to me. There are only two callers, so it may make more sense to just inline it, which lets us use nsRefPtrs and avoid manual ref counting. This is on top of the previous patch. (The weird whitespace changes in the previous patch remove trailing spaces.)
Assignee | ||
Updated•12 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 19•12 years ago
|
||
Attachment #608107 -
Attachment is obsolete: true
Attachment #608109 -
Attachment is obsolete: true
Assignee | ||
Comment 20•12 years ago
|
||
Attachment #608228 -
Attachment is obsolete: true
Assignee | ||
Comment 21•12 years ago
|
||
Try run looked fine https://tbpl.mozilla.org/?tree=Try&rev=76026331498f
Assignee | ||
Comment 22•12 years ago
|
||
I guess that nsXBLDocumentInfo probably survives until shutdown anyways, so this isn't really worth landing on older branches.
status-firefox-esr10:
--- → wontfix
status-firefox11:
--- → wontfix
status-firefox12:
--- → wontfix
status-firefox13:
--- → wontfix
status-firefox14:
--- → affected
Assignee | ||
Updated•12 years ago
|
Attachment #608360 -
Flags: review?(bugs)
Comment 23•12 years ago
|
||
Comment on attachment 608360 [details] [diff] [review] clean up refcounting for nsXBLDocumentInfo creation Much better
Attachment #608360 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 24•12 years ago
|
||
Thanks for the fast review! https://hg.mozilla.org/integration/mozilla-inbound/rev/733c0022cbc6
Target Milestone: --- → mozilla14
Assignee | ||
Comment 25•12 years ago
|
||
(In reply to Olli Pettay [:smaug] from comment #23) > Much better I kind of want to go through and clean up a bunch of this old code that manually addrefs and decrefs in weird places, but that's probably not a good use of my time.
Comment 26•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/733c0022cbc6
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
status-firefox14:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•