Closed Bug 937766 Opened 6 years ago Closed 6 years ago

ICC: Safely allow objects to die in between ICC slices

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28
Tracking Status
firefox28 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

(Whiteboard: [qa-])

Attachments

(5 files)

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.
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).
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
I'm just bad at pushing chained git repos...
  https://tbpl.mozilla.org/?tree=Try&rev=3072def4e830
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.
Attachment #8342406 - Flags: review?(bugs)
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.
Attachment #8342407 - Flags: review?(bugs)
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.
Attachment #8342408 - Flags: review?(bugs)
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.
Attachment #8342410 - Flags: review?(bugs)
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*
Attachment #8342406 - Flags: review?(bugs) → review+
Attachment #8342407 - Flags: review?(bugs) → review+
Attachment #8342408 - Flags: review?(bugs) → review+
Attachment #8342409 - Flags: review?(bugs) → review+
Attachment #8342410 - Flags: review?(bugs) → review+
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.