[debug] Make JSArrayBuffer (pre)views into internal properties.

For JSArrayBuffer instances (which map to both v8::ArrayBuffer and
v8::SharedArrayBuffer), we add a couple of synthetic views to its
ValueMirror to make it easy for developers to peak into the contents of
the JSArrayBuffer. These were previously real properties, but that's
just wrong (both intuitively and semantically), and they should instead
be internal properties.

Drive-by-fix: The [[IsDetached]] internal property should only be shown
on actually detached JSArrayBuffer's to reduce visual clutter. And for
detached JSArrayBuffers creating views on them throws TypeErrors per
specification, so we shouldn't attempt to display views on them.

Bug: v8:9308, chromium:1162229
Change-Id: Ia006de7873ca4b27aae7d00d46e1b69d2e326449
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2606047
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71892}

src/inspector/value-mirror.cc

@@ -1366,45 +1366,7 @@ bool doesAttributeHaveObservableSideEffectOnGet(v8::Local<v8::Context> context,
   }
   return false;
 }
-template <typename ArrayView, typename ArrayBuffer>
-void addTypedArrayView(v8::Local<v8::Context> context,
-                       v8::Local<ArrayBuffer> buffer, size_t length,
-                       const char* name,
-                       ValueMirror::PropertyAccumulator* accumulator) {
-  accumulator->Add(PropertyMirror{
-      String16(name), false, false, false, true, false,
-      ValueMirror::create(context, ArrayView::New(buffer, 0, length)), nullptr,
-      nullptr, nullptr, nullptr});
-}
-
-template <typename ArrayBuffer>
-void addTypedArrayViews(v8::Local<v8::Context> context,
-                        v8::Local<ArrayBuffer> buffer,
-                        ValueMirror::PropertyAccumulator* accumulator) {
-  // TODO(alph): these should be internal properties.
-  // TODO(v8:9308): Reconsider how large arrays are previewed.
-  const size_t byte_length = buffer->ByteLength();
-
-  size_t length = byte_length;
-  if (length > v8::TypedArray::kMaxLength) return;
-
-  addTypedArrayView<v8::Int8Array>(context, buffer, length, "[[Int8Array]]",
-                                   accumulator);
-  addTypedArrayView<v8::Uint8Array>(context, buffer, length, "[[Uint8Array]]",
-                                    accumulator);
-
-  length = byte_length / 2;
-  if (length > v8::TypedArray::kMaxLength || (byte_length % 2) != 0) return;
-
-  addTypedArrayView<v8::Int16Array>(context, buffer, length, "[[Int16Array]]",
-                                    accumulator);
 
-  length = byte_length / 4;
-  if (length > v8::TypedArray::kMaxLength || (byte_length % 4) != 0) return;
-
-  addTypedArrayView<v8::Int32Array>(context, buffer, length, "[[Int32Array]]",
-                                    accumulator);
-}
 }  // anonymous namespace
 
 ValueMirror::~ValueMirror() = default;
