[builtins] DataView should throws on detached buffer

DataView constructor, DataView.prototype.byteLength
and DataView.prototype.byteOffset should throw
TypeError when the buffer was detached.

Both SpiderMonkey and JSC passed the test262 suites.

Bug: v8:12162
Change-Id: I126d24213c00e4d26540519bce9b5388862eb32c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3140015
Reviewed-by: Shu-yu Guo <syg@chromium.org>
Commit-Queue: Shu-yu Guo <syg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#76818}

src/builtins/builtins-dataview.cc

@@ -19,6 +19,7 @@ namespace internal {
 
 // ES #sec-dataview-constructor
 BUILTIN(DataViewConstructor) {
+  const char* const kMethodName = "DataView constructor";
   HandleScope scope(isolate);
   if (args.new_target()->IsUndefined(isolate)) {  // [[Call]]
     THROW_NEW_ERROR_RETURN_FAILURE(
@@ -33,29 +34,31 @@ BUILTIN(DataViewConstructor) {
   Handle<Object> byte_offset = args.atOrUndefined(isolate, 2);
   Handle<Object> byte_length = args.atOrUndefined(isolate, 3);
 
-  // 2. If Type(buffer) is not Object, throw a TypeError exception.
-  // 3. If buffer does not have an [[ArrayBufferData]] internal slot, throw a
-  //    TypeError exception.
+  // 2. Perform ? RequireInternalSlot(buffer, [[ArrayBufferData]]).
   if (!buffer->IsJSArrayBuffer()) {
     THROW_NEW_ERROR_RETURN_FAILURE(
         isolate, NewTypeError(MessageTemplate::kDataViewNotArrayBuffer));
   }
   Handle<JSArrayBuffer> array_buffer = Handle<JSArrayBuffer>::cast(buffer);
 
-  // 4. Let offset be ? ToIndex(byteOffset).
+  // 3. Let offset be ? ToIndex(byteOffset).
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
       isolate, byte_offset,
       Object::ToIndex(isolate, byte_offset, MessageTemplate::kInvalidOffset));
   size_t view_byte_offset = byte_offset->Number();
 
-  // 5. If IsDetachedBuffer(buffer) is true, throw a TypeError exception.
-  //    We currently violate the specification at this point. TODO: Fix that.
+  // 4. If IsDetachedBuffer(buffer) is true, throw a TypeError exception.
+  if (array_buffer->was_detached()) {
+    THROW_NEW_ERROR_RETURN_FAILURE(
+        isolate, NewTypeError(MessageTemplate::kDetachedOperation,
+                              isolate->factory()->NewStringFromAsciiChecked(
+                                  kMethodName)));
+  }
 
-  // 6. Let bufferByteLength be the value of buffer's
-  // [[ArrayBufferByteLength]] internal slot.
+  // 5. Let bufferByteLength be buffer.[[ArrayBufferByteLength]].
   size_t const buffer_byte_length = array_buffer->byte_length();
 
-  // 7. If offset > bufferByteLength, throw a RangeError exception.
+  // 6. If offset > bufferByteLength, throw a RangeError exception.
   if (view_byte_offset > buffer_byte_length) {
     THROW_NEW_ERROR_RETURN_FAILURE(
         isolate, NewRangeError(MessageTemplate::kInvalidOffset, byte_offset));
@@ -63,11 +66,11 @@ BUILTIN(DataViewConstructor) {
 
   size_t view_byte_length;
   if (byte_length->IsUndefined(isolate)) {
-    // 8. If byteLength is either not present or undefined, then
+    // 7. If byteLength is undefined, then
     //       a. Let viewByteLength be bufferByteLength - offset.
     view_byte_length = buffer_byte_length - view_byte_offset;
   } else {
-    // 9. Else,
+    // 8. Else,
     //       a. Let viewByteLength be ? ToIndex(byteLength).
     //       b. If offset+viewByteLength > bufferByteLength, throw a
     //          RangeError exception.
@@ -82,30 +85,45 @@ BUILTIN(DataViewConstructor) {
     view_byte_length = byte_length->Number();
   }
 
-  // 10. Let O be ? OrdinaryCreateFromConstructor(NewTarget,
-  //     "%DataViewPrototype%", «[[DataView]], [[ViewedArrayBuffer]],
-  //     [[ByteLength]], [[ByteOffset]]»).
+  // 9. Let O be ? OrdinaryCreateFromConstructor(NewTarget,
+  //    "%DataViewPrototype%", «[[DataView]], [[ViewedArrayBuffer]],
+  //    [[ByteLength]], [[ByteOffset]]»).
   Handle<JSObject> result;
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
       isolate, result,
       JSObject::New(target, new_target, Handle<AllocationSite>::null()));
+  Handle<JSDataView> data_view = Handle<JSDataView>::cast(result);
   for (int i = 0; i < ArrayBufferView::kEmbedderFieldCount; ++i) {
     // TODO(v8:10391, saelo): Handle external pointers in EmbedderDataSlot
-    Handle<JSDataView>::cast(result)->SetEmbedderField(i, Smi::zero());
+    data_view->SetEmbedderField(i, Smi::zero());
   }
 
-  // 11. Set O's [[ViewedArrayBuffer]] internal slot to buffer.
-  Handle<JSDataView>::cast(result)->set_buffer(*array_buffer);
+  // We have to set the internal slots before the detached check on step 10 or
+  // the TorqueGeneratedClassVerifier ended up complaining that the slot is
+  // empty or invalid on heap teardown.
+  // The result object is not observable from JavaScript when step 10 early
+  // aborts so it is fine to set internal slots here.
 
-  // 12. Set O's [[ByteLength]] internal slot to viewByteLength.
-  Handle<JSDataView>::cast(result)->set_byte_length(view_byte_length);
+  // 11. Set O.[[ViewedArrayBuffer]] to buffer.
+  data_view->set_buffer(*array_buffer);
 
-  // 13. Set O's [[ByteOffset]] internal slot to offset.
-  Handle<JSDataView>::cast(result)->set_byte_offset(view_byte_offset);
-  Handle<JSDataView>::cast(result)->set_data_pointer(
+  // 12. Set O.[[ByteLength]] to viewByteLength.
+  data_view->set_byte_length(view_byte_length);
+
+  // 13. Set O.[[ByteOffset]] to offset.
+  data_view->set_byte_offset(view_byte_offset);
+  data_view->set_data_pointer(
       isolate,
       static_cast<uint8_t*>(array_buffer->backing_store()) + view_byte_offset);
 
+  // 10. If IsDetachedBuffer(buffer) is true, throw a TypeError exception.
+  if (array_buffer->was_detached()) {
+    THROW_NEW_ERROR_RETURN_FAILURE(
+        isolate, NewTypeError(MessageTemplate::kDetachedOperation,
+                              isolate->factory()->NewStringFromAsciiChecked(
+                                  kMethodName)));
+  }
+
   // 14. Return O.
   return *result;
 }

src/builtins/data-view.tq

@@ -6,6 +6,11 @@
 
 namespace data_view {
 
+const kBuiltinNameByteLength: constexpr string =
+    'DateView.prototype.byteLength';
+const kBuiltinNameByteOffset: constexpr string =
+    'DateView.prototype.byteOffset';
+
 macro MakeDataViewGetterNameString(kind: constexpr ElementsKind): String {
   if constexpr (kind == ElementsKind::UINT8_ELEMENTS) {
     return 'DataView.prototype.getUint8';
@@ -85,9 +90,7 @@ javascript builtin DataViewPrototypeGetByteLength(
   const dataView: JSDataView =
       ValidateDataView(context, receiver, 'get DataView.prototype.byte_length');
   if (WasDetached(dataView)) {
-    // TODO(bmeurer): According to the ES6 spec, we should throw a TypeError
-    // here if the JSArrayBuffer of the {dataView} was detached.
-    return 0;
+    ThrowTypeError(MessageTemplate::kDetachedOperation, kBuiltinNameByteLength);
   }
   return Convert<Number>(dataView.byte_length);
 }
@@ -98,9 +101,7 @@ javascript builtin DataViewPrototypeGetByteOffset(
   const dataView: JSDataView =
       ValidateDataView(context, receiver, 'get DataView.prototype.byte_offset');
   if (WasDetached(dataView)) {
-    // TODO(bmeurer): According to the ES6 spec, we should throw a TypeError
-    // here if the JSArrayBuffer of the {dataView} was detached.
-    return 0;
+    ThrowTypeError(MessageTemplate::kDetachedOperation, kBuiltinNameByteOffset);
   }
   return Convert<Number>(dataView.byte_offset);
 }

test/cctest/test-api-array-buffer.cc

@@ -230,7 +230,18 @@ THREADED_TEST(ArrayBuffer_DetachingScript) {
   CheckIsTypedArrayVarDetached("f32a");
   CheckIsTypedArrayVarDetached("f64a");
 
-  CHECK(CompileRun("dv.byteLength == 0 && dv.byteOffset == 0")->IsTrue());
+  {
+    v8::TryCatch try_catch(isolate);
+    CompileRun("dv.byteLength == 0 ");
+    CHECK(try_catch.HasCaught());
+  }
+
+  {
+    v8::TryCatch try_catch(isolate);
+    CompileRun("dv.byteOffset == 0");
+    CHECK(try_catch.HasCaught());
+  }
+
   CheckDataViewIsDetached(dv);
 }
 

test/mjsunit/regress/regress-353004.js

@@ -36,23 +36,6 @@ assertThrows(() =>
 assertTrue(convertedOffset);
 assertTrue(convertedLength);
 
-var buffer3 = new ArrayBuffer(100 * 1024 * 1024);
-var dataView1 = new DataView(buffer3, {valueOf : function() {
-  %ArrayBufferDetach(buffer3);
-  return 0;
-}});
-
-assertEquals(0, dataView1.byteLength);
-
-var buffer4 = new ArrayBuffer(100 * 1024);
-assertThrows(function() {
-  var dataView2 = new DataView(buffer4, 0, {valueOf : function() {
-    %ArrayBufferDetach(buffer4);
-    return 100 * 1024 * 1024;
-  }});
-}, RangeError);
-
-
 var buffer5 = new ArrayBuffer(100 * 1024);
 assertThrows(function() {
   buffer5.slice({valueOf : function() {

test/test262/test262.status

@@ -69,12 +69,6 @@
   # https://code.google.com/p/v8/issues/detail?id=10958
   'language/module-code/eval-gtbndng-indirect-faux-assertion': [FAIL],
 
-  # DataView functions should also throw on detached buffers
-  'built-ins/DataView/detached-buffer': [FAIL],
-  'built-ins/DataView/prototype/byteLength/detached-buffer': [FAIL],
-  'built-ins/DataView/prototype/byteOffset/detached-buffer': [FAIL],
-  'built-ins/DataView/prototype/byteLength/instance-has-detached-buffer': [FAIL],
-  'built-ins/DataView/custom-proto-access-detaches-buffer': [FAIL],
   # copyWithin should also throw on detached buffers
   'built-ins/TypedArray/prototype/copyWithin/coerced-values-end-detached-prototype': [FAIL],
   'built-ins/TypedArray/prototype/copyWithin/coerced-values-start-detached': [FAIL],

tools/v8heapconst.py

@@ -474,27 +474,27 @@ KNOWN_OBJECTS = {
   ("old_space", 0x029b5): "StringSplitCache",
   ("old_space", 0x02dbd): "RegExpMultipleCache",
   ("old_space", 0x031c5): "BuiltinsConstantsTable",
-  ("old_space", 0x035e5): "AsyncFunctionAwaitRejectSharedFun",
-  ("old_space", 0x03609): "AsyncFunctionAwaitResolveSharedFun",
-  ("old_space", 0x0362d): "AsyncGeneratorAwaitRejectSharedFun",
-  ("old_space", 0x03651): "AsyncGeneratorAwaitResolveSharedFun",
-  ("old_space", 0x03675): "AsyncGeneratorYieldResolveSharedFun",
-  ("old_space", 0x03699): "AsyncGeneratorReturnResolveSharedFun",
-  ("old_space", 0x036bd): "AsyncGeneratorReturnClosedRejectSharedFun",
-  ("old_space", 0x036e1): "AsyncGeneratorReturnClosedResolveSharedFun",
-  ("old_space", 0x03705): "AsyncIteratorValueUnwrapSharedFun",
-  ("old_space", 0x03729): "PromiseAllResolveElementSharedFun",
-  ("old_space", 0x0374d): "PromiseAllSettledResolveElementSharedFun",
-  ("old_space", 0x03771): "PromiseAllSettledRejectElementSharedFun",
-  ("old_space", 0x03795): "PromiseAnyRejectElementSharedFun",
-  ("old_space", 0x037b9): "PromiseCapabilityDefaultRejectSharedFun",
-  ("old_space", 0x037dd): "PromiseCapabilityDefaultResolveSharedFun",
-  ("old_space", 0x03801): "PromiseCatchFinallySharedFun",
-  ("old_space", 0x03825): "PromiseGetCapabilitiesExecutorSharedFun",
-  ("old_space", 0x03849): "PromiseThenFinallySharedFun",
-  ("old_space", 0x0386d): "PromiseThrowerFinallySharedFun",
-  ("old_space", 0x03891): "PromiseValueThunkFinallySharedFun",
-  ("old_space", 0x038b5): "ProxyRevokeSharedFun",
+  ("old_space", 0x035ed): "AsyncFunctionAwaitRejectSharedFun",
+  ("old_space", 0x03611): "AsyncFunctionAwaitResolveSharedFun",
+  ("old_space", 0x03635): "AsyncGeneratorAwaitRejectSharedFun",
+  ("old_space", 0x03659): "AsyncGeneratorAwaitResolveSharedFun",
+  ("old_space", 0x0367d): "AsyncGeneratorYieldResolveSharedFun",
+  ("old_space", 0x036a1): "AsyncGeneratorReturnResolveSharedFun",
+  ("old_space", 0x036c5): "AsyncGeneratorReturnClosedRejectSharedFun",
+  ("old_space", 0x036e9): "AsyncGeneratorReturnClosedResolveSharedFun",
+  ("old_space", 0x0370d): "AsyncIteratorValueUnwrapSharedFun",
+  ("old_space", 0x03731): "PromiseAllResolveElementSharedFun",
+  ("old_space", 0x03755): "PromiseAllSettledResolveElementSharedFun",
+  ("old_space", 0x03779): "PromiseAllSettledRejectElementSharedFun",
+  ("old_space", 0x0379d): "PromiseAnyRejectElementSharedFun",
+  ("old_space", 0x037c1): "PromiseCapabilityDefaultRejectSharedFun",
+  ("old_space", 0x037e5): "PromiseCapabilityDefaultResolveSharedFun",
+  ("old_space", 0x03809): "PromiseCatchFinallySharedFun",
+  ("old_space", 0x0382d): "PromiseGetCapabilitiesExecutorSharedFun",
+  ("old_space", 0x03851): "PromiseThenFinallySharedFun",
+  ("old_space", 0x03875): "PromiseThrowerFinallySharedFun",
+  ("old_space", 0x03899): "PromiseValueThunkFinallySharedFun",
+  ("old_space", 0x038bd): "ProxyRevokeSharedFun",
 }
 
 # Lower 32 bits of first page addresses for various heap spaces.