Closed Bug 975959 Opened 6 years ago Closed 6 years ago

GenerationalGC: Crash [@ js::jit::AssertValidObjectPtr]

Categories

(Core :: JavaScript: GC, defect, critical)

x86
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: gkw, Assigned: terrence)

References

(Blocks 1 open bug)

Details

(Keywords: testcase, Whiteboard: [jsbugmon:])

Crash Data

Attachments

(1 file)

try {
    y = z = [];
    y[1] = x
    for (var e, x = [];; d) {}
} catch (e) {}
try {
    Object.defineProperty(this, "y", {
        get: function() {
            z.filterPar(function() {})
        }
    })(1 instanceof 2)
} catch (e) {}
y
verifyprebarriers()
y
verifyprebarriers()
y

crashes js debug shell (tested with a threadsafe deterministic 32-bit debug build) on m-c changeset 1238ef12b996 with at js::jit::AssertValidObjectPtr and crashes js opt shell with a memory read failure.

My configure options are: (32-bit opt shell)

LD=ld CROSS_COMPILE=1 CXX="clang++ -Qunused-arguments -arch i386" RANLIB=ranlib CC="clang -Qunused-arguments -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments" HOST_CXX="clang++ -Qunused-arguments" sh ./configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --enable-optimize --disable-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>

32-bit debug shell:

LD=ld CROSS_COMPILE=1 CXX="clang++ -Qunused-arguments -arch i386" RANLIB=ranlib CC="clang -Qunused-arguments -arch i386" AS=$CC AR=ar STRIP="strip -x -S" HOST_CC="clang -Qunused-arguments" HOST_CXX="clang++ -Qunused-arguments" sh ./configure --target=i386-apple-darwin9.2.0 --enable-macos-target=10.5 --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)
Flags: needinfo?(jcoppeard)
> build) on m-c changeset 1238ef12b996 with at js::jit::AssertValidObjectPtr

with * --fuzzing-safe --ion-parallel-compile=off --ion-eager *
Flags: needinfo?(terrence)
This is a nursery allocation that gets stored into a pre-tenured CallObject's arguments vector that then escapes through a heavyweight function. The escaping function refers to the arg directly as an upvar after a GC. This case is a bit detailed, which is probably why it has only come up once before. There was a similarly hard to track down fuzz bug when working on bug 889129 that caught this exact case. Brian removed this code in bug 941311, probably by accident as it wasn't in the uploaded patch. Should be easy to add back.

I think bug 975961 and bug 976021 are both dups of this issue; will fix and verify tomorrow.
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Ah, in our OOL path we have:

#ifdef JSGC_GENERATIONAL
    // The JIT creates call objects in the nursery, so elides barriers for
    // the initializing writes. The interpreter, however, may have allocated
    // the call object tenured, so barrier as needed before re-entering.
    if (!IsInsideNursery(cx->runtime(), obj))
        cx->runtime()->gcStoreBuffer.putWholeCell(obj);
#endif

Sa failure to allocate in the nursery from the jit is a failure in this one case. Unfortunately, in newGCThing we skip to allocating tenured if the nursery is disabled. Since the pre-barrier verifier disables the nursery, we end up baking in a tenured allocation in a way we can't really test for, since codegen happens off main thread.

Until bug 969012 is ready, we'd best just bake in nursery allocations when that is possible and accept a slower fallback path at runtime in this crazy edge case.
Duplicate of this bug: 976021
Duplicate of this bug: 975961
Assignee: nobody → terrence
Status: NEW → ASSIGNED
Attachment #8381580 - Flags: review?(jdemooij)
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Comment on attachment 8381580 [details] [diff] [review]
do_not_accidentally_tenure-v0.diff

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

Good catch and always nice to get rid of dead code :)
Attachment #8381580 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/ca08ca9fd157
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Flags: needinfo?(jcoppeard) → in-testsuite+
Depends on: 979142
You need to log in before you can comment on or make changes to this bug.