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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14
Tracking Status
firefox11 --- wontfix
firefox12 --- wontfix
firefox13 --- wontfix
firefox-esr10 --- wontfix

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.
Attached file why nsDocuments are being held alive (obsolete) —
looks like the nsDocuments are being held alive by nsXBLDocumentInfo with missing edges, plus some marked JS.
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)
The GC path is bogus. I have to fix my scripts to deal with the B and G stuff.
I tried this again, and the nsDocuments were only being held live by nsXBLDocumentInfo.
Attachment #602415 - Attachment is obsolete: true
3/2 Aurora also exhibits the same behavior.
Setting nglayout.debug.disable_xul_cache to true stops the shutdown leak.
Component: General → XBL
QA Contact: general → xbl
Attached patch disable startup cache (obsolete) — Splinter Review
With this patch applied, I no longer see the leak.  All the patch does is set startupCache to false.
Blocks: 94199
I guess it could be some cycle collector optimization not shutting down properly on shutdown or something like that.
3/3 nightly Beta Debug is also affected, which suggests it isn't anything from recent cycle collector optimizations.
Summary: shutdown leaks on a fresh profile → shutdown leaks on a fresh profile due to XBL cache
Assignee: nobody → continuation
Whiteboard: [MemShrink] → [MemShrink:P2]
Attached patch hacky fix (obsolete) — Splinter Review
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
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.
Whoops, info.forget(aResult).
(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?
Attached file ref count log
This is the ref count log for nsXBLDocumentInfo with id 1 (chosen arbitrarily).  61k compressed, 20 megs uncompressed.
Attached patch squash the extra addref (obsolete) — Splinter Review
Big thanks to jdm, who found the extra addref and suggested this fix.
Attachment #608027 - Attachment is obsolete: true
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.)
OS: Mac OS X → All
Hardware: x86 → All
Attached patch folded the two patches together (obsolete) — Splinter Review
Attachment #608107 - Attachment is obsolete: true
Attachment #608109 - Attachment is obsolete: true
I guess that nsXBLDocumentInfo probably survives until shutdown anyways, so this isn't really worth landing on older branches.
Attachment #608360 - Flags: review?(bugs)
Comment on attachment 608360 [details] [diff] [review]
clean up refcounting for nsXBLDocumentInfo creation

Much better
Attachment #608360 - Flags: review?(bugs) → review+
Thanks for the fast review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/733c0022cbc6
Target Milestone: --- → mozilla14
(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.
https://hg.mozilla.org/mozilla-central/rev/733c0022cbc6
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.