@@ -1439,15 +1401,6 @@ bool ValueMirror::getProperties(v8::Local<v8::Context> context,
 
   bool formatAccessorsAsProperties =
       clientFor(context)->formatAccessorsAsProperties(object);
-
-  if (object->IsArrayBuffer()) {
-    addTypedArrayViews(context, object.As<v8::ArrayBuffer>(), accumulator);
-  }
-  if (object->IsSharedArrayBuffer()) {
-    addTypedArrayViews(context, object.As<v8::SharedArrayBuffer>(),
-                       accumulator);
-  }
-
   for (auto iterator = v8::debug::PropertyIterator::Create(object);
        !iterator->Done(); iterator->Advance()) {
     bool isOwn = iterator->is_own();

src/runtime/runtime-debug.cc

@@ -306,13 +306,47 @@ MaybeHandle<JSArray> Runtime::GetInternalProperties(Isolate* isolate,
     return factory->NewJSArrayWithElements(result);
   } else if (object->IsJSArrayBuffer()) {
     Handle<JSArrayBuffer> js_array_buffer = Handle<JSArrayBuffer>::cast(object);
-    Handle<FixedArray> result = factory->NewFixedArray(1 * 2);
-
-    Handle<String> is_detached_str =
-        factory->NewStringFromAsciiChecked("[[IsDetached]]");
-    result->set(0, *is_detached_str);
-    result->set(1, isolate->heap()->ToBoolean(js_array_buffer->was_detached()));
-    return factory->NewJSArrayWithElements(result);
+    if (js_array_buffer->was_detached()) {
+      // Mark a detached JSArrayBuffer and such and don't even try to
+      // create views for it, since the TypedArray constructors will
+      // throw a TypeError when the underlying buffer is detached.
+      Handle<FixedArray> result = factory->NewFixedArray(1 * 2);
+      Handle<String> is_detached_str =
+          factory->NewStringFromAsciiChecked("[[IsDetached]]");
+      result->set(0, *is_detached_str);
+      result->set(1, isolate->heap()->ToBoolean(true));
+      return factory->NewJSArrayWithElements(result, PACKED_ELEMENTS);
+    }
+    const size_t byte_length = js_array_buffer->byte_length();
+    static const ExternalArrayType kTypes[] = {
+        kExternalInt8Array,
+        kExternalUint8Array,
+        kExternalInt16Array,
+        kExternalInt32Array,
+    };
+    Handle<FixedArray> result = factory->NewFixedArray(arraysize(kTypes) * 2);
+    int index = 0;
+    for (auto type : kTypes) {
+      switch (type) {
+#define TYPED_ARRAY_CASE(Type, type, TYPE, ctype)                            \
+  case kExternal##Type##Array: {                                             \
+    if ((byte_length % sizeof(ctype)) != 0) continue;                        \
+    Handle<String> typed_array_str =                                         \
+        factory->NewStringFromStaticChars("[[" #Type "Array]]");             \
+    Handle<JSTypedArray> js_typed_array =                                    \
+        factory->NewJSTypedArray(kExternal##Type##Array, js_array_buffer, 0, \
+                                 byte_length / sizeof(ctype));               \
+    result->set(index++, *typed_array_str);                                  \
+    result->set(index++, *js_typed_array);                                   \
+    break;                                                                   \
+  }
+        TYPED_ARRAYS(TYPED_ARRAY_CASE)
+#undef TYPED_ARRAY_CASE
+        default:
+          UNREACHABLE();
+      }
+    }
+    return factory->NewJSArrayWithElements(result, PACKED_ELEMENTS, index);
   }
   return factory->NewJSArray(0);
 }

test/inspector/debugger/get-properties-paused-expected.txt

@@ -52,40 +52,9 @@ Running test: testTypedArrayWithoutLength
   __proto__ own object undefined
 
 Running test: testArrayBuffer
-[[Int8Array]]
-  0 own number 1
-  1 own number 1
-  2 own number 1
-  3 own number 1
-  4 own number 1
-  5 own number 1
-  6 own number 1
-  7 own number 1
-  __proto__ own object undefined
-[[Uint8Array]]
-  0 own number 1
-  1 own number 1
-  2 own number 1
-  3 own number 1
-  4 own number 1
-  5 own number 1
-  6 own number 1
-  7 own number 1
-  __proto__ own object undefined
-[[Int16Array]]
-  0 own number 257
-  1 own number 257
-  2 own number 257
-  3 own number 257
-  __proto__ own object undefined
-[[Int32Array]]
-  0 own number 16843009
-  1 own number 16843009
-  __proto__ own object undefined
 
 Running test: testArrayBufferWithBrokenUintCtor
-  [[Int8Array]] own object undefined
-  [[Uint8Array]] own object undefined
   __proto__ own object undefined
 Internal properties
-  [[IsDetached]] boolean false
+  [[Int8Array]] object undefined
+  [[Uint8Array]] object undefined

test/inspector/runtime/get-properties-expected.txt

@@ -102,22 +102,12 @@ Running test: testArrayBuffer
   0 own number 16843009
   1 own number 16843009
   __proto__ own object undefined
-[[IsDetached]] false
 
 Running test: testDetachedArrayBuffer
-[[Int8Array]]
-  __proto__ own object undefined
-[[Uint8Array]]
-  __proto__ own object undefined
-[[Int16Array]]
-  __proto__ own object undefined
-[[Int32Array]]
-  __proto__ own object undefined
 [[IsDetached]] true
 
 Running test: testArrayBufferWithBrokenUintCtor
-  [[Int8Array]] own object undefined
-  [[Uint8Array]] own object undefined
   __proto__ own object undefined
 Internal properties
-  [[IsDetached]] boolean false
+  [[Int8Array]] object undefined
+  [[Uint8Array]] object undefined

test/inspector/runtime/get-properties.js

@@ -51,7 +51,8 @@ InspectorTest.runAsyncTestSuite([
       await logGetPropertiesResult(prop.value.objectId);
     }
     for (let prop of props.result.internalProperties) {
-      InspectorTest.log(prop.name + ' ' + prop.value.value);
+      InspectorTest.log(prop.name);
+      await logGetPropertiesResult(prop.value.objectId);
     }
   },