Closed Bug 731642 Opened 8 years ago Closed 8 years ago

Assertion failure: isDenseArray(), at ../jsobjinlines.h:504

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla16

People

(Reporter: decoder, Assigned: jonco)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase, Whiteboard: js-triage-done)

Attachments

(1 file, 1 obsolete file)

The following test crashes on mozilla-central revision 2dc40eb83023 (options -m -n):


Object.prototype.__defineGetter__(1, function() {this.nameGETS++; return this._name;});
[1,,].pop();
Assignee: general → jcoppeard
Attached patch Potential fix (obsolete) — Splinter Review
The assertion happens because in array_pop_dense, GetElement() calls the getter defined on the prototype and this adds a property, causing the array to no longer be dense anymore.

The fix is to check if the array is still dense afterwards, and if not take the same actions as the slow path.
Attachment #629371 - Flags: review?(jwalden+bmo)
Comment on attachment 629371 [details] [diff] [review]
Potential fix

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

Looks good, just style nits.  Post a fixed-up patch and you should be good to go here.

::: js/src/jit-test/tests/basic/bug731642.js
@@ +1,1 @@
> +Object.prototype.__defineGetter__(1, function() {this.foo++; return 23;});

Use Object.defineProperty to add the getter here, please.  (__defineGetter__ is non-standard.  And you're not losing test coverage, because it's implemented in terms of the same underlying primitive.)

::: js/src/jsarray.cpp
@@ +2468,5 @@
>  
>      args.rval() = elt;
> +    
> +    // obj may not be a dense array any more, e.g. if the element was a missing and a
> +    // getter supplied by the prototype modified the object

Period at the end of a complete sentence.

@@ +2477,5 @@
> +        obj->setArrayLength(cx, index);
> +        return JS_TRUE;
> +    }
> +    else
> +        return js_SetLengthProperty(cx, obj, index);

In SpiderMonkey code, when you have if (...) stmt1 else stmt2, if stmt1 ends in a return statement, we don't have the else before stmt2.

Also, these should all be returning true or false, not JS_TRUE or JS_FALSE.  (We've been moving to the bool values slowly, incrementally changing as we change surrounding code usually.  You should probably fix up the rest of the method while you're here.)
Attachment #629371 - Flags: review?(jwalden+bmo) → review+
Update patch with review comments addressed.
Attachment #629371 - Attachment is obsolete: true
Thanks for the review.  I've updated the patch and attached above.

Jon
Status: NEW → ASSIGNED
Comment on attachment 632223 [details] [diff] [review]
Patch with review comments addressed

(r+ should be brought forward)
Attachment #632223 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f41d4b18acaa
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: js-triage-needed → js-triage-done
Target Milestone: --- → mozilla16
https://hg.mozilla.org/mozilla-central/rev/f41d4b18acaa
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Duplicate of this bug: 755294
You need to log in before you can comment on or make changes to this bug.