712735 - [CC] Don't add JS holders with no gray children as XPConnect roots

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

These aren't a huge deal, but I'm seeing about 500 to 1100 nsXULPrototypes in each CC graph.  Jonas said these probably live for the duration of the browser, so maybe we can avoid visiting them in the cycle collector except at shutdown, or not cycle collect them altogether and add some special shutdown code to remove them.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I would guess that they'll simply go away when we shut down since they would be no longer held on to by the xul-cache or any live documents. I.e. I suspect their refcount will go down to 0 and we'll just kill them the "normal" way.

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

An alternative, and safer, way to deal with them is to keep their CC code, but make the CC code bail if called before we have started shutting down.

This way our shut-down behavior is exactly as it is now and we don't risk any cycles keeping things alive there.

Comment 3

[Andrew McCreight [:mccr8]]

Yeah, that's true.  We can even leave things like that for a few release cycles, and later cut out the CC code if nothing seems to show up.  I also need to look into how much of this is actually traversed after bug 713865.

Comment 4

[Andrew McCreight [:mccr8]]

A bunch of nsXULPrototypeNodes still show up in the CC graph even with Smaug's patches, so this may still be worth investigating.

Comment 5

[Andrew McCreight [:mccr8]]

It looks like nsXULPrototypeNodes are being added to the CC graph because they are JS holders, even if the JS they contain only pointers to black JS.  They are not nsISupports, so they are never in the purple buffer.  I think we should be able to fix this (and perhaps other things) by changing CheckParticipatesInCycleCollection, in XPCJSRuntime.  I don't 100% understand why that function is the way it is, so I'll have to talk to Peter about it.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: add less JS holders as roots

I talked to Peter, and he can't see why this only checks the last object a JS holder holds, so I switched it back to being a flag that stays on once it is on.  It starts off, and is set to true only if we see a gray JS child.  This gets rid of the bulk of nsXULPrototypeNodes from the graph.

There are a few odd cases where they hold onto nsNodeInfo (XBL) or some JS, but we can handle that in a followup.

Comment 7

[Andrew McCreight [:mccr8]]

If a JS holder is in the purple buffer, it will still be added later, but that's okay.

Comment 8

[Andrew McCreight [:mccr8]]

By "that's okay", I mean "that's necessary for correctness".

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 609869 [details] [diff] [review]
add less JS holders as roots

Try run came back looking good.

https://tbpl.mozilla.org/?tree=Try&rev=f7ec71615b11

And before you complain, these weird ifs are in proper Spidermonkey style. ;)

https://wiki.mozilla.org/JavaScript:SpiderMonkey:C%2B%2B_Coding_Style#Conditionals

Comment 10

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e919664cdf5

Comment 11

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/0e919664cdf5