Last Comment Bug 731642 - Assertion failure: isDenseArray(), at ../jsobjinlines.h:504
: Assertion failure: isDenseArray(), at ../jsobjinlines.h:504
: assertion, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86_64 Linux
-- critical (vote)
: mozilla16
Assigned To: Jon Coppeard (:jonco)
: Jason Orendorff [:jorendorff]
: 755294 (view as bug list)
Depends on:
Blocks: langfuzz
  Show dependency treegraph
Reported: 2012-02-29 08:43 PST by Christian Holler (:decoder)
Modified: 2012-08-22 05:19 PDT (History)
6 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Potential fix (1.67 KB, patch)
2012-06-01 15:32 PDT, Jon Coppeard (:jonco)
jwalden+bmo: review+
Details | Diff | Splinter Review
Patch with review comments addressed (1.97 KB, patch)
2012-06-12 06:12 PDT, Jon Coppeard (:jonco)
gary: review+
Details | Diff | Splinter Review

Description User image Christian Holler (:decoder) 2012-02-29 08:43:49 PST
The following test crashes on mozilla-central revision 2dc40eb83023 (options -m -n):

Object.prototype.__defineGetter__(1, function() {this.nameGETS++; return this._name;});
Comment 1 User image Jon Coppeard (:jonco) 2012-06-01 15:32:56 PDT
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.
Comment 2 User image Jeff Walden [:Waldo] (remove +bmo to email) 2012-06-08 14:33:47 PDT
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() {; 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.)
Comment 3 User image Jon Coppeard (:jonco) 2012-06-12 06:12:48 PDT
Created attachment 632223 [details] [diff] [review]
Patch with review comments addressed

Update patch with review comments addressed.
Comment 4 User image Jon Coppeard (:jonco) 2012-06-12 06:13:46 PDT
Thanks for the review.  I've updated the patch and attached above.

Comment 5 User image Gary Kwong [:gkw] [:nth10sd] 2012-06-12 13:19:47 PDT
Comment on attachment 632223 [details] [diff] [review]
Patch with review comments addressed

(r+ should be brought forward)
Comment 6 User image Ryan VanderMeulen [:RyanVM] 2012-06-12 14:09:36 PDT
Comment 7 User image Ed Morley [:emorley] 2012-06-13 05:59:18 PDT
Comment 8 User image Christian Holler (:decoder) 2012-08-22 05:19:48 PDT
*** Bug 755294 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.