Closed
Bug 731642
Opened 12 years ago
Closed 12 years ago
Assertion failure: isDenseArray(), at ../jsobjinlines.h:504
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: decoder, Assigned: jonco)
References
Details
(Keywords: assertion, testcase, Whiteboard: js-triage-done)
Attachments
(1 file, 1 obsolete file)
1.97 KB,
patch
|
gkw
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Assignee: general → jcoppeard
Assignee | ||
Comment 1•12 years ago
|
||
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 2•12 years ago
|
||
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•12 years ago
|
||
Update patch with review comments addressed.
Attachment #629371 -
Attachment is obsolete: true
Assignee | ||
Comment 4•12 years ago
|
||
Thanks for the review. I've updated the patch and attached above. Jon
Status: NEW → ASSIGNED
Comment 5•12 years ago
|
||
Comment on attachment 632223 [details] [diff] [review] Patch with review comments addressed (r+ should be brought forward)
Attachment #632223 -
Flags: review+
Updated•12 years ago
|
Keywords: checkin-needed
Comment 6•12 years ago
|
||
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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f41d4b18acaa
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•