1466633 - Assertion isGcMarkingTracer || IsBufferGrayRootsTracer(trc) || IsUnmarkGrayTracer(trc)) with GCManagedDeletePolicy free during sweep

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

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

efaust ran into an assertion failure from

    bool isGcMarkingTracer = trc->isMarkingTracer();
    MOZ_ASSERT_IF(zone->requireGCTracer(),
                  isGcMarkingTracer || IsBufferGrayRootsTracer(trc) || IsUnmarkGrayTracer(trc));

when he released a GCManagedDeletePolicy UniquePtr during a sweep. I don't entirely understand that assertion. But I guess this code ran for the delete policy:

    void operator()(const T* constPtr) {
        if (constPtr) {
            auto ptr = const_cast<T*>(constPtr);
            gc::ClearEdgesTracer trc;
            ptr->trace(&trc);
            js_delete(ptr);
        }
    }

and that ClearEdgesTracer must have hit the assert. efaust, if you have the full stack, can you put it here?

Can we handle this automatically, by skipping the ClearEdgesTracer stuff if we're in a GC slice?

Or would that conflict with removing nursery collections for every sweep slice? If so, do we need to relax the assertion or something?

I'm passing this on to better minds.

Comment 1

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

Oh, and as efaust points out, it would be nice to be able to use the same UniquePtr type for both stack- and heap-allocated versions of a type.

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] (PTO June 31) from comment #0)
> Can we handle this automatically, by skipping the ClearEdgesTracer stuff if
> we're in a GC slice?

Yes.  We don't need to do this if are sweeping.  The GCManagedDeletePolicy is there to ensure things work correctly if the object is deleted *outside* of the GC (I think I already raised a bug about the name...).

> Or would that conflict with removing nursery collections for every sweep slice?

No, that change already needs to ensure that there are no pointers from the store buffer to things that might get swept.

Comment 3

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

Attachment: GCManagedDeletePolicy: do not clear edges during GC

This adds a Tls lookup. I don't know if there's a better way.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8983629 [details] [diff] [review]
GCManagedDeletePolicy: do not clear edges during GC

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

I can't immediately think of one.  This seems fine.

Maybe I'm being paranoid, but can you make this assert CurrentThreadIsGCSweeping() if we are collecting?

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/17bb0a45975b
GCManagedDeletePolicy: do not clear edges during GC, r=jonco

Comment 6

[Stefan Hindli [:stefan_hindli]]

Backed out 3 changesets (bug 1466633, bug 1467248, bug 1466387) for spidermonkey bustages in non262/regress/regress-1466387-worker-grayroot.js on a CLOSED TREE

Problematic push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1e833c6a3ba2a4d592868fd2167e94c1cb933004&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=182617140
Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a8231f7c5073c597ab8bb21cee441f4e54f21c52&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified
Log: https://treeherder.mozilla.org/logviewer.html#?job_id=182617140&repo=mozilla-inbound&lineNumber=561457

[task 2018-06-09T00:19:25.943Z] TEST-UNEXPECTED-FAIL | non262/regress/regress-1466387-worker-grayroot.js | (args: "--dll /builds/worker/workspace/breakpad-tools/libbreakpadinjector.so --ion-eager --ion-offthread-compile=off --ion-check-range-analysis --ion-extra-checks --no-sse3 --no-threads") [0.1 s]

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2492ed8bf219
GCManagedDeletePolicy: do not clear edges during GC, r=jonco

Comment 8

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

Re-landed since this one is ok as per https://treeherder.mozilla.org/#/jobs?repo=try&revision=b48c6be7de90c19a012f19aa4c67d61479b26565

Comment 9

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/2492ed8bf219