Closed
Bug 618350
Opened 10 years ago
Closed 10 years ago
TM calls setter for array literals
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jandem, Assigned: bhackett1024)
References
Details
(Keywords: regression, Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file)
2.56 KB,
patch
|
Waldo
:
review+
|
Details | Diff | Splinter Review |
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?
TM is incorrect.
Assignee | ||
Comment 2•10 years ago
|
||
This was probably regressed by bug 606477.
Assignee: general → bhackett1024
Comment 3•10 years ago
|
||
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!
Updated•10 years ago
|
Keywords: regression
Assignee | ||
Comment 4•10 years ago
|
||
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.
Assignee | ||
Comment 5•10 years ago
|
||
Make array_defineProperty not call array_setProperty.
Attachment #496968 -
Flags: review?(jwalden+bmo)
Comment 6•10 years ago
|
||
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+
Comment 7•10 years ago
|
||
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?
Assignee | ||
Comment 9•10 years ago
|
||
(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.
Assignee | ||
Comment 10•10 years ago
|
||
http://hg.mozilla.org/tracemonkey/rev/41ff7f6d02d9
Whiteboard: fixed-in-tracemonkey
Comment 11•10 years ago
|
||
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.
Description
•