Amend DataView, ArrayBuffer, and TypedArray methods to use ToIndex.

The spec was modified to relax some requirements which implementors had not been
enforcing. Part of this process involved introducing a new abstract operation
ToIndex, which had partial overlap with our existing semantics as well as some
differences (most notably treating undefined as 0). Test262 tests were introduced to
check for the new semantics, some of which we were failing. This patch amends the
parts of our implementation corresponding to specification algorithms which use
ToIndex to follow its semantics precisely.

BUG=v8:4784,v8:5120

Review-Url: https://codereview.chromium.org/2090353003
Cr-Commit-Position: refs/heads/master@{#37406}

src/builtins.cc

@@ -3101,65 +3101,52 @@ BUILTIN(DataViewConstructor_ConstructStub) {
   }
   Handle<JSArrayBuffer> array_buffer = Handle<JSArrayBuffer>::cast(buffer);
 
-  // 4. Let numberOffset be ? ToNumber(byteOffset).
-  Handle<Object> number_offset;
-  if (byte_offset->IsUndefined(isolate)) {
-    // We intentionally violate the specification at this point to allow
-    // for new DataView(buffer) invocations to be equivalent to the full
-    // new DataView(buffer, 0) invocation.
-    number_offset = handle(Smi::FromInt(0), isolate);
-  } else {
-    ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, number_offset,
-                                       Object::ToNumber(byte_offset));
-  }
-
-  // 5. Let offset be ToInteger(numberOffset).
+  // 4. Let offset be ToIndex(byteOffset).
   Handle<Object> offset;
-  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, offset,
-                                     Object::ToInteger(isolate, number_offset));
-
-  // 6. If numberOffset ≠ offset or offset < 0, throw a RangeError exception.
-  if (number_offset->Number() != offset->Number() || offset->Number() < 0.0) {
-    THROW_NEW_ERROR_RETURN_FAILURE(
-        isolate, NewRangeError(MessageTemplate::kInvalidDataViewOffset));
-  }
+  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
+      isolate, offset,
+      Object::ToIndex(isolate, byte_offset,
+                      MessageTemplate::kInvalidDataViewOffset));
 
-  // 7. If IsDetachedBuffer(buffer) is true, throw a TypeError exception.
+  // 5. If IsDetachedBuffer(buffer) is true, throw a TypeError exception.
   // We currently violate the specification at this point.
 
-  // 8. Let bufferByteLength be the value of buffer's [[ArrayBufferByteLength]]
+  // 6. Let bufferByteLength be the value of buffer's [[ArrayBufferByteLength]]
   // internal slot.
   double const buffer_byte_length = array_buffer->byte_length()->Number();
 
-  // 9. If offset > bufferByteLength, throw a RangeError exception
+  // 7. If offset > bufferByteLength, throw a RangeError exception
   if (offset->Number() > buffer_byte_length) {
     THROW_NEW_ERROR_RETURN_FAILURE(
-        isolate, NewRangeError(MessageTemplate::kInvalidDataViewOffset));
+        isolate,
+        NewRangeError(MessageTemplate::kInvalidDataViewOffset, offset));
   }
 
   Handle<Object> view_byte_length;
   if (byte_length->IsUndefined(isolate)) {
-    // 10. If byteLength is undefined, then
+    // 8. If byteLength is undefined, then
     //       a. Let viewByteLength be bufferByteLength - offset.
     view_byte_length =
         isolate->factory()->NewNumber(buffer_byte_length - offset->Number());
   } else {
-    // 11. Else,
-    //       a. Let viewByteLength be ? ToLength(byteLength).
+    // 9. Else,
+    //       a. Let viewByteLength be ? ToIndex(byteLength).
     //       b. If offset+viewByteLength > bufferByteLength, throw a RangeError
     //          exception
     ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
-        isolate, view_byte_length, Object::ToLength(isolate, byte_length));
+        isolate, view_byte_length,
+        Object::ToIndex(isolate, byte_length,
+                        MessageTemplate::kInvalidDataViewLength));
     if (offset->Number() + view_byte_length->Number() > buffer_byte_length) {
       THROW_NEW_ERROR_RETURN_FAILURE(
           isolate, NewRangeError(MessageTemplate::kInvalidDataViewLength));
     }
   }
 
