661330 - Array.length behavior doesn't follow ECMA262 near 2^32-1

Status: RESOLVED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Steven Johnson]

(See Bug 658677 for history)

ECMA262 requires that using index=2^32-1 not affect Array.length; different code paths in our Array handle this differently (most code paths wrap to zero, but some leave it unaffected). Furthermore, the behavior differs subtly between interp/jit modes, and 32/64 bit builds.

Also, ECMA262 specifies that RangeError be thrown in other situations (eg calling push() on an Array with length=2^32-1), which we have never done.

IMHO, existing behavior should be preserved as closely as possible (though we may have to exterminate interp/jit, 32/64 differences), but a fully-conforming Array implementation should be added as a future versioned fix.

Comment 1

[Chris Peyer]

Also note that the behavior is different depending on whether or not it is compiled with the -AS3 asc.jar swtich.

Comment 3

[Felix S. Klock II [:pnkfelix, :fklock]]

More careful reading of this bug's description implies that Bug 677923 does not subsume it.

Bug 677923 was largely dedicated to identifying the different axes on which inconsistencies could arise.  This bug (Bug 661330) says it is dedicated to implementing a versioned, ECMA262-conforming Array implementation.

They certainly seem related though; I'll mark Bug 677923 as blocking 661330.

Comment 4

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v1: guard assignment to Array length, non-AS3 mode

I got sucked into fixing this in order to address Bug 681399, comment 27 in a semi-disciplined manner.

Comment 5

[Lars T Hansen]

Comment on attachment 556063 [details] [diff] [review]
patch A v1: guard assignment to Array length, non-AS3 mode

Seems reasonable enough.

Comment 6

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Lars T Hansen from comment #5)
> Comment on attachment 556063 [details] [diff] [review]
> patch A v1: guard assignment to Array length, non-AS3 mode
> 
> Seems reasonable enough.

Now that you've brought up subclassing Arrays (in Bugzilla 681399, comment 32, and Bugzilla 681803, comment 5), I am worried about changing the type signature of the length setter in Array.as...

Notably here:

         // E262 {DontEnum, DontDelete}
         public native function get length():uint
