1255128 - ArrayBuffer constructor uses ToIndex in ES2017

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Nicolas B. Pierron [:nbp]]

In SpiderMonkey:
> new ArrayBuffer(0x100000000)
ArrayBuffer { byteLength: 0 }

In v8:
> new ArrayBuffer(0x100000000)
RangeError: Invalid array buffer length

This issue is likely to be related to the way we handle argument and force the input to be coerced into an Int32 in [1], I do not know the spec, but we should probably follow the same schema as we do for Array [2].

(sounds like a good-first-bug)

[1] https://dxr.mozilla.org/mozilla-central/source/js/src/vm/ArrayBufferObject.cpp#207
[2] https://dxr.mozilla.org/mozilla-central/source/js/src/jsarray.cpp#3211

Comment 1

[Jason Orendorff [:jorendorff]]

The spec says go ahead and allocate a 4GB buffer. Throwing RangeErrors instead is fine, though.

V8 seems to have a hard cut-off at 1GB. Luke, is it a problem if we follow suit?

https://tc39.github.io/ecma262/#sec-arraybuffer-length
https://tc39.github.io/ecma262/#sec-typedarray-length

Comment 2

[Luke Wagner [:luke]]

No problem with hard cut off from an asm.js POV.  We actually have the reverse constraint: our heap optimization of a[i>>2] interprets any unsigned index > INT32_MAX to be out-of-bounds which, since >> performs ToInt32(), not ToUint32(), is only valid when ArrayBuffers are smaller than 2GiB.  However, if we wanted to allow >= 2GiB buffers one day, we could simply have asm.js linking fail for these large buffers, so it's not a hard constraint on the engine, just something to watch out for in the future.

Comment 4

[Jason Orendorff [:jorendorff]]

Attachment: Standard argument coercion in `new ArrayBuffer(length)`

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8732917 [details] [diff] [review]
Standard argument coercion in `new ArrayBuffer(length)`

