Closed Bug 712735 Opened 12 years ago Closed 12 years ago

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


(Core :: XPConnect, defect)

Not set





(Reporter: mccr8, Assigned: mccr8)


(Blocks 1 open bug)


(Whiteboard: [Snappy])


(1 file)

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.
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.
Blocks: 716598
No longer blocks: 698919
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.
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.
A bunch of nsXULPrototypeNodes still show up in the CC graph even with Smaug's patches, so this may still be worth investigating.
Blocks: 722715
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.
Assignee: nobody → continuation
Summary: investigate not cycle collecting nsXULPrototype nodes → avoid adding nsXULPrototype nodes to the CC graph
Component: XUL → XPConnect
QA Contact: xptoolkit.widgets → xpconnect
Summary: avoid adding nsXULPrototype nodes to the CC graph → [CC] Don't add JS holders with no gray children as XPConnect 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.
If a JS holder is in the purple buffer, it will still be added later, but that's okay.
By "that's okay", I mean "that's necessary for correctness".
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. ;)
Attachment #609869 - Flags: review?(bugs)
Attachment #609869 - Flags: review?(bugs) → review+
Whiteboard: [Snappy]
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.