Closed
Bug 681803
Opened 13 years ago
Closed 6 years ago
non-AS3 Array.prototype.push property modification loop wraps around
Categories
(Tamarin Graveyard :: Library, defect)
Tamarin Graveyard
Library
Tracking
(Not tracked)
RESOLVED
WONTFIX
Future
People
(Reporter: pnkfelix, Unassigned)
References
Details
(Whiteboard: has-patch)
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
lhansen
:
review+
edwsmith
:
superreview+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
Followup work from Bug 681399 (and mentioned in passing in Bug 661330).
Right now our Array.prototype.push doing arithmetic mod 2^32.
This has consequences which we probably do not intend. Compliance with ECMA-262 and with the majority of web browsers entails not using modular arithmetic in the overflowing cases.
See:
Bug 681399, comment 5
Bug 681399, comment 15
Bug 681399, comment 22
Reporter | ||
Comment 1•13 years ago
|
||
This patch assumes that one has already pushed attachment 556063 [details] [diff] [review] from Bug 661330.
(It is not strictly necessary to fix Bug 661330 in order to fix this bug; it just seemed like the natural way to get the ECMA-262 behavior. Its easy to remove the !bug661330 conjunct, and then we'd just avoid throwing the RangeError.)
Attachment #556065 -
Flags: superreview?(lhansen)
Attachment #556065 -
Flags: review?(wmaddox)
Reporter | ||
Updated•13 years ago
|
Reporter | ||
Comment 2•13 years ago
|
||
(In reply to Felix S Klock II from comment #1)
> Created attachment 556065 [details] [diff] [review]
> patch B v2: versioned [[Put]] variation in non-AS3 .push
(oh, its v2 because its a refinement of patch B v1, attachment 555262 [details] [diff] [review] on Bugzilla 681399.)
Reporter | ||
Comment 3•13 years ago
|
||
(miniscule fix to mistyped BZ number in a comment.)
Attachment #556065 -
Attachment is obsolete: true
Attachment #556065 -
Flags: superreview?(lhansen)
Attachment #556065 -
Flags: review?(wmaddox)
Attachment #556067 -
Flags: superreview?(lhansen)
Attachment #556067 -
Flags: review?(wmaddox)
Reporter | ||
Comment 4•13 years ago
|
||
(In reply to Felix S Klock II from comment #3)
> Created attachment 556067 [details] [diff] [review]
> patch B v3: versioned [[Put]] variation in non-AS3 .push
>
> (miniscule fix to mistyped BZ number in a comment.)
(I probably need to change the return value of push in the overflow case as well. I'm not going to update the patch for this immediately; I just wanted to note it ahead of any review.)
Comment 5•13 years ago
|
||
Comment on attachment 556067 [details] [diff] [review]
patch B v3: versioned [[Put]] variation in non-AS3 .push
Looks OK and is, from what I understand, in line with commonly observed behavior in most browsers.
The casts in this line are redundant:
const overflowed_length:Number = Number(n) + Number(argc);
Reason: There are no int or uint values in the language, just Number values (int and uint are predicates on locations). Once n and argc have been read from their variables, the resulting values are numbers.
The only nagging issue is that Array can be subclassed and the length setter can be overridden in the subclass. This code will hit the "this is Array" case for a subclassed vector and the overridden setter will therefore observe a clamped length rather than one that's wrapped around. I think that's OK.
Attachment #556067 -
Flags: superreview?(lhansen) → superreview+
Reporter | ||
Comment 6•13 years ago
|
||
An odd detail: This test:
function WithLength(l) { this.length = l; }
var a = new WithLength(uint.MAX_VALUE - 2);
Array.prototype.public::push.call(a,1,2,3,4,5);
print(a.length);
currently prints 2 on the following hosts:
32-bit release interpreter (-Dinterp)
32-bit release jit (-Ojit)
64-bit release jit (-Ojit)
but on a 64-bit interpreter, it prints 4294967298.
(Thats not even a uint!)
It might be worth checking if this is a jit/interpreter discrepancy that is inadvertently exposed here...
Comment 7•13 years ago
|
||
We might want to keep bug #601426 in mind here.
Reporter | ||
Comment 8•13 years ago
|
||
Addressed feedback from comment 5.
This introduces a dependence on Bug 661330, so I'll mark that bug as blocking this one.
Also, there's a follow-on patch that fixes the length_mods.as test case that was posted to Bug 661330; I'll be posting that here next.
I didn't version-check the assignment of the overflowed-length to a non-array, but if anyone wants me to, I will. Its observable when you apply this generic function to a non-array object that has a length property of type *. (If we do version it, the question will then become whether to assign a clamped length or a wrapped one. It seems simpler to just continue down the path we've already chosen of "just fix it" for these oddball length cases.)
Attachment #556067 -
Attachment is obsolete: true
Attachment #556067 -
Flags: review?(wmaddox)
Reporter | ||
Comment 10•13 years ago
|
||
Comment on attachment 559450 [details] [diff] [review]
patch B v4: versioned [[Put]] variation with guarded set_length call
(Ed: feel free to reassign yourself as SR? and put someone else as R?.)
This is meant to be applied atop Bug 661330's attachment 559447 [details] [diff] [review].
Attachment #559450 -
Flags: review?(edwsmith)
Reporter | ||
Comment 11•13 years ago
|
||
Comment on attachment 559452 [details] [diff] [review]
patch T v1: fixes test from Bug 661330.
(This is meant to be applied atop Bug 661330's attachment 559294 [details] [diff] [review])
This test specifically only deals with .length modification. It is not checking what the state of the push receiver is otherwise.
For Bug 681803, we need a separate test that inspects how the property modification loop in push is dealing with the overflow cases (because ECMA-262 says that the overflowed indices get written without wrapping around, writing properties with keys that are not array indices). That would be a good follow-on task for QE.
Attachment #559452 -
Flags: review?(dschaffe)
Reporter | ||
Updated•13 years ago
|
Assignee: nobody → fklockii
Whiteboard: has-patch
Reporter | ||
Updated•13 years ago
|
Status: NEW → ASSIGNED
Reporter | ||
Comment 12•13 years ago
|
||
Comment on attachment 559450 [details] [diff] [review]
patch B v4: versioned [[Put]] variation with guarded set_length call
(revising reviewers based on advice from dan. as stated above, probably best to review in parallel with patch from Bug 661330.)
Attachment #559450 -
Flags: superreview?(edwsmith)
Attachment #559450 -
Flags: review?(lhansen)
Attachment #559450 -
Flags: review?(edwsmith)
Updated•13 years ago
|
Attachment #559450 -
Flags: review?(lhansen) → review+
Reporter | ||
Comment 13•13 years ago
|
||
Comment on attachment 559452 [details] [diff] [review]
patch T v1: fixes test from Bug 661330.
(retracting review request for this revision of the test; it doesn't pass and so I think I messed something up; probably minor but no reason to waste time reviewing broken code.)
Attachment #559452 -
Flags: review?(dschaffe)
Reporter | ||
Updated•13 years ago
|
Blocks: ecma-incompatibility
Comment 14•13 years ago
|
||
changeset: 6709:ac91969bcc46
user: Felix S Klock II <fklockii@adobe.com>
summary: Bug 661330: acceptance tests for modifying Array .length property (r=dschaffe).
http://hg.mozilla.org/tamarin-redux/rev/ac91969bcc46
Reporter | ||
Updated•13 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 15•13 years ago
|
||
(resolved wrong bug)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter | ||
Updated•13 years ago
|
Blocks: array-correctness
Updated•13 years ago
|
Attachment #559450 -
Flags: superreview?(edwsmith) → superreview+
Reporter | ||
Comment 16•12 years ago
|
||
This is too risky for me to push today. But someone should pick up the work here, the patch has already gotten some review, and we certainly don't want to put the bogus semantics into AS4.
Assignee: fklockii → nobody
Target Milestone: Q1 12 - Brannan → Future
Comment 17•6 years ago
|
||
Tamarin is a dead project now. Mass WONTFIX.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 6 years ago
Resolution: --- → WONTFIX
Comment 18•6 years ago
|
||
Tamarin isn't maintained anymore. WONTFIX remaining bugs.
You need to log in
before you can comment on or make changes to this bug.
Description
•