-  // 12. Let O be ? OrdinaryCreateFromConstructor(NewTarget,
+  // 10. Let O be ? OrdinaryCreateFromConstructor(NewTarget,
   //     "%DataViewPrototype%", «[[DataView]], [[ViewedArrayBuffer]],
   //     [[ByteLength]], [[ByteOffset]]»).
-  // 13. Set O's [[DataView]] internal slot to true.
+  // 11. Set O's [[DataView]] internal slot to true.
   Handle<JSObject> result;
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, result,
                                      JSObject::New(target, new_target));
@@ -3167,16 +3154,16 @@ BUILTIN(DataViewConstructor_ConstructStub) {
     Handle<JSDataView>::cast(result)->SetInternalField(i, Smi::FromInt(0));
   }
 
-  // 14. Set O's [[ViewedArrayBuffer]] internal slot to buffer.
+  // 12. Set O's [[ViewedArrayBuffer]] internal slot to buffer.
   Handle<JSDataView>::cast(result)->set_buffer(*array_buffer);
 
-  // 15. Set O's [[ByteLength]] internal slot to viewByteLength.
+  // 13. Set O's [[ByteLength]] internal slot to viewByteLength.
   Handle<JSDataView>::cast(result)->set_byte_length(*view_byte_length);
 
-  // 16. Set O's [[ByteOffset]] internal slot to offset.
+  // 14. Set O's [[ByteOffset]] internal slot to offset.
   Handle<JSDataView>::cast(result)->set_byte_offset(*offset);
 
-  // 17. Return O.
+  // 15. Return O.
   return *result;
 }
 

src/js/runtime.js

@@ -45,6 +45,13 @@ function ToPositiveInteger(x, rangeErrorIndex) {
 }
 
 
+function ToIndex(x, rangeErrorIndex) {
+  var i = TO_INTEGER(x) + 0;
+  if (i < 0 || i > kMaxSafeInteger) throw MakeRangeError(rangeErrorIndex);
+  return i;
+}
+
+
 function MaxSimple(a, b) {
   return a > b ? a : b;
 }
@@ -94,6 +101,7 @@ utils.Export(function(to) {
   to.MaxSimple = MaxSimple;
   to.MinSimple = MinSimple;
   to.ToPositiveInteger = ToPositiveInteger;
+  to.ToIndex = ToIndex;
   to.SpeciesConstructor = SpeciesConstructor;
 });
 

src/js/typedarray.js

@@ -46,6 +46,7 @@ var MinSimple;
 var PackedArrayReverse;
 var SpeciesConstructor;
 var ToPositiveInteger;
+var ToIndex;
 var iteratorSymbol = utils.ImportNow("iterator_symbol");
 var speciesSymbol = utils.ImportNow("species_symbol");
 var toStringTagSymbol = utils.ImportNow("to_string_tag_symbol");
@@ -99,6 +100,7 @@ utils.Import(function(from) {
   PackedArrayReverse = from.PackedArrayReverse;
   SpeciesConstructor = from.SpeciesConstructor;
   ToPositiveInteger = from.ToPositiveInteger;
+  ToIndex = from.ToIndex;
 });
 
 // --------------- Typed Arrays ---------------------
