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

RESOLVED FIXED in mozilla26

Status

()

Core
JavaScript Engine
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gkw, Assigned: Waldo)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
mozilla26
assertion, regression, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [jsbugmon:update,ignore])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 791184 [details]
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
Simplified test case, fails with the same assertion:
---
Object.defineProperty(new Proxy([], {}), "length", ({configurable:true}))
---
(Reporter)

Comment 2

5 years ago
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?
Blocks: 788172
Flags: needinfo?(jorendorff)
I guess you could try replacing, "eval = Proxy(" with "eval = new Proxy(".
(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.
Flags: needinfo?(jorendorff)
(Reporter)

Comment 5

5 years ago
(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?
Flags: needinfo?(jwalden+bmo)
(Reporter)

Updated

5 years ago
Blocks: 858381
No longer blocks: 788172
Yes, that's the right regressor.
(Assignee)

Comment 7

5 years ago
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.
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 8

5 years ago
Created attachment 793156 [details] [diff] [review]
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.
Assignee: general → jwalden+bmo
Status: NEW → ASSIGNED
Attachment #793156 - Flags: review?(jorendorff)
(Assignee)

Comment 9

5 years ago
Created attachment 793207 [details] [diff] [review]
v2

Actually, we should be modifying ArraySetLength, surely.  Also added some tests, and fixed a related issue I should have noticed before.
Attachment #793156 - Attachment is obsolete: true
Attachment #793156 - Flags: review?(jorendorff)
Attachment #793207 - Flags: review?(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.
Attachment #793207 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 11

5 years ago
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.
OS: Mac OS X → All
Hardware: x86_64 → All
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 44e615a66f3f).
https://hg.mozilla.org/mozilla-central/rev/f619e14327ad
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.