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

RESOLVED FIXED in mozilla16

Status

()

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

People

(Reporter: decoder, Assigned: jonco)

Tracking

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

Trunk
mozilla16
x86_64
Linux
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: js-triage-done)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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)

Updated

5 years ago
Assignee: general → jcoppeard
(Assignee)

Comment 1

5 years ago
Created attachment 629371 [details] [diff] [review]
Potential fix

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+
(Assignee)

Comment 3

5 years ago
Created attachment 632223 [details] [diff] [review]
Patch with review comments addressed

Update patch with review comments addressed.
Attachment #629371 - Attachment is obsolete: true
(Assignee)

Comment 4

5 years ago
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+
Keywords: checkin-needed
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

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/f41d4b18acaa
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Updated

5 years ago
Duplicate of this bug: 755294
You need to log in before you can comment on or make changes to this bug.