937766 - ICC: Safely allow objects to die in between ICC slices

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

To safely run incremental cycle collection, the CC must be able to deal with objects dying while the CC is running, because the rest of the browser is allowed to do whatever it wants, and the CC graph holds pointers to these objects.

There are three categories of objects, which must all must be removed from the graph if they die during a CC: snow-white-able objects, JS objects, and XPCWN/XPCWJS (these are C++, but do not support snow-white.

For snow-whitable objects (the normal case for C++ objects), this is just a matter of removing dying objects from the graph in SnowWhiteKiller. (Pre snow white, handling this was pretty gross.)

For JS objects, the CC has no way to see when an individual JS object dies, but they can only die during a garbage collection.  Therefore, if when a GC starts there is an ICC in progress, the CC finishes itself synchronously.  This is potentially pretty janky, but hopefully it won't happen much.

For XPCWN and XPCWJ, I add a new callback nsCycleCollector_objectDying() that simply removes the object in question from the graph, and call it from the destructor of those classes.

When an object is removed is removed from the graph, the mPointer and mPaticipant fields of its PtrInfo are nulled out.  This requires adding a bunch of null checks to the CC to avoid null derefs.  Note that because we nulled them out, we'll just get a safe null deref if we forget a check.

A final bit of oddness that we must deal with is that the traversal of nodes caches information in an array gCCBlackMarkedNodes, which weakly holds pointers to nodes that can die.  This data structure must be modified a bit to allow dying objects to be removed.

Comment 1

[Andrew McCreight [:mccr8]]

Rather than hacking up weird infrastructure for dealing with XPCWN and XPCWJ, as described in comment 0, I am planning on turning them into regular purple buffered classes (bug 944492 and bug 942528).

Comment 2

[Andrew McCreight [:mccr8]]

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

Comment 3

[Andrew McCreight [:mccr8]]

Huh, I have no idea why that didn't build.  It builds locally for me.  Maybe it will work if I push again...

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

Comment 4

[Andrew McCreight [:mccr8]]

I'm just bad at pushing chained git repos...
  https://tbpl.mozilla.org/?tree=Try&rev=3072def4e830

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Remove dying nodes from gCCBlackMarkedNodes. r=smaug

With ICC, arbitrary other activity can happen while nodes are in gCCBlackMarkedNodes,
so the nodes can die, because gCCBlackMarkedNodes only holds a weak reference.
To avoid this, remove nodes from gCCBlackMarkedNodes when they are going away.

Comment 6

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Make SnowWhiteKiller remove dying things from the CC graph. r=smaug

If we purge snow white objects while ICC is in progress, we need to
make sure to remove anything from the CC graph to avoid dangling pointers.
We don't need to do that after shutdown.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Add mParticipant null checks. r=smaug

When an object dies during an incremental cycle collection, we null out
its mParticipant, so we must add various null checks to avoid crashing
when we reach the CC graph representation of an object that has died.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Add GCGraph::IsEmpty method. r=smaug

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 5 - Implement PrepareForGarbageCollection. r=smaug

Running the garbage collector can cause objects in the CC graph to
die, so just finish off an incremental cycle collection when we
start a GC.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8342406 [details] [diff] [review]
part 1 - Remove dying nodes from gCCBlackMarkedNodes. r=smaug


>-nsAutoTArray<nsINode*, 1020>* gCCBlackMarkedNodes = nullptr;
>+StaticAutoPtr<nsTHashtable<nsPtrHashKey<nsINode> > > gCCBlackMarkedNodes;
I think you don't need space between > and > these days.


>+static PLDHashOperator
>+VisitBlackMarkedNode(nsPtrHashKey<nsINode>* aEntry, void *)
void*

Comment 11

[Andrew McCreight [:mccr8]]

Thanks for the reviews! I fixed the two things you pointed out for patch 1.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/4dcb040e3c84
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b015eac4566f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9a4f57330003
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/abc785d60d75
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/edb01fe9d000

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4dcb040e3c84
https://hg.mozilla.org/mozilla-central/rev/b015eac4566f
https://hg.mozilla.org/mozilla-central/rev/9a4f57330003
https://hg.mozilla.org/mozilla-central/rev/abc785d60d75
https://hg.mozilla.org/mozilla-central/rev/edb01fe9d000