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.
Created attachment 602415 [details] why nsDocuments are being held alive 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.
Created attachment 602457 [details] why documents are being held alive, attempt 2 I tried this again, and the nsDocuments were only being held live by nsXBLDocumentInfo.
3/2 Aurora also exhibits the same behavior.
Setting nglayout.debug.disable_xul_cache to true stops the shutdown leak.
Created attachment 602481 [details] [diff] [review] disable startup cache With this patch applied, I no longer see the leak. All the patch does is set startupCache to false.
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.
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? This is a very hacky fix. I'm not sure what the idiomatic way to do this is.
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.
(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] (email@example.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?
Created attachment 608041 [details] ref count log This is the ref count log for nsXBLDocumentInfo with id 1 (chosen arbitrarily). 61k compressed, 20 megs uncompressed.
Created attachment 608107 [details] [diff] [review] squash the extra addref Big thanks to jdm, who found the extra addref and suggested this fix.
Created attachment 608109 [details] [diff] [review] optional part 2: clean up the manual addref 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.)
Created attachment 608228 [details] [diff] [review] folded the two patches together
Created attachment 608360 [details] [diff] [review] clean up refcounting for nsXBLDocumentInfo creation
Try run looked fine https://tbpl.mozilla.org/?tree=Try&rev=76026331498f
I guess that nsXBLDocumentInfo probably survives until shutdown anyways, so this isn't really worth landing on older branches.
Comment on attachment 608360 [details] [diff] [review] clean up refcounting for nsXBLDocumentInfo creation Much better
Thanks for the fast review! https://hg.mozilla.org/integration/mozilla-inbound/rev/733c0022cbc6
(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.