The default bug view has changed. See this FAQ.

shutdown leaks on a fresh profile due to XBL cache

RESOLVED FIXED in mozilla14

Status

()

Core
XBL
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

Trunk
mozilla14
Points:
---

Firefox Tracking Flags

(firefox11 wontfix, firefox12 wontfix, firefox13 wontfix, firefox-esr10 wontfix)

Details

(Whiteboard: [MemShrink:P2])

Attachments

(3 attachments, 6 obsolete attachments)

(Assignee)

Description

5 years ago
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

5 years ago
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.
(Assignee)

Comment 2

5 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

5 years ago
The GC path is bogus. I have to fix my scripts to deal with the B and G stuff.
(Assignee)

Comment 4

5 years ago
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.
Attachment #602415 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
3/2 Aurora also exhibits the same behavior.
(Assignee)

Comment 6

5 years ago
Setting nglayout.debug.disable_xul_cache to true stops the shutdown leak.
Component: General → XBL
QA Contact: general → xbl
(Assignee)

Comment 7

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 94199
(Assignee)

Comment 8

5 years ago
I guess it could be some cycle collector optimization not shutting down properly on shutdown or something like that.
(Assignee)

Comment 9

5 years ago
3/3 nightly Beta Debug is also affected, which suggests it isn't anything from recent cycle collector optimizations.
(Assignee)

Updated

5 years ago
Summary: shutdown leaks on a fresh profile → shutdown leaks on a fresh profile due to XBL cache
Assignee: nobody → continuation
Whiteboard: [MemShrink] → [MemShrink:P2]
(Assignee)

Comment 10

5 years ago
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.
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).
(Assignee)

Comment 14

5 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

5 years ago
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.
(Assignee)

Comment 17

5 years ago
Created attachment 608107 [details] [diff] [review]
squash the extra addref

Big thanks to jdm, who found the extra addref and suggested this fix.
Attachment #608027 - Attachment is obsolete: true
(Assignee)

Comment 18

5 years ago
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.)
(Assignee)

Updated

5 years ago
OS: Mac OS X → All
Hardware: x86 → All
(Assignee)

Comment 19

5 years ago
Created attachment 608228 [details] [diff] [review]
folded the two patches together
Attachment #608107 - Attachment is obsolete: true
Attachment #608109 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Created attachment 608360 [details] [diff] [review]
clean up refcounting for nsXBLDocumentInfo creation
Attachment #608228 - Attachment is obsolete: true
(Assignee)

Comment 21

5 years ago
Try run looked fine https://tbpl.mozilla.org/?tree=Try&rev=76026331498f
(Assignee)

Comment 22

5 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

5 years ago
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+
(Assignee)

Comment 24

5 years ago
Thanks for the fast review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/733c0022cbc6
Target Milestone: --- → mozilla14
(Assignee)

Comment 25

5 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.
https://hg.mozilla.org/mozilla-central/rev/733c0022cbc6
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
status-firefox14: affected → ---
You need to log in before you can comment on or make changes to this bug.