618350 - TM calls setter for array literals

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Consider this test case:
---
var a = 0;
Object.prototype.__defineSetter__(1, function() { a++; });

for (var x = 0; x < 20; ++x) {
    [1, 2];
}
assertEq(a, 0);
---
This fails with -j:
test.js:7: Error: Assertion failed: got 12, expected 0 

(note that 20 - HOTLOOP == 12)

Or is TM correct and interpreter/JM wrong?

Comment 1

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

TM is incorrect.

Comment 2

[Brian Hackett [Laid off!]]

This was probably regressed by bug 606477.

Comment 3

[Jeff Walden [:Waldo]]

I could have sworn we had tests for this, but it looks like I was just a bad person who didn't write tests when fixing bug 474501.  Someone castigate me harshly!

Comment 4

[Brian Hackett [Laid off!]]

Actually, this bug predates bug 606477 (and bug 594817 for that matter).  The problem is that the defineProperty hook for dense arrays calls setProperty after slowifying due to indexed properties in a prototype.  It shouldn't be checking that flag and slowifying in the first place, and it shouldn't be calling setProperty.  The interpreter only works on this example because at the second INITELEM the array has already been slowified, and slow arrays don't have a defineProperty hook.

This example prints 20 in the interpreter (and -j), in rev 87e44effa997 (before 584917).

var a = 0;
Object.prototype.__defineSetter__(0, function() { a++; });

for (var x = 0; x < 20; ++x)
  [1,2];
print(a);

Will work on a fix.

Comment 5

[Brian Hackett [Laid off!]]

Attachment: fix

Make array_defineProperty not call array_setProperty.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 496968 [details] [diff] [review]
fix

>diff --git a/js/src/jsarray.cpp b/js/src/jsarray.cpp

>+    do {
>+        uint32 i = 0;       // init to shut GCC up
>+        JSBool isIndex;
>+
>+        isIndex = js_IdIsIndex(id, &i);

Delay declaration as long as possible, and unify declarations with initializations when it makes sense, as here.

Comment 7

[Jeff Walden [:Waldo]]

Pretty sure this is a dup of bug 618150 (or rather, other way around, since this bug has a patch) -- mind checking and adding a test in that bug's vein when landing this if so?

Comment 9

[Brian Hackett [Laid off!]]

(In reply to comment #7)
> Pretty sure this is a dup of bug 618150 (or rather, other way around, since
> this bug has a patch) -- mind checking and adding a test in that bug's vein
> when landing this if so?

Yes, this is right.  I'll add a test.

Comment 10

[Brian Hackett [Laid off!]]

http://hg.mozilla.org/tracemonkey/rev/41ff7f6d02d9

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/41ff7f6d02d9