Closed Bug 618350 Opened 10 years ago Closed 10 years ago

TM calls setter for array literals

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: bhackett1024)

References

Details

(Keywords: regression, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file)

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?
Blocks: es5
This was probably regressed by bug 606477.
Assignee: general → bhackett1024
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!
Blocks: 606477
Keywords: regression
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.
Attached patch fixSplinter Review
Make array_defineProperty not call array_setProperty.
Attachment #496968 - Flags: review?(jwalden+bmo)
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.
Attachment #496968 - Flags: review?(jwalden+bmo) → review+
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?
Duplicate of this bug: 618150
(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.
http://hg.mozilla.org/tracemonkey/rev/41ff7f6d02d9
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/41ff7f6d02d9
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.