911707 - Assertion failure: arr->lengthIsWritable() (setter shouldn't be called if property is non-writable), at jsarray.cpp

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

try {
    x = [];
    y = [];
    f = (function() {
        y.length = 6
    });
    Object.freeze(y);
    f();
    Object.prototype.watch.call(x, "length", (function() {
        return function() {
            f()
        }
    })());
    Array.prototype.pop.call(x)
} catch (e) {}
Array.prototype.push.call(x, "")

asserts js debug threadsafe shell on m-c changeset 1179318fb5aa with --ion-eager --ion-inlining=off at Assertion failure: arr->lengthIsWritable() (setter shouldn't be called if property is non-writable), at jsarray.cpp

My configure flags 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-threadsafe --with-ccache <some NSPR compile flags, or use --with-system-nspr>

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/c1ccfd8f31bf
user:        Eric Faust
date:        Fri Aug 30 18:50:36 2013 -0700
summary:     Bug 824393 - Part 0: Open SetPropertyIC to cases with uncertain TI. (r=bhackett)

(s-s because the stack is short and strange) Eric, is bug 824393 probably related?

Comment 1

[Christian Holler (:decoder)]

JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.

Comment 2

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 3

[Christian Holler (:decoder)]

Easier test that should also reproduce better (m-c rev 1179318fb5aa, options --fuzzing-safe --ion-eager):


x = [ "CNY", "TWD", "invalid" ];
Object.freeze(x).map(function() {
    x.length = 6
})

Comment 4

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/c1ccfd8f31bf
user:        Eric Faust
date:        Fri Aug 30 18:50:36 2013 -0700
summary:     Bug 824393 - Part 0: Open SetPropertyIC to cases with uncertain TI. (r=bhackett)

This iteration took 330.328 seconds to run.

Comment 5

[Eric Faust [:efaust]]

Attachment: Fix

Yeah, OK. Don't IC unwritable PropertyOp setter shapes. Even though it should be relevant, we mirror the state that is in that flag, so it's the easiest way to check.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 799015 [details] [diff] [review]
Fix

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

::: js/src/jit/IonCaches.cpp
@@ +2054,5 @@
>  
>      if (shape->hasSetterValue())
>          return false;
>  
> +    // Despite the vehement claims of Shape.h that writable() is only

I don't actually see these claims, only a commented-out assertion in Shape::writable() that belies them.  :-)

Comment 7

[Andrew McCreight [:mccr8]]

How bad is this, security-wise, Eric?

Comment 8

[Eric Faust [:efaust]]

Best to play it safe here, I think. Jeff and I agree that while the actual assertion that is failing is not a big deal, there could be other PropertyOp setters that are not as carefully written. I don't know what that translates to in terms of flags? moderate is my best guess.

Not hugely bad on the path taken, but with other potential consequences. Suggest we keep it closed until this lands.

Comment 9

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 10

[Eric Faust [:efaust]]

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

Comment 11

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 680c89c76100).

Comment 12

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

This landed on m-c:

http://hg.mozilla.org/mozilla-central/rev/dcd4c1b60d7f

Comment 13

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.