Closed
Bug 913715
Opened 11 years ago
Closed 11 years ago
GenerationalGC: Assertion failure: !UninlinedIsInsideNursery(GetIonContext()->runtime, ptr), at jit/CompilerRoot.h
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: gkw, Assigned: terrence)
References
Details
(Keywords: assertion, testcase)
Attachments
(2 files)
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
bhackett1024
:
review+
|
Details | Diff | Splinter Review |
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•11 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•11 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)
Comment 3•11 years ago
|
||
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•11 years ago
|
||
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 5•11 years ago
|
||
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•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f6d42c8c7d2
Ah, that simplifies the patch tremendously!
Comment 7•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in
before you can comment on or make changes to this bug.
Description
•