913715 - GenerationalGC: Assertion failure: !UninlinedIsInsideNursery(GetIonContext()->runtime, ptr), at jit/CompilerRoot.h

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: stack

try {
    (function() {
        Object.defineProperty(this, "x", {
            get: function() {
                Object.defineProperty(this, "y", {
                    configurable: true,
                    get: function() {
                        return Proxy(this.y)
                    }
                });
                x;
            }
        })
    })()
    x
} catch (e) {}
try {
    x
} catch (e) {}
try {
    x
} catch (e) {}
try {
    y
} catch (e) {}
try {
    y
} catch (e) {}
try {
    y
} catch (e) {}

asserts js debug shell (tested with a threadsafe deterministic 64-bit debug build) on m-c changeset 77ed46bf4c1a without any CLI arguments at Assertion failure: !UninlinedIsInsideNursery(GetIonContext()->runtime, ptr), at jit/CompilerRoot.h

My configure options are:

--enable-optimize --enable-debug --enable-profiling --enable-gczeal --enable-debug-symbols --enable-methodjit --enable-type-inference --disable-tests --enable-more-deterministic --enable-exact-rooting --enable-gcgenerational --with-ccache --enable-threadsafe <other NSPR options>

Comment 1

[Terrence Cole [:terrence]]

Looks like a nursery thing is flowing somewhere in the compiler where it shouldn't. Taking a look.

Comment 2

[Terrence Cole [:terrence]]

The |get:function| lambda JSFunction object for "y" gets created in the nursery from the jitcode for "x", working as expected. Then later we go to recompile "x" and decide to inline "y" into it. However, "y"'s call object is still in the nursery. This compilation path tries to attach a nursery thing into the MIR. This is blatantly not allowed, hence the assertion.

Brian, what is the expected behavior here?

Comment 3

[Brian Hackett [Laid off!]]

Generally, we rely on the type set tests to filter out nursery things read from the GC heap, but that doesn't happen here because getPropTryCommonGetter doesn't look at the type information for the actual getter being read (properties with getters have unknown type information, essentially) and relies on shape checks to do what it wants.  I don't know anywhere else in the builder with this behavior, and the test case situation seems bizarre enough that just having getPropTryCommonGetter or one of its helpers test for a function in the nursery seems ok, if unpalatable.

Comment 4

[Terrence Cole [:terrence]]

Attachment: fuzz_913715-v0.diff

Excellent, that's exactly what I thought the situation was, but did not want to do something so hackish without being sure.

The patch is a little awkward because getPropTryCommonGetter must succeed if a getter is present or, I think, the getter will be ignored. I chose to duplicate the fallback MCallGetProperty instead of messing with the clean organization of jsop_getprop: adding confusion to that method seems like it would be a terrible idea.

Comment 5

[Brian Hackett [Laid off!]]

Comment on attachment 812867 [details] [diff] [review]
fuzz_913715-v0.diff

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

::: js/src/jit/IonBuilder.cpp
@@ +8412,5 @@
> +        current->push(call);
> +        if (!resumeAfter(call))
> +            return false;
> +        *emitted = true;
> +        return true;

This stuff isn't necessary, just leave *emitted at false and return true.  This function can't be relied on to succeed because there's never any guarantee that type information is perfect, and later attempts to optimize getprop must do the right thing regardless of whether this was turned on.

Comment 6

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6d42c8c7d2

Ah, that simplifies the patch tremendously!

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7f6d42c8c7d2