Last Comment Bug 732495 - shutdown leaks on a fresh profile due to XBL cache
: shutdown leaks on a fresh profile due to XBL cache
Status: RESOLVED FIXED
[MemShrink:P2]
:
Product: Core
Classification: Components
Component: XBL (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
Depends on:
Blocks: 94199
  Show dependency treegraph
 
Reported: 2012-03-02 10:53 PST by Andrew McCreight [:mccr8]
Modified: 2012-03-24 09:31 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
wontfix
wontfix
wontfix


Attachments
why nsDocuments are being held alive (24.68 KB, text/plain)
2012-03-02 11:07 PST, Andrew McCreight [:mccr8]
no flags Details
why documents are being held alive, attempt 2 (19.78 KB, text/plain)
2012-03-02 13:07 PST, Andrew McCreight [:mccr8]
no flags Details
disable startup cache (1.02 KB, patch)
2012-03-02 14:00 PST, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
hacky fix (2.05 KB, patch)
2012-03-21 11:09 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
ref count log (59.66 KB, application/x-bzip2)
2012-03-21 11:44 PDT, Andrew McCreight [:mccr8]
no flags Details
squash the extra addref (3.71 KB, patch)
2012-03-21 15:11 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
optional part 2: clean up the manual addref (2.84 KB, patch)
2012-03-21 15:17 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
folded the two patches together (4.33 KB, patch)
2012-03-21 22:12 PDT, Andrew McCreight [:mccr8]
no flags Details | Diff | Splinter Review
clean up refcounting for nsXBLDocumentInfo creation (4.39 KB, patch)
2012-03-22 09:15 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review

Description Andrew McCreight [:mccr8] 2012-03-02 10:53:54 PST
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.
Comment 1 Andrew McCreight [:mccr8] 2012-03-02 11:07:39 PST
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.
Comment 2 Andrew McCreight [:mccr8] 2012-03-02 11:10:34 PST
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)
Comment 3 Andrew McCreight [:mccr8] 2012-03-02 11:43:40 PST
The GC path is bogus. I have to fix my scripts to deal with the B and G stuff.
Comment 4 Andrew McCreight [:mccr8] 2012-03-02 13:07:28 PST
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.
Comment 5 Andrew McCreight [:mccr8] 2012-03-02 13:38:12 PST
3/2 Aurora also exhibits the same behavior.
Comment 6 Andrew McCreight [:mccr8] 2012-03-02 13:41:45 PST
Setting nglayout.debug.disable_xul_cache to true stops the shutdown leak.
Comment 7 Andrew McCreight [:mccr8] 2012-03-02 14:00:18 PST
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.
Comment 8 Andrew McCreight [:mccr8] 2012-03-02 14:22:38 PST
I guess it could be some cycle collector optimization not shutting down properly on shutdown or something like that.
Comment 9 Andrew McCreight [:mccr8] 2012-03-02 14:42:38 PST
3/3 nightly Beta Debug is also affected, which suggests it isn't anything from recent cycle collector optimizations.
Comment 10 Andrew McCreight [:mccr8] 2012-03-21 11:09:07 PDT
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.
Comment 11 Josh Matthews [:jdm] 2012-03-21 11:19:53 PDT
Yes, the existing behaviour looks incorrect. It should be using |info.forget(*aResult)| instead of the NS_IF_ADDREF call.
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-21 11:21:15 PDT
(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 Josh Matthews [:jdm] 2012-03-21 11:22:01 PDT
Whoops, info.forget(aResult).
Comment 14 Andrew McCreight [:mccr8] 2012-03-21 11:36:40 PDT
(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.
Comment 15 Kyle Huey [:khuey] (khuey@mozilla.com) 2012-03-21 11:39:55 PDT
Can you post a refcount log for the relevant object?
Comment 16 Andrew McCreight [:mccr8] 2012-03-21 11:44:22 PDT
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.
Comment 17 Andrew McCreight [:mccr8] 2012-03-21 15:11:51 PDT
Created attachment 608107 [details] [diff] [review]
squash the extra addref

Big thanks to jdm, who found the extra addref and suggested this fix.
Comment 18 Andrew McCreight [:mccr8] 2012-03-21 15:17:08 PDT
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.)
Comment 19 Andrew McCreight [:mccr8] 2012-03-21 22:12:27 PDT
Created attachment 608228 [details] [diff] [review]
folded the two patches together
Comment 20 Andrew McCreight [:mccr8] 2012-03-22 09:15:39 PDT
Created attachment 608360 [details] [diff] [review]
clean up refcounting for nsXBLDocumentInfo creation
Comment 21 Andrew McCreight [:mccr8] 2012-03-22 09:16:30 PDT
Try run looked fine https://tbpl.mozilla.org/?tree=Try&rev=76026331498f
Comment 22 Andrew McCreight [:mccr8] 2012-03-22 13:27:45 PDT
I guess that nsXBLDocumentInfo probably survives until shutdown anyways, so this isn't really worth landing on older branches.
Comment 23 Olli Pettay [:smaug] 2012-03-22 13:43:49 PDT
Comment on attachment 608360 [details] [diff] [review]
clean up refcounting for nsXBLDocumentInfo creation

Much better
Comment 24 Andrew McCreight [:mccr8] 2012-03-22 13:47:54 PDT
Thanks for the fast review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/733c0022cbc6
Comment 25 Andrew McCreight [:mccr8] 2012-03-22 13:52:23 PDT
(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 Marco Bonardo [::mak] 2012-03-23 05:48:14 PDT
https://hg.mozilla.org/mozilla-central/rev/733c0022cbc6

Note You need to log in before you can comment on or make changes to this bug.