Review of attachment 8732917 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsnum.h
@@ +269,5 @@
> +/* ECMA-262 draft (2016 Mar 19) 7.1.15 ToLength ( argument ) */
> +inline double
> +ToLength(double argument)
> +{
> +    const double MAX_SAFE_INTEGER = 9007199254740991;

Can you add this value as part of the DoubleTypeTraits [1], and use it as:
  mozilla::FloatingPoint<double>::kMaxInteger;

[1] https://dxr.mozilla.org/mozilla-central/source/mfbt/FloatingPoint.h#58

Comment 6

[Lars T Hansen [:lth]]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> > +inline double
> > +ToLength(double argument)
> > +{
> > +    const double MAX_SAFE_INTEGER = 9007199254740991;
> 
> Can you add this value as part of the DoubleTypeTraits [1], and use it as:
>   mozilla::FloatingPoint<double>::kMaxInteger;

Or use (uint64_t(1) << 54) - 1...

(cc :jolesen, I think I made him use 1 << 53 by accident the other day)

Comment 7

[Jakob Stoklund Olesen [:jolesen]]

2^53 is the last of the contiguous integers that are floating point numbers. 2^53+1 is not a floating point number.

I don't know why ECMA ToLength() uses 2^53-1 instead of 2^53 as the upper limit?

Comment 8

[Lars T Hansen [:lth]]

My fault - brain malfunction.  The number in Jason's post is 2^53-1 of course.

Comment 9

[Jeff Walden [:Waldo]]

(In reply to Nicolas B. Pierron [:nbp] from comment #5)
> > +    const double MAX_SAFE_INTEGER = 9007199254740991;
> 
> Can you add this value as part of the DoubleTypeTraits [1], and use it as:
>   mozilla::FloatingPoint<double>::kMaxInteger;

Yeah, this is a good idea.  But add it to template<typename T> struct FloatingPoint so that both FloatingPoint<double> and FloatingPoint<float> have it:

template<typename T>
struct FloatingPoint : public SelectTrait<T>
{
  typedef SelectTrait<T> Base;
  typedef typename Base::Bits Bits;

  ...

  static_assert(Base::kExponentShift + 1 < 64, "kMaxSafeInteger needs to have a bigger type");
  static const uint64_t kMaxSafeInteger = (uint64_t(1) << (Base::kExponentShift + 1)) - 1;
};

And add to the comment on the struct, something like

"""
kMaxSafeInteger is the maximum integral value representable in T, that cannot be the inexact result of a rounding operation on an integral computation.  This is *one less* than the maximum integral value in T before integral precision is lost.  For example, considering the IEEE-754 double type: 2**53 - 2 is a double, 2**53 - 1 is a double, 2**53 is a double, 2**53 + 1 IS NOT a double, 2**53 + 2 is a double.  A computation that evaluates to the mathematical value 2**53 + 1, will round-nearest-to-even to 2**53.  Therefore 2**53 is the maximum integral value in T before precision is lost.  And therefore 2**53 - 1 is the maximum integral value representable in T, that cannot be the inexact result of a rounding operation on an integral computation -- so kMaxSafeInteger is 2**53 - 1.
"""

Wording subject to criticism.  This concept is very messy and hard to explain.  And honestly, I'd make the constant 2**53, but there's minor JS precedent in Number.MAX_SAFE_INTEGER for having the constant be the one-less value.  And https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER is my current best attempt at describing this, and even that I'm really not happy with anyway.

(In reply to Lars T Hansen [:lth] from comment #6)
> Or use (uint64_t(1) << 54) - 1...

Noooooooooooooooooooooo we want a named constant here.  :-)

Comment 10

[Jason Orendorff [:jorendorff]]

Good grief.

Comment 11

[Jakob Stoklund Olesen [:jolesen]]

Thanks, Waldo. Makes sense to me. "Safe integer" != "integer".

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: Standard argument coercion in `new ArrayBuffer(length)`

MozReview-Commit-ID: 8cFpoe4je9O

Comment 13

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=62394f30f849

Comment 15

[Jason Orendorff [:jorendorff]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed7ae11297d3004257a98124d80cab8f34f30764
Bug 1255128 - Standard argument coercion in `new ArrayBuffer(length)`. r=nbp. Thanks to snowmantw for tests.

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This and bug 1260272 are backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/242bab2c3b06 for breaking android cpp tests: https://treeherder.mozilla.org/logviewer.html#?job_id=27166818&repo=mozilla-inbound

Comment 17

[Jason Orendorff [:jorendorff]]

This has to be updated; they changed the draft spec again.

Comment 18

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6481cfc1523e

Comment 19

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c09f9234f034

Comment 20

[Jason Orendorff [:jorendorff]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a2906003e05

Comment 21

[Tom S [:evilpie]]

Attachment: Unify ToIndex handling

We are going to add one correct ToIndex method. Some tests for SIMD and Atomics need to be updated.

Comment 22

[Tom S [:evilpie]]

Attachment: Unify ToIndex handling

I am actually a bit surprised about how much ToIndex actually still allows and it seems to be used only quite sparely in the spec. I didn't bother to update SIMD, too many test failures to make that worth it. I opened https://github.com/tc39/ecmascript_sharedmem/issues/160 about using ToIndex for Atomics in the spec as well.

Comment 23

[Tom S [:evilpie]]

Attachment: Import test262 ArrayBuffer ToIndex tests

Comment 24

[Tom S [:evilpie]]

Comment on attachment 8819947 [details] [diff] [review]
Unify ToIndex handling

Switching reviewers.

Comment 25

[Tooru Fujisawa [:arai]]

Comment on attachment 8819947 [details] [diff] [review]
Unify ToIndex handling

Review of attachment 8819947 [details] [diff] [review]:
-----------------------------------------------------------------

Looks almost good, but I cannot r+ SharedMemory part without the spec change.

::: js/src/builtin/AtomicsObject.cpp
@@ +114,4 @@
>  GetTypedArrayIndex(JSContext* cx, HandleValue v, Handle<TypedArrayObject*> view, uint32_t* offset)
>  {
>      uint64_t index;
> +    if (!ToIndex(cx, v, &index))

Can this keep using old algorithm until the spec actually changes?

::: js/src/jsnum.cpp
@@ +1815,4 @@
>  js::ToLengthClamped<ExclusiveContext>(ExclusiveContext*, HandleValue, uint32_t*, bool*);
>  
>  bool
> +js::ToIndex(JSContext* cx, JS::HandleValue v, uint64_t* index)

can you add spec section comment above?
"ES 2017 draft 7.1.17" or similar.

::: js/src/jsnum.h
@@ +276,3 @@
>  MOZ_MUST_USE bool ToLengthClamped(T* cx, HandleValue v, uint32_t* out, bool* overflow);
>  
>  /* Convert and range check an index value as for DataView, SIMD, and Atomics

can you remove SIMD here?

Comment 26

[André Bargull [:anba]]

Comment on attachment 8819947 [details] [diff] [review]
Unify ToIndex handling

Review of attachment 8819947 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsnum.cpp
@@ +1829,5 @@
>          return false;
>  
> +    // Inlined version of ToLength.
> +    // 1. Already an integer
> +    // 2. Step eliminates < 0, +0 == -0 with SameValueZero

Please add a trailing full stop here and at step 1.

@@ +1831,5 @@
> +    // Inlined version of ToLength.
> +    // 1. Already an integer
> +    // 2. Step eliminates < 0, +0 == -0 with SameValueZero
> +    // 3/4. Limit to <= 2^53-1, so everything above should fail.
> +    if (integerIndex < 0 || integerIndex > 9007199254740991) {

Slight preference for either a hex-literal or `(uint64_t(1) << 53) - 1`.

::: js/src/jsnum.h
@@ +280,3 @@
>   *
>   * Return true and set |*index| to the integer value if |argument| is a valid
> + * array index argument. Otherwise report an RangeError and return false.

Can we use a different name than "array index" to avoid confusion with ECMAScript's "array index" definition <https://tc39.github.io/ecma262/#sec-array-exotic-objects>? For example <https://tc39.github.io/ecma262/#sec-toindex> uses "valid integer index value".
 
And "a RangeError".

::: js/src/vm/ArrayBufferObject.cpp
@@ +281,4 @@
>          return false;
>  
> +    // Non-standard: Refuse to allocate buffers 1GiB or larger.
> +    const uint64_t SIZE_LIMIT = 1024 * 1024 * 1024;

Shouldn't we keep the limit at INT32_MAX?

Comment 27

[Tom S [:evilpie]]

Attachment: Unify ToIndex handling and fix ArrayBuffer constructor

Thank you both for reviewing.

Comment 28

[Tom S [:evilpie]]

Attachment: Disable DOM test

Comment 29

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8821345 [details] [diff] [review]
Disable DOM test

This test is meant to be testing whatever the spec says.  If the spec has changed from when the test was written, please fix the test accordingly instead of disabling it.

Comment 30

[Tooru Fujisawa [:arai]]

Comment on attachment 8821221 [details] [diff] [review]
Unify ToIndex handling and fix ArrayBuffer constructor

Review of attachment 8821221 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you :D

::: js/src/jsnum.h
@@ +285,2 @@
>   *
>   * The returned index will always be in the range 0 <= *index <= 2^53.

2^53-1

so, the range is different between ToIndex and NonStandardToIndex, right?
in that case it would be nice to keep the old comment for NonStandardToIndex.

Comment 31

[Tom S [:evilpie]]

Attachment: Adjust webplatform ArrayBuffer test

I didn't realize we could change wpt files.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8821503 [details] [diff] [review]
Adjust webplatform ArrayBuffer test

We can totally change wpt files if they're not following the spec.  We automatically upstream our local changes.  ;)

OK, so why are we no longer testing behavior of the +Infinity/-Infinity/-4043309056 cases?  The spec does define those, right?

Looking at the spec, specifically, it defines the following behavior:

1.  -4043309056 and -Infinity should throw RangeError per https://tc39.github.io/ecma262/#sec-toindex step 2b.
2.  +Infinity should throw RangeError per step 2d, right?

So just modify the test to test this.  Something like this:

var invalid_args = [
  +Infinity, -Infinity, 2**64, -2**64, -2**32, -2**31, -1
];
invalid_args.forEach(function (arg, i) {
  test(function () {
    assert_throws(RangeError, function() {
      var abuffer = new ArrayBuffer(arg);
    }
  }, "The argument " + format_value(arg) + " should cause a RangeError exception when passed to the ArrayBuffer constructor." + i);
});

(I haven't checked whether this code even runs, much less passes; so you probably want to do that.)

Comment 33

[Tom S [:evilpie]]

Attachment: Adjust webplatform ArrayBuffer test

> Looking at the spec, specifically, it defines the following behavior:
> 
> 1.  -4043309056 and -Infinity should throw RangeError per
> https://tc39.github.io/ecma262/#sec-toindex step 2b.
> 2.  +Infinity should throw RangeError per step 2d, right?
Yes that seems correct. I also added a test for 2**53, which is just above the 2**53-1 limit. (Technically we have a much lower limit internal limit, but other browser could allow higher numbers to succeed)

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8821662 [details] [diff] [review]
Adjust webplatform ArrayBuffer test

r=me.  Thank you!

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/824d87defff9
Import test262 ArrayBuffer ToIndex tests. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ae67adc68ac
Fix ArrayBuffer constructor parameter handling with ToIndex. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbef6d096dd9
Adjust webplatform ArrayBuffer test . r=bz

Comment 36

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/824d87defff9
https://hg.mozilla.org/mozilla-central/rev/0ae67adc68ac
https://hg.mozilla.org/mozilla-central/rev/cbef6d096dd9

Comment 37

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer#Exceptions
https://developer.mozilla.org/en-US/Firefox/Releases/53#JavaScript