Last Comment Bug 712735 - [CC] Don't add JS holders with no gray children as XPConnect roots
: [CC] Don't add JS holders with no gray children as XPConnect roots
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla14
Assigned To: Andrew McCreight [:mccr8]
: Andrew Overholt [:overholt]
Depends on:
Blocks: 716598 722715
  Show dependency treegraph
Reported: 2011-12-21 12:17 PST by Andrew McCreight [:mccr8]
Modified: 2012-03-29 08:48 PDT (History)
7 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

add less JS holders as roots (1.62 KB, patch)
2012-03-27 13:42 PDT, Andrew McCreight [:mccr8]
bugs: review+
Details | Diff | Splinter Review

Description User image Andrew McCreight [:mccr8] 2011-12-21 12:17:49 PST
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 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-12-21 12:21:20 PST
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 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-01-09 11:06:06 PST
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 User image Andrew McCreight [:mccr8] 2012-01-09 11:37:07 PST
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 User image Andrew McCreight [:mccr8] 2012-01-19 08:45:51 PST
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 User image Andrew McCreight [:mccr8] 2012-03-26 20:26:18 PDT
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 User image Andrew McCreight [:mccr8] 2012-03-27 13:42:17 PDT
Created attachment 609869 [details] [diff] [review]
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 User image Andrew McCreight [:mccr8] 2012-03-27 13:44:28 PDT
If a JS holder is in the purple buffer, it will still be added later, but that's okay.
Comment 8 User image Andrew McCreight [:mccr8] 2012-03-27 13:45:00 PDT
By "that's okay", I mean "that's necessary for correctness".
Comment 9 User image Andrew McCreight [:mccr8] 2012-03-28 21:05:46 PDT
Comment on attachment 609869 [details] [diff] [review]
add less JS holders as roots

Try run came back looking good.

And before you complain, these weird ifs are in proper Spidermonkey style. ;)
Comment 10 User image Andrew McCreight [:mccr8] 2012-03-28 21:27:32 PDT
Comment 11 User image Matt Brubeck (:mbrubeck) 2012-03-29 08:48:07 PDT

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