905947 - Assertion failure: attrs & 0x04, at jsarray.cpp

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

Attachment: stack

s = newGlobal()
evalcx("\
    eval = Proxy(RegExp().exec(), objectEmulatingUndefined)\
", s);
evalcx("\
    Object.defineProperty(eval, \"length\", ({\
        configurable:true,\
    }));\
", s)

asserts js debug shell on m-c changeset 1ed5a88cd4d0 without any CLI arguments at Assertion failure: attrs & 0x04, at jsarray.cpp

Comment 1

[André Bargull [:anba]]

Simplified test case, fails with the same assertion:
---
Object.defineProperty(new Proxy([], {}), "length", ({configurable:true}))
---

Comment 2

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/09dcdc2b2120
user:        Sankha Narayan Guria
date:        Tue Jul 02 21:57:14 2013 +0530
summary:     Bug 788172 - Make Proxy a function. r=ejpbruel

jorendorff, you wrote most of the patch in bug 788172, is it a likely regressor?

Comment 3

[Tom S [:evilpie]]

I guess you could try replacing, "eval = Proxy(" with "eval = new Proxy(".

Comment 4

[Jason Orendorff [:jorendorff]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)
> jorendorff, you wrote most of the patch in bug 788172, is it a likely
> regressor?

evilpie is right: That patch simply made `Proxy(...)` do the same thing as `new Proxy(...)`. It probably did not cause the bug.

Comment 5

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

(In reply to André Bargull from comment #1)
> Simplified test case, fails with the same assertion:
> ---
> Object.defineProperty(new Proxy([], {}), "length", ({configurable:true}))
> ---

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/8eac2a78a791
user:        Jeff Walden
date:        Tue Mar 19 17:12:06 2013 -0700
summary:     Bug 858381 - Implement non-writable array lengths, and add a boatload of tests.  r=jorendorff and r=bhackett for the major parts of this, r=jandem for the methodjit changes, r=jimb on a debugger test change, r=nmatsakis for the parallel test.  (More details available in the bug, where individual components of the fix were separately reviewed.)

Waldo, is bug 858381 a likely regressor?

Comment 6

[Jason Orendorff [:jorendorff]]

Yes, that's the right regressor.

Comment 7

[Jeff Walden [:Waldo]]

The proxy defineProperty code tries to perform the definition on the target.  It creates a descriptor from the object passed in.  But it's a PropertyDescriptor, which has no concept of a field being absent from the descriptor.  The default value for configurability is true, so js::ArraySetLength gets that, and things die.

Execution continues on to CanonicalizeArrayLengthValue, which is passed the value from the PropertyDescriptor object.  That happens to be |undefined| because of the bug here, so the testcase here safely throws an exception.  If the descriptor's augmented with a value field, it happens that the missing JSPROP_PERMANENT is added into the attributes within JSObject::changeProperty, from the existing shape->attrs:

761	JSObject::changeProperty(ExclusiveContext *cx, HandleObject obj, HandleShape shape, unsigned attrs,
762	                         unsigned mask, PropertyOp getter, StrictPropertyOp setter)
763	{
764	    JS_ASSERT(obj->nativeContains(cx, shape));
765	
766	    attrs |= shape->attrs & mask;

So the end consequence here is an assertion failure but no more than safe but incorrect results in non-debug builds.

So in the end we need to fix our property-definition APIs to work in terms of property descriptor structures that allow fields to be missing, to make progress here.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Don't assert when redefining an array's length to be configurable via JSObject::defineProperty

Proxy semantics don't actually really say what should happen here, regarding throwing.  See <http://wiki.ecmascript.org/doku.php?id=harmony:proxies_semantics>.  But at least this doesn't fail any jstests/jit-tests in the shell, so it's probably good enough to avoid the assertion.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: v2

Actually, we should be modifying ArraySetLength, surely.  Also added some tests, and fixed a related issue I should have noticed before.

Comment 10

[Jason Orendorff [:jorendorff]]

Comment on attachment 793207 [details] [diff] [review]
v2

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

r=me with the comment addressed.

::: js/src/jsarray.cpp
@@ +437,5 @@
>      MOZ_ASSERT(id == NameToId(cx->names().length));
> +
> +    // Abort if we're being asked to change enumerability or configurability.
> +    // This behavior is spread throughout the ArraySetLength algorithm, but
> +    // our array implementation lets us check this in just one place.

I don't understand the comment.

Does this mean that it's correct to detect this particular kind of error here, out of order, rather than in the steps of the algorithm where we are supposed to call OrdinaryDefineOwnProperty?

If so, why aren't there cases where that is observably wrong--that is, where we throw this error when we should be throwing a RangeError in step 4?

    // both of these should throw RangeError
    Object.defineProperty(Object.freeze([]),
                          "length", {value: (1 << 31) * 2});
    Object.defineProperty(new Proxy(Object.freeze([]), {}),
                          "length", {value: (1 << 31) * 2});

It'd be nice if the comment could explain a little more why it's ok to check early.

Comment 11

[Jeff Walden [:Waldo]]

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

The out-of-orderness doesn't actually matter, because the Object.defineProperty code path does all its own checks of the property-descriptor conflict stuff, because our defineProperty hook can't do them.  It would only affect JSAPI attempts to redefine an array length, where that array length is overbig.  But yeah, you're right, there's a slight issue here, good catch.  I moved it below the range-check stuff and beefed up the comment a bit further to address your comments.

Comment 12

[Christian Holler (:decoder)]

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

Comment 13

[Ed Morley [:emorley]]

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