Closed Bug 1255128 Opened 4 years ago Closed 3 years ago

ArrayBuffer constructor uses ToIndex in ES2017

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox48 --- affected
firefox53 --- fixed

People

(Reporter: nbp, Assigned: evilpie)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete)

Attachments

(3 files, 6 obsolete files)

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
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
Flags: needinfo?(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.
Flags: needinfo?(luke)
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1205616
Assignee: nobody → jorendorff
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
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
Attachment #8732917 - Flags: review?(nicolas.b.pierron) → review+
(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)
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?
My fault - brain malfunction.  The number in Jason's post is 2^53-1 of course.
(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.  :-)
Good grief.
Thanks, Waldo. Makes sense to me. "Safe integer" != "integer".
MozReview-Commit-ID: 8cFpoe4je9O
Attachment #8732917 - Attachment is obsolete: true
Duplicate of this bug: 1232265
Blocks: es6
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.
This has to be updated; they changed the draft spec again.
Flags: needinfo?(jorendorff)
No longer blocks: es6
Blocks: test262
Attached patch Unify ToIndex handling (obsolete) — Splinter Review
We are going to add one correct ToIndex method. Some tests for SIMD and Atomics need to be updated.
Attachment #8745508 - Attachment is obsolete: true
Assignee: jorendorff → evilpies
Blocks: 1317382
Blocks: 1317383
Attached patch Unify ToIndex handling (obsolete) — Splinter Review
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.
Attachment #8818511 - Attachment is obsolete: true
Attachment #8819947 - Flags: review?(andrebargull)
Attachment #8819949 - Flags: review?(andrebargull)
Comment on attachment 8819947 [details] [diff] [review]
Unify ToIndex handling

Switching reviewers.
Attachment #8819947 - Flags: review?(andrebargull) → review?(arai.unmht)
Attachment #8819949 - Flags: review?(andrebargull) → review?(arai.unmht)
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?
Attachment #8819947 - Flags: review?(arai.unmht)
Attachment #8819949 - Flags: review?(arai.unmht) → review+
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?
Thank you both for reviewing.
Attachment #8819947 - Attachment is obsolete: true
Attachment #8821221 - Flags: review?(arai.unmht)
Attached patch Disable DOM test (obsolete) — Splinter Review
Attachment #8821345 - Flags: review?(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.
Attachment #8821345 - Flags: review?(bzbarsky) → review-
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.
Attachment #8821221 - Flags: review?(arai.unmht) → review+
I didn't realize we could change wpt files.
Attachment #8821345 - Attachment is obsolete: true
Attachment #8821503 - Flags: review?(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.)
Attachment #8821503 - Flags: review?(bzbarsky) → review-
> 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)
Attachment #8821503 - Attachment is obsolete: true
Attachment #8821662 - Flags: review?(bzbarsky)
Comment on attachment 8821662 [details] [diff] [review]
Adjust webplatform ArrayBuffer test

r=me.  Thank you!
Attachment #8821662 - Flags: review?(bzbarsky) → review+
Pushed by evilpies@gmail.com:
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
Summary: "new ArrayBuffer(0x100000000) " does not report any error message. → ArrayBuffer constructor uses ToIndex in ES2017
https://hg.mozilla.org/mozilla-central/rev/824d87defff9
https://hg.mozilla.org/mozilla-central/rev/0ae67adc68ac
https://hg.mozilla.org/mozilla-central/rev/cbef6d096dd9
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Duplicate of this bug: 1290349
You need to log in before you can comment on or make changes to this bug.