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

RESOLVED FIXED in mozilla27

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gkw, Assigned: terrence)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla27
x86_64
Linux
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Created attachment 801025 [details]
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>
Flags: needinfo?(terrence)
(Assignee)

Comment 1

4 years ago
Looks like a nursery thing is flowing somewhere in the compiler where it shouldn't. Taking a look.
Assignee: general → terrence
Flags: needinfo?(terrence)
(Assignee)

Comment 2

4 years ago
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?
Flags: needinfo?(bhackett1024)
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.
Flags: needinfo?(bhackett1024)
(Assignee)

Comment 4

4 years ago
Created attachment 812867 [details] [diff] [review]
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.
Attachment #812867 - Flags: review?(bhackett1024)
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.
Attachment #812867 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 6

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

Ah, that simplifies the patch tremendously!
https://hg.mozilla.org/mozilla-central/rev/7f6d42c8c7d2
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.