1371234 - Nursery::queueSweepAction uses nursery allocation which is fallible

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Jon Coppeard (:jonco)]

Nursery::queueSweepAction is used for queuing thunks to dispose of C++ objects which might have store buffer entries pointing into them.  It uses nursery allocation to do this, which can fail if called at the wrong point.

We should investigate handling this in a different way.

This API is seeing more use due to scope data which creates and destroys objects during normal XDR decoding.  This was originally intended only to be used for destroying objects when initialisation failed due to OOM.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: bug1371234-clear-deleted-edges

The problem with deleting GC managed objects outside of the GC is that they can contain GCPtrs and ~GCPtr doesn't trigger any barriers.  Without this, we may have store buffer pointers into the object which will become invalid when it's freed.

Not only is it not necessary to trigger these barriers while sweeping, it's not always possible to do this because the referent of the pointer might have already been freed by this point.

The current solution is for the GCMangagedDeletePolicy to queue to object for deletion at a later point when we know it's safe to do so.  However this requires allocation, which is bad because one thing that can cause this to happen is failing to initialize an object due to OOM.

This patch uses a different approach and clears the object's GC pointers (by tracing it) while also triggering barriers.  This will remove any store buffer entries pointing into the object and make it safe to delete immediately.

This clears every edge, not just those that are GCPtrs.  I'm pretty sure this is safe.

I had to add a couple more TraceEdge calls in places where they're not strictly necessary for GC marking.  I also refactored the debugger tracing to common up some code.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff245df6dca4667c4c16b6a83da3e2026ae23e8a

Comment 2

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8875764 [details] [diff] [review]
bug1371234-clear-deleted-edges

Review of attachment 8875764 [details] [diff] [review]:
-----------------------------------------------------------------

This seems like a good idea.

::: js/src/gc/Zone.h
@@ +911,5 @@
> +#endif
> +
> +        template <typename S>
> +        void clearEdge(S** thingp) {
> +            InternalBarrierMethods<S*>::preBarrier(*thingp);

Why pre-barrier? It seems like this clearing would always happen in the post barrier. It doesn't seem like the cell you're clearing out should be marked just because you're clearing it.

::: js/src/vm/Debugger.cpp
@@ +3172,5 @@
>  Debugger::trace(JSTracer* trc)
>  {
> +    TraceEdge(trc, &object, "Debugger Object");
> +
> +    // TODO: trace this in traceForMovingGC above? Common up two methods?

Probably ought to address this TODO. Maybe trace it in traceForMovingGC, then file a bug for merging the two and put the number in a comment?

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #2)
> Why pre-barrier? It seems like this clearing would always happen in the post
> barrier. It doesn't seem like the cell you're clearing out should be marked
> just because you're clearing it.

This is probably unnecessary, but it is what normally happens when a HeapPtr is destroyed or cleared.  The rationale is that we are removing an edge from the GC graph so we need to make sure that this doesn't mess up our incremental marking invariant.  It's unliklely that the main object we're operating on is part of the GC graph yet, but I think it's safer to do this just in case.

> Probably ought to address this TODO.

Oh, I actually did what I what I intended here (changed traceForMovingGC to call trace and remove the duplicated parts) I just forgot to remove the comment :)

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2dbc6a1e815
Clear GC edges when deleting a GC managed object outside a GC r=sfink

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/d2dbc6a1e815