912734 - GenerationalGC: Crash [@ JSObject::is]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

(function() {
    Object.defineProperty(this, "h2", {
        e: false,
        e: false,
        get: function() {
            return {}
        }
    })
})()
a2 = new Array
Object.create(a2);
(function() {
    Object.defineProperty(this, "t2", {
        e: true,
        e: RegExp(""),
        get: function() {
            return Uint32Array(this.a2)
        }
    })
})()
r = (function() {
    for each(let c in []) {}
})()
var r = 1;
s = ""
print(s.match(r));
r.l
with(b = 3);
t2;
(function() {
    Object.defineProperty(a2, 3, {
        e: false,
        e: true,
        get: (function() {
            evalcx("[]", s.g)
        })
    })
})()
h2
r = RegExp("");
r.exec();
schedulegc(7);
t2;

crashes js debug shell (tested with a threadsafe deterministic 32-bit debug build) on m-i changeset 4dceda951fba with --no-baseline at JSObject::is

My configure options are:

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>

Comment 1

[Jon Coppeard (:jonco)]

Attachment: fix-fuzz-crash

The crash is happening because a minor GC moves the a typed array's elements while it is being populated in copyFromArray(), causing it to overwrite whatever ends up at that address.

This patch adds a check to see if gcNumber had changed, and recalculates src and dest pointers if so.  This bug concerns the dest pointer, but I don't see why the same thing couldn't apply to the src pointer too.

There were some comments there about returning immediately in the case of a GC, which I didn't understand and didn't seem to be correct, so I removed them.

Comment 2

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

Comment on attachment 809932 [details] [diff] [review]
fix-fuzz-crash

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

Hm. The comment you deleted was false in the version it was added, because the else branch that calls JSObject::getElement was already there. So I don't know what I was thinking.

But the comment is still valid for the first loop -- neither src nor dest can be held live across a GC call, because the only way nativeFromValue can gc is if it fails, and if it fails src & dest are both dead.

This region of code makes me a little nervous, because it is the precise reason why I spent a bunch of effort in optimizing the dynamic rooting analysis for the case where the stacks are almost entirely identical. This patch doesn't change that, but I still remember that this is a perf-sensitive bit of code, and that's the reason why I hesitate. Without the added gcNumber check, this should reduce to a memcpy. (Whether it does or not, I don't know.) And the gcNumber check isn't necessary (except in the else clause), but I don't know if the compiler can figure that out or not.

So, I think I would prefer one of these two options:

1. Verify that this doesn't regress perf, make MaybeCheckStack bump gcNumber if it doesn't already, and remove the SkipRoots. I *think* this would be valid, though it would make the control flow with dynamic rooting analysis rather different from the regular flow (regular flow -- never GC in the dense array case so src/dest are never recalculated, analysis flow -- recalculate on every iteration. Or in the else branch, rarely recalculate in the regular flow.)

2. Apply this change only to the else clause. More complicated and brittle, but maybe better perf in the dense array case.

It's totally possible that either (1) the compiler is smart enough to optimize out the gcNumber check, or (2) it doesn't really matter in the first place. So if perf testing shows that this is irrelevant, I'd go for #1.

Comment 3

[Jon Coppeard (:jonco)]

Attachment: fix-fuzz-crash v2

My bad - I didn't spot that there is in fact no GC possible in the first loop.

I've removed the gc number check from the first loop and added an assertion that no GC happened instead, and I've reinstated the comment.

Comment 4

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

Comment on attachment 810580 [details] [diff] [review]
fix-fuzz-crash v2

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

Thanks! I still feel dumb about missing the JS_GetProperty GC...

::: js/src/vm/TypedArrayObject.cpp
@@ +2285,5 @@
>                  if (!nativeFromValue(cx, v, &n))
>                      return false;
> +
> +                /*
> +                 * Detect when a GC has occured so we can update src and dest

*occurred

@@ +2289,5 @@
> +                 * Detect when a GC has occured so we can update src and dest
> +                 * pointers in case they have been moved.
> +                 */
> +                if (runtime->gcNumber != gcNumber) {
> +                    src = ar->getDenseElements();

src is not used in this else branch. Can you remove it from here and move everything related to it (the declaration and SkipRoot) into the first clause?

Comment 5

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c9e0a88c88fa

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c9e0a88c88fa