cpeyer points out to me that even though I asserted that we would now clamp the Array length at its maximal value and never wrap around (see Bug 658677, comment 58), the test case acceptance/spidermonkey/js1_5/Array/regress-465980-02.as is still exhibiting wrap-around to zero. (This is my mistake, I should have followed up on this after wmaddox mentioned to me on Friday that he thought we were still wrapping around in certain cases in the current code.) Need to stamp out the non-clamps!

Illustration of an issue I'm discussing with Chris.) With TR, Mac OS X, 64-bit, shell 2.1 debug-debugger build 6535:c23c8f1e0bfa Without -AS3 to asc.jar: % $AVM cpeyer.dflt.abc 4294967295 4294967295 3 3 [4,5,6,...,,,,3] [4,5,6,...,,,,3] With -AS3 to asc.jar: % $AVM cpeyer.as3.abc 4294967295 4294967295 4294967295 4294967295 [1,2,,...,,,2,6] [1,2,,...,,,2,6] [I have an easy patch that clamps the .length. My current question is whether to do anything about the fact that setting operation is overwriting the initial elements of the Array when the test is compiled without -AS3.]

(I think for consistency we'll want to get this into Serrano.)

Comment on attachment 555174 [details] [diff] [review] patch A v1: clamp length, but leave other questionable behavior alone Ugh sorry bad patch; its both out-of-date and has ugly stuff like generated code in it. Will fix.

Replacement for attachment 555174 [details] [diff] [review]. The comment in this patch mentions a problem with the clamping semantics I attempted to highlight in attachment 555164 [details]: even though the length does not wrap around anymore, the property calculation does, and so you clobber the initial elements of the array. With this patch, here's what happens now: Without -AS3 to asc.jar: % $AVM cpeyer.dflt.abc 4294967295 4294967295 4294967295 4294967295 [4,5,6,...,,,2,3] [1,2,,...,,,2,6] With -AS3 to asc.jar: % $AVM cpeyer.as3.abc 4294967295 4294967295 4294967295 4294967295 [1,2,,...,,,2,6] [1,2,,...,,,2,6] So, comparing to comment 2, the -AS3 behavior is still in sync before and after the patch, but the two kinds of pushing are exposing the wrap-around within the for-loop in Array.prototype.push. The patch discusses this and proposes three ways to fix it, which I copy here in this comment and add a fourth [as well as identifying the current behavior as the zeroth]. (0) leave as is; keep clobbering the initial array elements via wrap-around (1) throw RangeError (according to stejohns, matches ecma-262; see description of Bug 661330) (2) set non-uint properties; i.e. lose the :uint qualifier on wrap-around (3) match AS3::push behavior (i.e. repeatedly push to the clamped length) (4) ignore any arguments to push beyond those that fit in the space available. Given that Array.prototype.push is supposed to be generic, option (2) is a plausible option, and best matches my reading of ECMA-262 [I am not seeing what stejohns was referring to; it is possible he was basing his statement from Bug 661330 on the behavior he observed from running spidermonkey rather than reading the specification]. (Either way, I will hack up an alternative patch that adjusts the loop behavior in some version-checked manner as an exploration of how to go about it.)

(In reply to Felix S Klock II from comment #5) > Given that Array.prototype.push is supposed to be generic, option (2) is a > plausible option, and best matches my reading of ECMA-262 [I am not seeing > what stejohns was referring to; it is possible he was basing his statement > from Bug 661330 on the behavior he observed from running spidermonkey rather > than reading the specification]. Or of course it could be that the observed behavior described by option (1) is a consequence of following the behavior described by option (2). In particular, invoking the [[Put]] internal method can in turn invoke the [[DefineOwnProperty]] internal method, and that in turn can throw RangeError for array.

There is also length weirdness when using Array.splice. See 681511

Realized my earlier test was missing part of the point; printing the array directly won't necessarily tell you the whole story, especially if the Array prototype toString just prints up to the .length. This version of the test attempts to extract out properties from the areas under consideration, namely, indices near 0 and those near 2^31-1. I'll copy sample output in my next comment where I post an alternative to patch A for consideration.

0000000000111111111112222222222233333333333444444444445555555555566666666666777777777778 1234567889012345678890123456788901234567889012345678890123456788901234567889012345678890 This patch illustrates one way to go about implementing Option (2) as described in comment 5, without sacrificing efficiency in the common case where we do not overflow. So, what's the consequence of this change. Well, lets first establish the baseline: What does avmshell do before this patch when applied to test C (v2). (In all of the transcripts below, the file cpeyer.dflt.abc is test C compiled without -AS3, and cpeyer.as3.abc is test C compiled with -AS3. The capital 'M' in the output is meant to be a short-hand for uint.MAX_VALUE, as you can see by reading the source code to test C (cpeyer.as), v2.) Before any patch (A or B), running TR 6537:61345a99c5d8 (OS X, DebugDebugger, 64-bit): % ./objdir-ged64/shell/avmshell.r6537 ~/Dev/Bugz/bugz681399/cpeyer.dflt.abc 4294967295 4294967295 3 3 [0:4, 1:5, 2:6, 3:undefined..., M-3:undefined, M-2:undefined, M-1:undefined, M:3, M+1:undefined, M+2:undefined, M+3:undefined] [0:4, 1:5, 2:6, 3:undefined..., M-3:undefined, M-2:undefined, M-1:undefined, M:3, M+1:undefined, M+2:undefined, M+3:undefined] % ./objdir-ged64/shell/avmshell.r6537 ~/Dev/Bugz/bugz681399/cpeyer.as3.abc 4294967295 4294967295 4294967295 4294967295 [0:1, 1:2, 2:undefined, 3:undefined..., M-3:undefined, M-2:undefined, M-1:2, M:6, M+1:undefined, M+2:undefined, M+3:undefined] [0:1, 1:2, 2:undefined, 3:undefined..., M-3:undefined, M-2:undefined, M-1:2, M:6, M+1:undefined, M+2:undefined, M+3:undefined] We can see above that the length is wrapping for the non-AS3 case, and also the non-AS3 case is clobbering the elements at array indices 0,1,2 with the values [4,5,6]. The AS3 case is not clobbering the elements at 0,1,2; it is instead repeatedly writing to the value at the end of the array [even when all of the arguments are being fed to push at once, as in a.push(3,4,5,6)], reflecting the clamped nature of the .length property, which is what AS3_push happens to use as the basis for the [[Put]] in its internal loop deep in the implementation. (The M-1:2 comes from the initial write of 2 to a[i] and b[i] when i = 0xfffffffe, as one can see in the code; though I'm currently at a loss to expalin where that disappeared to in the non-AS3 output.) ---- After patch A: % ./objdir-ged64/shell/avmshell.patchA ~/Dev/Bugz/bugz681399/cpeyer.dflt.abc 4294967295 4294967295 4294967295 4294967295 [0:4, 1:5, 2:6, 3:undefined..., M-3:undefined, M-2:undefined, M-1:2, M:3, M+1:undefined, M+2:undefined, M+3:undefined] [0:1, 1:2, 2:undefined, 3:undefined..., M-3:undefined, M-2:undefined, M-1:2, M:6, M+1:undefined, M+2:undefined, M+3:undefined] % ./objdir-ged64/shell/avmshell.patchA ~/Dev/Bugz/bugz681399/cpeyer.as3.abc 4294967295 4294967295 4294967295 4294967295 [0:1, 1:2, 2:undefined, 3:undefined..., M-3:undefined, M-2:undefined, M-1:2, M:6, M+1:undefined, M+2:undefined, M+3:undefined] [0:1, 1:2, 2:undefined, 3:undefined..., M-3:undefined, M-2:undefined, M-1:2, M:6, M+1:undefined, M+2:undefined, M+3:undefined] Now we see that the that the .length is clamped to 4294967295 in both the non-AS3 and the AS3 variants. This has the side-effect in the non-AS3 code of making the repeated pushes as in b.push(3); b.push(4); b.push(5); b.push(6); repeatedly clobber the same spot at the end of the array. However, the non-AS3 code is still clobbering the elements at a[0], a[1], a[2] when all the push arguments are fed at once. The AS3 behavior is unchanged. ---- After patch B, supplying swf version 13: % ./objdir-ged64/shell/avmshell.patchB -swfversion 13 \ ~/Dev/Bugz/bugz681399/cpeyer.dflt.abc 4294967295 4294967295 4294967295 4294967295 [0:4, 1:5, 2:6, 3:undefined..., M-3:undefined, M-2:undefined, M-1:2, M:3, M+1:undefined, M+2:undefined, M+3:undefined] [0:1, 1:2, 2:undefined, 3:undefined..., M-3:undefined, M-2:undefined, M-1:2, M:6, M+1:undefined, M+2:undefined, M+3:undefined] % ./objdir-ged64/shell/avmshell.patchB -swfversion 13 \ ~/Dev/Bugz/bugz681399/cpeyer.as3.abc 4294967295 4294967295 4294967295 4294967295 [0:1, 1:2, 2:undefined, 3:undefined..., M-3:undefined, M-2:undefined, M-1:2, M:6, M+1:undefined, M+2:undefined, M+3:undefined] [0:1, 1:2, 2:undefined, 3:undefined..., M-3:undefined, M-2:undefined, M-1:2, M:6, M+1:undefined, M+2:undefined, M+3:undefined] This transcript matches patch A above: the .length as been clamped (regardless of the version number, since that's the policy we decided on in the low-level runtime code). ---- After patch B, supplying swf version 14: % ./objdir-ged64/shell/avmshell.patchB -swfversion 14 \ ~/Dev/Bugz/bugz681399/cpeyer.dflt.abc 4294967295 4294967295 4294967295 4294967295 [0:1, 1:2, 2:undefined, 3:undefined..., M-3:undefined, M-2:undefined, M-1:2, M:3, M+1:4, M+2:5, M+3:6] [0:1, 1:2, 2:undefined, 3:undefined..., M-3:undefined, M-2:undefined, M-1:2, M:6, M+1:undefined, M+2:undefined, M+3:undefined] % ./objdir-ged64/shell/avmshell.patchB -swfversion 14 \ ~/Dev/Bugz/bugz681399/cpeyer.as3.abc 4294967295 4294967295 4294967295 4294967295 [0:1, 1:2, 2:undefined, 3:undefined..., M-3:undefined, M-2:undefined, M-1:2, M:6, M+1:undefined, M+2:undefined, M+3:undefined] [0:1, 1:2, 2:undefined, 3:undefined..., M-3:undefined, M-2:undefined, M-1:2, M:6, M+1:undefined, M+2:undefined, M+3:undefined] Here, we finally see the point of patch B: when running in non-AS3 mode, the act of doing a.push(3,4,5,6) when the length is M-1 will put the four elements at M, M+1, M+2, and M+3. (Another option would be to throw a RangeError. That is easily implemented. But before doing that, I'd want to double-check how Firefox behaves if you move the Array.prototype.push function onto a non-array object, as an experiment to test the hypothesis I put forth in comment 6.)

(note that patch A and patch B are alternatives; you cannot directly apply patch B atop patch A. Arguably I should not have selected adjacent letters to identify them.)

To be clear, I am only suggesting submitting patch A for Serrano; the motivation is for consistency with the other places where the .length property of Arrays is going to be clamped. Patch B is just me thinking out loud; changes like the ones it described would not be targeted for Serrano, but rather for Anza or perhaps beyond.

Comment on attachment 555262 [details] [diff] [review] patch B v1: clamp .length unconditionally; versioned [[Put]] variation You should move this statement into the "else" branch: const bug681399:Boolean = bugzilla(681399); because you don't need it until then. Keep the common case as simple as possible. n_orig seems redundant here, lose it. args.length is a uint, no cast is needed (no cast would be needed even if it were not guaranteed to be uint). I'm far from certain that the bugfixed code is correct/desirable, is this compatible with the browsers?

Comment on attachment 555184 [details] [diff] [review] patch A v2: clamp length; leave other questionable behavior alone The cast on args.length is redundant and should be removed.

(In reply to Lars T Hansen from comment #12) > I'm far from certain that the bugfixed code is correct/desirable, is this > compatible with the browsers? I've added sample html (attachment 555428 [details], aka html P) to explore behavior here. I was careful to attempt to include a test where the push method has been attached to a non-Array object. I've done my survey on Safari, Chrome, and Firefox. I will post the cut-and-pasted transcripts shortly, but here is the executive summary: Chrome and Firefox seem to follow option (2) from comment 5, setting non-uint properties, and then both throw a RangeError on the *length* modification (not on the calls to [[Put]] as I had hypothesized in comment 6). Safari wraps around and clobbers the initial array/object elements (and wraps the length around as well). [[I'll ask someone in #tamarin to do the same survey on IE.]]

(i've confimed that this behaves the same as html P v1 (attachment 555428 [details]) on chrome/firefox/safari. Hopefully it will get a bit farther on MS IE than html P does.)

Figured out one reason why tLt (from #tamarin) was seeing errors: apparently Array.prototype.map is too new for at least some versions of IE. This modification of the code should remain faithful to the test (since we are just trying to test an edge case of push here, not the boundaries of the spec for map). Will post results from IE v8 shortly. Executive summary: it looks to me like it matches Chrome/Firefox on this point.

results for Internet Explorer v8 running html Q v2 (attachment 555495 [details]).

It looks to me like the survey results from Chrome, FF5, and IE8 all support the semantics put forth in patch B. (A separate issue will be to make attempts to set an Array length to a non-uint throw a RangeError. But that is independent of changing Array.prototype.push to keep incrementing the numbers, without wrapping mod 2^32.)

Addressed Lars' comment about uint(args.length). Left the uint(this.length) cases alone. Three and a half notes: 1. I filed follow-on ticket, Bug 681803, to address the wrap-around question/issue. 2. Patch A is meant to be a minimal fix for Serrano, just to get to have a semi-consistent story with respect to our handling of Array .length and wrap-around (or rather, lack-thereof). 3i. We could also get rid of the .length assignment entirely. This would not be ecma-262 compliant, but neither is what we currently have, AFAICT. 3ii. Or we could assign the *non-uint-wrapped* sum to this.length this.length = Number(n_orig) + Number(args.length) Which might be the most ecma compliant option of all, in terms of how the generic push function would behave when transferred to a non-array object. But both 3i and 3ii are more severe changes than I would want to adopt for Serrano. for the short term, I just want to get feedback on what is suggested here in patch A.

Comment on attachment 555262 [details] [diff] [review] patch B v1: clamp .length unconditionally; versioned [[Put]] variation (Removing F? requests; this patch will migrate to Bug 681803 and I want reviewer attention focused on patch A, v3 for short term.)

Comment on attachment 555579 [details] [diff] [review] patch A v3: clamp length; leave other questionable behavior alone (In reply to Felix S Klock II from comment #23) > > 3i. We could also get rid of the .length assignment entirely. This would > not be ecma-262 compliant, but neither is what we currently have, AFAICT. You can't actually get rid of the .length assignment, because only for Array data will the length be updated as you push elements with the assignment in the loop over the arguments to push; for the function to remain "generic" you must update length. > 3ii. Or we could assign the *non-uint-wrapped* sum to this.length > this.length = Number(n_orig) + Number(args.length) > Which might be the most ecma compliant option of all, in terms of how > the generic push function would behave when transferred to a non-array > object. Yes, I believe that would be more appropriate. > But both 3i and 3ii are more severe changes than I would want to adopt for > Serrano. for the short term, I just want to get feedback on what is > suggested here in patch A. I think it's a mistake to ship something that is not what we had and we know is not what we want, but it's Dan's decision that we do so. Given that, this patch seems adequate.

(In reply to Lars T Hansen from comment #25) > > > 3ii. Or we could assign the *non-uint-wrapped* sum to this.length > > this.length = Number(n_orig) + Number(args.length) > > Which might be the most ecma compliant option of all, in terms of how > > the generic push function would behave when transferred to a non-array > > object. > > Yes, I believe that would be more appropriate. But the act of assigning an overflowing Number will overwrite the "correct" clamped value with an "uncorrect" wrapped one, in our current .length implementation for Array. So doing this may solve a different problem, but it won't solve the immediate one on its own. :( > > But both 3i and 3ii are more severe changes than I would want to adopt for > > Serrano. for the short term, I just want to get feedback on what is > > suggested here in patch A. > > I think it's a mistake to ship something that is not what we had and we know > is not what we want, but it's Dan's decision that we do so. Given that, > this patch seems adequate. I'll look into what our options are. If I can quickly figure out how to make the assignment (3ii) above throw a RangeError, I'd be happier, but i don't know if that's possible without changing the type signature for the length getter/setter in Array.as ...

(In reply to Felix S Klock II from comment #26) > (In reply to Lars T Hansen from comment #25) > > > > > 3ii. Or we could assign the *non-uint-wrapped* sum to this.length > > > this.length = Number(n_orig) + Number(args.length) > > > Which might be the most ecma compliant option of all, in terms of how > > > the generic push function would behave when transferred to a non-array > > > object. > > > > Yes, I believe that would be more appropriate. > > But the act of assigning an overflowing Number will overwrite the "correct" > clamped value with an "uncorrect" wrapped one, in our current .length > implementation for Array. Ah, you're right. Guarding with a (this is Array) would be a good start.

Lars: here is another option for patch A. I could be convinced to land this instead of patch A v3 (attachment 555579 [details] [diff] [review]). (I freely admit: the distinction between the two patches seems a bit silly in the absence of a fix Bug 681803. But I really don't want to try to push that into Serrano. Nonetheless, I can be convinced that this patch, A' v4, is the appropriate fix under the assumption that we will be fixing 681803 soon-ish.)

(In reply to Felix S Klock II from comment #28) > (I freely admit: the distinction between the two patches seems a bit silly > in the absence of a fix Bug 681803.) Of course, there are _two_ distinctions of interest between the patches: The main one is that patch A' writes Number(n_orig)+Number(argc) to the length for non-arrays, as encouraged in comment 25 and comment 27. The other is that patch A' doesn't write anything to the length for arrays. As far as I can tell, this is sound, but I am not an expert on the semantics of the prototype model; is it possible for an object O to say true when asked "(O is Array)" but for O to *lack* the .length auto-update semantics? (My impression is "no", but if there are any doubts, I will happily put in the clamping code again.)

Comment on attachment 555579 [details] [diff] [review] patch A v3: clamp length; leave other questionable behavior alone (removing review requests; we're not going to do a half-baked fix; we'll save a fully-baked one for later.)

Comment on attachment 556091 [details] [diff] [review] patch A' v4: a variant on patch A that allows overflow on non-Arrays (removing review requests; we're not going to do a half-baked fix; we'll save a fully-baked one for later.)

(In reply to Felix S Klock II from comment #29) > > The other is that patch A' doesn't write anything to the length for arrays. > As far as I can tell, this is sound, but I am not an expert on the semantics > of the prototype model; is it possible for an object O to say true when > asked "(O is Array)" but for O to *lack* the .length auto-update semantics? (Preliminary comments, I need to ponder this more carefully.) I'm not sure it's sound. Consider this: Array is not final, and its "length" getter and setter are also not final. (That Array is not final is probably a bug, but it's hard to change now.) Ergo a subclass of Array can override those methods. Not writing "length" a final time at the end of push arguably violates the contract (as set out in E262), since it would be observable that length is not written n+1 times for a push of n elements. Another aspect is that since length is a getter/setter pair there's no compulsion for the getter to return the value passed to the setter, but that only matters to us if we read the length on each iteration and do not keep a local variable about where to store a value next.

changeset: 6548:62b201951d5e user: Chris Peyer <cpeyer@adobe.com> summary: Bug 681399: Update array length wraparound testcase to expect clampped behavior. This currently still fails when compiled with -ES, but succeeds when compiled with -AS3. Setting expectedfail in failconfig for now since this test always runs in -ES mode. Bug 681399 will provide a wraparound fix for -ES mode at some point in the future. (r=fklockii) http://hg.mozilla.org/tamarin-redux/rev/62b201951d5e

Pushed updated testcase to redux: http://hg.mozilla.org/tamarin-redux/rev/62b201951d5e Note that this also sets the test to expectedfail in failconfig.txt since the test now expects clamped behavoir. Once a fix has been submitted, the line will need to be removed from failconfig. Also note: I've pushed this version of the test to the ATS, but compiled with -AS3 so that the test passes there.

changeset: 6549:2be48ac29353 user: Chris Peyer <cpeyer@adobe.com> summary: Bug 681399 followup - ignore expectedfail when running differential tests (r=dschaffe) http://hg.mozilla.org/tamarin-redux/rev/2be48ac29353

(In reply to Felix S Klock II from comment #31) > Comment on attachment 556091 [details] [diff] [review] > patch A' v4: a variant on patch A that allows overflow on non-Arrays > > (removing review requests; we're not going to do a half-baked fix; we'll > save a fully-baked one for later.) Fully baked fixes have been filed as separate bugs: - Bug 661330 - Bug 681803 - Bug 685323

(In reply to Felix S Klock II from comment #36) > Fully baked fixes have been filed as separate bugs: > - Bug 661330 > - Bug 681803 > - Bug 685323 Potentially related: Bug 681511 (I don't know yet whether the latter is AS3 or non-AS3 or both. And anyway that investigation should be performed on that ticket; it doesn't change the fact that this bug is itself closed.)