@@ -143,10 +145,15 @@ function TypedArraySpeciesCreate(exemplar, arg0, arg1, arg2, conservative) {
 macro TYPED_ARRAY_CONSTRUCTOR(ARRAY_ID, NAME, ELEMENT_SIZE)
 function NAMEConstructByArrayBuffer(obj, buffer, byteOffset, length) {
   if (!IS_UNDEFINED(byteOffset)) {
-    byteOffset = ToPositiveInteger(byteOffset, kInvalidTypedArrayLength);
+    byteOffset = ToIndex(byteOffset, kInvalidTypedArrayLength);
   }
   if (!IS_UNDEFINED(length)) {
-    length = ToPositiveInteger(length, kInvalidTypedArrayLength);
+    length = ToIndex(length, kInvalidTypedArrayLength);
+  }
+  if (length > %_MaxSmi()) {
+    // Note: this is not per spec, but rather a constraint of our current
+    // representation (which uses smi's).
+    throw MakeRangeError(kInvalidTypedArrayLength);
   }
 
   var bufferByteLength = %_ArrayBufferGetByteLength(buffer);
@@ -160,35 +167,35 @@ function NAMEConstructByArrayBuffer(obj, buffer, byteOffset, length) {
       throw MakeRangeError(kInvalidTypedArrayAlignment,
                            "start offset", "NAME", ELEMENT_SIZE);
     }
-    if (offset > bufferByteLength) {
-      throw MakeRangeError(kInvalidTypedArrayOffset);
-    }
   }
 
   var newByteLength;
-  var newLength;
   if (IS_UNDEFINED(length)) {
     if (bufferByteLength % ELEMENT_SIZE !== 0) {
       throw MakeRangeError(kInvalidTypedArrayAlignment,
                            "byte length", "NAME", ELEMENT_SIZE);
     }
     newByteLength = bufferByteLength - offset;
-    newLength = newByteLength / ELEMENT_SIZE;
+    if (newByteLength < 0) {
+      throw MakeRangeError(kInvalidTypedArrayAlignment,
+                           "byte length", "NAME", ELEMENT_SIZE);
+    }
   } else {
-    var newLength = length;
-    newByteLength = newLength * ELEMENT_SIZE;
+    newByteLength = length * ELEMENT_SIZE;
+    if (offset + newByteLength > bufferByteLength) {
+      throw MakeRangeError(kInvalidTypedArrayAlignment,
+                           "byte length", "NAME", ELEMENT_SIZE);
     }
-  if ((offset + newByteLength > bufferByteLength)
-      || (newLength > %_MaxSmi())) {
-    throw MakeRangeError(kInvalidTypedArrayLength);
   }
   %_TypedArrayInitialize(obj, ARRAY_ID, buffer, offset, newByteLength, true);
 }
 
 function NAMEConstructByLength(obj, length) {
   var l = IS_UNDEFINED(length) ?
-    0 : ToPositiveInteger(length, kInvalidTypedArrayLength);
-  if (l > %_MaxSmi()) {
+    0 : ToIndex(length, kInvalidTypedArrayLength);
+  if (length > %_MaxSmi()) {
+    // Note: this is not per spec, but rather a constraint of our current
+    // representation (which uses smi's).
     throw MakeRangeError(kInvalidTypedArrayLength);
   }
   var byteLength = l * ELEMENT_SIZE;
@@ -866,8 +873,7 @@ function DataViewGetTYPENAMEJS(offset, little_endian) {
     throw MakeTypeError(kIncompatibleMethodReceiver,
                         'DataView.getTYPENAME', this);
   }
-  if (arguments.length < 1) throw MakeTypeError(kInvalidArgument);
-  offset = ToPositiveInteger(offset, kInvalidDataViewAccessorOffset);
+  offset = IS_UNDEFINED(offset) ? 0 : ToIndex(offset, kInvalidDataViewAccessorOffset);
   return %DataViewGetTYPENAME(this, offset, !!little_endian);
 }
 %FunctionSetLength(DataViewGetTYPENAMEJS, 1);
@@ -877,8 +883,7 @@ function DataViewSetTYPENAMEJS(offset, value, little_endian) {
     throw MakeTypeError(kIncompatibleMethodReceiver,
                         'DataView.setTYPENAME', this);
   }
-  if (arguments.length < 2) throw MakeTypeError(kInvalidArgument);
-  offset = ToPositiveInteger(offset, kInvalidDataViewAccessorOffset);
+  offset = IS_UNDEFINED(offset) ? 0 : ToIndex(offset, kInvalidDataViewAccessorOffset);
   %DataViewSetTYPENAME(this, offset, TO_NUMBER(value), !!little_endian);
 }
 %FunctionSetLength(DataViewSetTYPENAMEJS, 2);

src/messages.h

@@ -327,8 +327,9 @@ class CallSite {
   T(InvalidCurrencyCode, "Invalid currency code: %")                           \
   T(InvalidDataViewAccessorOffset,                                             \
     "Offset is outside the bounds of the DataView")                            \
-  T(InvalidDataViewLength, "Invalid data view length")                         \
-  T(InvalidDataViewOffset, "Start offset is outside the bounds of the buffer") \
+  T(InvalidDataViewLength, "Invalid DataView length %")                        \
+  T(InvalidDataViewOffset,                                                     \
+    "Start offset % is outside the bounds of the buffer")                      \
   T(InvalidHint, "Invalid hint: %")                                            \
   T(InvalidLanguageTag, "Invalid language tag: %")                             \
   T(InvalidWeakMapKey, "Invalid value used as weak map key")                   \

src/objects.cc

@@ -228,6 +228,18 @@ MaybeHandle<Object> Object::ToLength(Isolate* isolate, Handle<Object> input) {
   return isolate->factory()->NewNumber(len);
 }
 
+// static
+MaybeHandle<Object> Object::ToIndex(Isolate* isolate, Handle<Object> input,
+                                    MessageTemplate::Template error_index) {
+  if (input->IsUndefined(isolate)) return isolate->factory()->NewNumber(0.0);
+  ASSIGN_RETURN_ON_EXCEPTION(isolate, input, ToNumber(input), Object);
+  double len = DoubleToInteger(input->Number()) + 0.0;
+  auto js_len = isolate->factory()->NewNumber(len);
+  if (len < 0.0 || len > kMaxSafeInteger) {
+    THROW_NEW_ERROR(isolate, NewRangeError(error_index, js_len), Object);
+  }
+  return js_len;
+}
 
 bool Object::BooleanValue() {
   if (IsSmi()) return Smi::cast(this)->value() != 0;

src/objects.h

@@ -18,9 +18,10 @@
 #include "src/field-index.h"
 #include "src/flags.h"
 #include "src/list.h"
+#include "src/messages.h"
 #include "src/property-details.h"
-#include "src/unicode.h"
 #include "src/unicode-decoder.h"
+#include "src/unicode.h"
 #include "src/zone.h"
 
 #if V8_TARGET_ARCH_ARM
@@ -1177,6 +1178,11 @@ class Object {
   MUST_USE_RESULT static MaybeHandle<Object> ToLength(Isolate* isolate,
                                                       Handle<Object> input);
 
+  // ES6 section 7.1.17 ToIndex
+  MUST_USE_RESULT static MaybeHandle<Object> ToIndex(
+      Isolate* isolate, Handle<Object> input,
+      MessageTemplate::Template error_index);
+
   // ES6 section 7.3.9 GetMethod
   MUST_USE_RESULT static MaybeHandle<Object> GetMethod(
       Handle<JSReceiver> receiver, Handle<Name> name);

test/mjsunit/harmony/dataview-accessors.js

@@ -400,13 +400,9 @@ function TestGeneralAccessors() {
     assertThrows(function() { f(); }, TypeError);
     f.call(a, 0, 0); // should not throw
     assertThrows(function() { f.call({}, 0, 0); }, TypeError);
-    assertThrows(function() { f.call(a); }, TypeError);
-    if (name.indexOf("set") == 0) {
-      assertThrows(function() { f.call(a, 1); }, TypeError);
-    } else {
+    f.call(a);
     f.call(a, 1); // should not throw
   }
-  }
   CheckAccessor("getUint8");
   CheckAccessor("setUint8");
   CheckAccessor("getInt8");
@@ -429,33 +425,27 @@ TestGeneralAccessors();
 
 function TestInsufficientArguments() {
   var a = new DataView(new ArrayBuffer(256));
+  function CheckInsuficientArguments(type) {
+    var expectedValue = type === "Float32" || type === "Float64" ? NaN : 0;
+    var offset = getElementSize(type);
+
+    assertSame(undefined, a["set" + type](0, 7));
+    assertSame(undefined, a["set" + type]());
+    assertSame(expectedValue, a["get" + type]());
+
+    assertSame(undefined, a["set" + type](offset, 7));
+    assertSame(undefined, a["set" + type](offset));
+    assertSame(expectedValue, a["get" + type](offset));
+  }
 
-  assertThrows(function() { a.getUint8(); }, TypeError);
-  assertThrows(function() { a.getInt8(); }, TypeError);
-  assertThrows(function() { a.getUint16(); }, TypeError);
-  assertThrows(function() { a.getInt16(); }, TypeError);
-  assertThrows(function() { a.getUint32(); }, TypeError);
-  assertThrows(function() { a.getInt32(); }, TypeError);
-  assertThrows(function() { a.getFloat32(); }, TypeError);
-  assertThrows(function() { a.getFloat64(); }, TypeError);
-
-  assertThrows(function() { a.setUint8(); }, TypeError);
-  assertThrows(function() { a.setInt8(); }, TypeError);
-  assertThrows(function() { a.setUint16(); }, TypeError);
-  assertThrows(function() { a.setInt16(); }, TypeError);
-  assertThrows(function() { a.setUint32(); }, TypeError);
-  assertThrows(function() { a.setInt32(); }, TypeError);
-  assertThrows(function() { a.setFloat32(); }, TypeError);
-  assertThrows(function() { a.setFloat64(); }, TypeError);
-
-  assertThrows(function() { a.setUint8(1) }, TypeError);
-  assertThrows(function() { a.setInt8(1) }, TypeError);
-  assertThrows(function() { a.setUint16(1) }, TypeError);
-  assertThrows(function() { a.setInt16(1) }, TypeError);
-  assertThrows(function() { a.setUint32(1) }, TypeError);
-  assertThrows(function() { a.setInt32(1) }, TypeError);
-  assertThrows(function() { a.setFloat32(1) }, TypeError);
-  assertThrows(function() { a.setFloat64(1) }, TypeError);
+  CheckInsuficientArguments("Uint8");
+  CheckInsuficientArguments("Int8");
+  CheckInsuficientArguments("Uint16");
+  CheckInsuficientArguments("Int16");
+  CheckInsuficientArguments("Uint32");
+  CheckInsuficientArguments("Int32");
+  CheckInsuficientArguments("Float32");
+  CheckInsuficientArguments("Float64");
 }
 
 TestInsufficientArguments();

test/test262/test262.status

@@ -361,34 +361,6 @@
   'annexB/built-ins/Object/prototype/__lookupGetter__/this-non-obj': [FAIL],
   'annexB/built-ins/Object/prototype/__lookupSetter__/this-non-obj': [FAIL],
 
-  # https://bugs.chromium.org/p/v8/issues/detail?id=5120
-  'built-ins/DataView/negative-bytelength-throws': [FAIL],
-  'built-ins/DataView/prototype/getFloat32/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/getFloat64/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/getInt16/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/getInt32/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/getInt8/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/getUint16/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/getUint32/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/getUint8/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/setFloat32/no-value-arg': [FAIL],
-  'built-ins/DataView/prototype/setFloat32/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/setFloat64/no-value-arg': [FAIL],
-  'built-ins/DataView/prototype/setFloat64/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/setInt16/no-value-arg': [FAIL],
-  'built-ins/DataView/prototype/setInt16/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/setInt32/no-value-arg': [FAIL],
-  'built-ins/DataView/prototype/setInt32/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/setInt8/no-value-arg': [FAIL],
-  'built-ins/DataView/prototype/setInt8/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/setUint16/no-value-arg': [FAIL],
-  'built-ins/DataView/prototype/setUint16/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/setUint32/no-value-arg': [FAIL],
-  'built-ins/DataView/prototype/setUint32/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/prototype/setUint8/no-value-arg': [FAIL],
-  'built-ins/DataView/prototype/setUint8/toindex-byteoffset': [FAIL],
-  'built-ins/DataView/toindex-byteoffset': [FAIL],
-
   # https://bugs.chromium.org/p/v8/issues/detail?id=5121
   'language/expressions/assignment/destructuring/obj-prop-__proto__dup': [FAIL],
 
@@ -528,6 +500,20 @@
   # https://github.com/tc39/test262/issues/694
   'language/statements/class/subclass/builtin-objects/ArrayBuffer/regular-subclassing': [FAIL],
 
+  # https://github.com/tc39/test262/issues/685
+  'built-ins/DataView/prototype/setUint8/range-check-after-value-conversion': [FAIL],
+  'built-ins/DataView/prototype/setUint16/range-check-after-value-conversion': [FAIL],
+  'built-ins/DataView/prototype/setUint32/range-check-after-value-conversion': [FAIL],
+  'built-ins/DataView/prototype/setInt8/range-check-after-value-conversion': [FAIL],
+  'built-ins/DataView/prototype/setInt16/range-check-after-value-conversion': [FAIL],
+  'built-ins/DataView/prototype/setInt32/range-check-after-value-conversion': [FAIL],
+  'built-ins/DataView/prototype/setFloat32/range-check-after-value-conversion': [FAIL],
+  'built-ins/DataView/prototype/setFloat64/range-check-after-value-conversion': [FAIL],
+
+  # https://github.com/tc39/test262/issues/686
+  'built-ins/DataView/prototype/setFloat32/toindex-byteoffset': [FAIL],
+  'built-ins/DataView/prototype/setFloat64/toindex-byteoffset': [FAIL],
+
   ############################ SKIPPED TESTS #############################
 
   # These tests take a looong time to run.