-        public native function set length(newLength:uint)
+        private native function set_uint_length(newLength:uint):void
+        public function set length(newLength:*) {
...

Its necessary to expand the domain of 'set length' if we're going to throw a RangeError on a non-uint, but I don't yet know how that interacts with subclasses that were previously overriding 'set length'

Will investigate.

Comment 7

[Lars T Hansen]

(In reply to Felix S Klock II from comment #6)

> Now that you've brought up subclassing Arrays (in Bugzilla 681399, comment
> 32, and Bugzilla 681803, comment 5), I am worried about changing the type
> signature of the length setter in Array.as...

Gosh, I completely missed that.  An overriding subclass that uses 'uint' in the signature will fail to compile in strict mode, and will probably fail to verify if compiled in standard mode.

Comment 8

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Lars T Hansen from comment #7)
> Gosh, I completely missed that.  An overriding subclass that uses 'uint' in
> the signature will fail to compile in strict mode, and will probably fail to
> verify if compiled in standard mode.

That means we can't land this, unless we can version it (and even then we might not be able to land it), right?

If we are restricted to uint for the length setter, then I don't know what is best for the generic Array operations to do.  I don't want to write a value that is going to be silently wrapped mod 2^32.  I guess I could bubble up the throwing of RangeError to all the sites that would write to .length when (this is Array)...

Comment 9

[Lars T Hansen]

(In reply to Felix S Klock II from comment #8)
> (In reply to Lars T Hansen from comment #7)
> > Gosh, I completely missed that.  An overriding subclass that uses 'uint' in
> > the signature will fail to compile in strict mode, and will probably fail to
> > verify if compiled in standard mode.
> 
> That means we can't land this, unless we can version it (and even then we
> might not be able to land it), right?

The current versioning system does not allow for variant interfaces; it only allows us to add new classes and add methods to existing classes (to the best of my knowledge).  Thus non-final signatures, once introduced, pretty much have to stay that way forever.

I think that a versioning system that allows variant definitions of types and methods will eventually be necessary.

> If we are restricted to uint for the length setter, then I don't know what
> is best for the generic Array operations to do.  I don't want to write a
> value that is going to be silently wrapped mod 2^32.  I guess I could bubble
> up the throwing of RangeError to all the sites that would write to .length
> when (this is Array)...

Yeah.  Sucks to be us.  The simpler alternative might be to have a private length-setter method that does not take uint, and have all the builtin methods call that, ie, the opposite of what you just tried.  After all a setter is just a method, nothing magic about it; the value to be set can be set by other means too.  (Even if it's simpler it's annoyingly complex and brittle.)

Comment 10

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Lars T Hansen from comment #7)
> An overriding subclass that uses 'uint' in
> the signature will fail to compile in strict mode, and will probably fail to
> verify if compiled in standard mode.

From my attempts to check this statement, it looks to me like:

- As expected, you get "Incompatible override" failures at compile-time in strict mode,

- You also get the same failures at compile-time when in non-strict mode,

- If you attempt to run an .abc compiled with the previous builtins, *then* you get a verification failure at run-time.

(In reply to Lars T Hansen from comment #9)
> (In reply to Felix S Klock II from comment #8)
> > (In reply to Lars T Hansen from comment #7)
> > > Gosh, I completely missed that.  An overriding subclass that uses 'uint' in
> > > the signature will fail to compile in strict mode, and will probably fail to
> > > verify if compiled in standard mode.
> > 
> > That means we can't land this, unless we can version it (and even then we
> > might not be able to land it), right?
> 
> The current versioning system does not allow for variant interfaces; it only
> allows us to add new classes and add methods to existing classes (to the
> best of my knowledge).  Thus non-final signatures, once introduced, pretty
> much have to stay that way forever.
> 
> I think that a versioning system that allows variant definitions of types
> and methods will eventually be necessary.

Okay, I won't bother trying to see if I introduce a verision variant interface here then.

> > If we are restricted to uint for the length setter, then I don't know what
> > is best for the generic Array operations to do.  I don't want to write a
> > value that is going to be silently wrapped mod 2^32.  I guess I could bubble
> > up the throwing of RangeError to all the sites that would write to .length
> > when (this is Array)...
> 
> Yeah.  Sucks to be us.  The simpler alternative might be to have a private
> length-setter method that does not take uint, and have all the builtin
> methods call that, ie, the opposite of what you just tried.  After all a
> setter is just a method, nothing magic about it; the value to be set can be
> set by other means too.  (Even if it's simpler it's annoyingly complex and
> brittle.)

Okay.  The calls would still need to be special-cased on (this is Array), but that sounds preferable to putting the uint-or-RangeError checks on all the call sites.

Comment 11

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 556063 [details] [diff] [review]
patch A v1: guard assignment to Array length, non-AS3 mode

(retracting review request; see comment 6 and comment 7 for why.  I'll work on something following the outline of comment 8 and comment 9.)

Comment 12

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 556063 [details] [diff] [review]
patch A v1: guard assignment to Array length, non-AS3 mode

(removing the SR+ so that no one gets fooled into thinking that this patch is okay as is.)

Comment 13

[Felix S. Klock II [:pnkfelix, :fklock]]

(Filed Bug 685220 as an obvious sub-task that comment 7 is screaming out for.)

Comment 14

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #10)
> (In reply to Lars T Hansen from comment #7)

[...]

> > > If we are restricted to uint for the length setter, then I don't know what
> > > is best for the generic Array operations to do.  I don't want to write a
> > > value that is going to be silently wrapped mod 2^32.  I guess I could bubble
> > > up the throwing of RangeError to all the sites that would write to .length
> > > when (this is Array)...
> > 
> > Yeah.  Sucks to be us.  The simpler alternative might be to have a private
> > length-setter method that does not take uint, and have all the builtin
> > methods call that, ie, the opposite of what you just tried.  After all a
> > setter is just a method, nothing magic about it; the value to be set can be
> > set by other means too.  (Even if it's simpler it's annoyingly complex and
> > brittle.)
> 
> Okay.  The calls would still need to be special-cased on (this is Array),
> but that sounds preferable to putting the uint-or-RangeError checks on all
> the call sites.

The call sites in question are also the subject of Bug 681803 (for push) and Bug 685323 (for unshift); arguably all of these problems should just get fixed in one swoop through the code.

Comment 15

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: Acceptance test for length wraparound

After thinking further about this bug, I think there are some semi-clean approaches we can use to get ES5 semantics.  but before I put them in, I wanted to make sure that I had a reasonable test suite to cover the different pathological cases where wrap-around can occur.

(Some cases are explicitly skipped because they consume so much time/memory that they are unlikely to occur in practice.  See comments in the code.)

Comment 16

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #15)
> After thinking further about this bug, I think there are some semi-clean
> approaches we can use to get ES5 semantics

The thinking I was employing above was flawed.  I had thought that I might be able to catch the coercion from Atom to uint within AVM, and throw the RangeError from there (in a version-checked manner).  (I was willing to accept that I would only be able to perform such interception for a subset of the array length assignment forms, e.g. forms like ``arr["length"] = newlen'' or similar.)

But this thinking, as I said, was flawed.  The place where the coercion from an Atom to a uint happens is in the nativegen.py generated code for avmplus::NativeID::Array_length_set_thunk.  I don't want to mess with nativegen.py.

So I'm going back to decorating the call-sites, as described in comment 8 and comment 9 and comment 10.

Comment 17

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: test L v2: acceptance test for length wraparound

(realized while working on the patch that I should cover subclasses of Array too.)

Comment 18

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Felix S Klock II from comment #10)
> (In reply to Lars T Hansen from comment #7)
> > Yeah.  Sucks to be us.  The simpler alternative might be to have a private
> > length-setter method that does not take uint, and have all the builtin
> > methods call that, ie, the opposite of what you just tried.  After all a
> > setter is just a method, nothing magic about it; the value to be set can be
> > set by other means too.  (Even if it's simpler it's annoyingly complex and
> > brittle.)
> 
> Okay.  The calls would still need to be special-cased on (this is Array),
> but that sounds preferable to putting the uint-or-RangeError checks on all
> the call sites.

Another note: This would only "fix" Array (by special-casing on it).  It would not fix cases where the method is being generically applied to instances of classes that define their own var length:uint or likewise uint-typed length setter.

(Examples of such classes include Vector and ByteArray.)

I'm willing to live with that.

Besides, there's really not any way to deal with it, apart from either:
  1. changing the semantics of the implicit coercion that occurs when you assign a non-uint to a uint-typed location, or
  2. extending the ActionScript language with some way for a program to introspectively detect the type of a location, so that it could avoid the implicit coercion.  (Maybe this is already doable, in an expensive fashion, via describeType.)

I'm assuming that (1.) is a non-starter and (2.) would have to wait until there was actual customer demand for such a capability.

Comment 19

[Felix S. Klock II [:pnkfelix, :fklock]]

Attachment: patch A v2: add private set_length with versioned exception

The private set_length function added here can only be used for Arrays; so callers from generic routines like Array.prototype.public::push need to first check that the receiver is an Array before invoking private::set_length on it.

The behavior is versioned checked and its up to the caller to provide an appropriate value for when the newLength is not a uint.  (This seemed like the cleanest way to go, rather than assuming that all failing calls would be cases where uint.MAX_VALUE is the correct fallback.)

This patch does not introduce any invocations of set_length.  Such invocations will be forthcoming on Bug 681803 (its going to be uploaded next), and presumably on Bug 685323 (though I don't have that patch prepared yet).

Comment 20

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 559447 [details] [diff] [review]
patch A v2: add private set_length with versioned exception

(Ed: feel free to reassign yourself as SR? and put someone else as R?.)

((also, this patch may not make sense without also reviewing Bug 681803's attachment 559450 [details] [diff] [review] in parallel))

Comment 22

[Edwin Smith]

I'll keep the SR open but go ahead and land when you're ready.

Comment 23

[Felix S. Klock II [:pnkfelix, :fklock]]

(running buildbot now)

Comment 24

[Felix S. Klock II [:pnkfelix, :fklock]]

Comment on attachment 559294 [details] [diff] [review]
test L v2: acceptance test for length wraparound

(gentle review ping)

Comment 25

[Tamarin Bot]

changeset: 6603:7087fdc0c36b
user:      Felix S Klock II <[e-mail]>
summary:   Bug 661330: add private set_length method with versioned exception (r=lhansen, sr pending=edwsmith).

http://hg.mozilla.org/tamarin-redux/rev/7087fdc0c36b

Comment 26

[Tamarin Bot]

changeset: 6709:ac91969bcc46
user:      Felix S Klock II <[e-mail]>
summary:   Bug 661330: acceptance tests for modifying Array .length property (r=dschaffe).

http://hg.mozilla.org/tamarin-redux/rev/ac91969bcc46

Comment 27

[Edwin Smith]

length_mods.as fails with 64-bit Release builds and -Dinterp, but apparently other configurations and jit-modes work.

Comment 28

[Felix S. Klock II [:pnkfelix, :fklock]]

oops, will revert.  (didn't see notification in email; is buildbot just laggy at the moment?)

Comment 29

[Tamarin Bot]

changeset: 6710:0bd4094457a5
user:      Felix S Klock II <[e-mail]>
summary:   Bug 661330: Backed out changeset ac91969bcc46

http://hg.mozilla.org/tamarin-redux/rev/0bd4094457a5

Comment 30

[Felix S. Klock II [:pnkfelix, :fklock]]

(In reply to Edwin Smith from comment #27)
> length_mods.as fails with 64-bit Release builds and -Dinterp, but apparently
> other configurations and jit-modes work.

This appears to be due to Bug 700613 (which may itself bifurcate into separate bugs).

I am guessing I should either not land the acceptance tests until that is fixed, or disable this paticular part of the tests but still land the tests, so that this bug need not be blocked by Bug 700613.

Comment 31

[Tamarin Bot]

changeset: 6714:27339d0b1836
user:      Felix S Klock II <[e-mail]>
summary:   Bug 661330: acceptance tests for Array .length property, land attempt II (r=dschaffe).

http://hg.mozilla.org/tamarin-redux/rev/27339d0b1836