Bug 216062

Created attachment 407730 [details]
Patch

Created attachment 407731 [details]
Patch

Comment on attachment 407731 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407731&action=review

r=me with DFG / FTL tests and `DataView.prototype.byteLength` handling in `getOwnPropertySlot` revising.

> Source/JavaScriptCore/runtime/JSArrayBufferPrototype.cpp:74
> +    auto* thisObject = jsDynamicCast<JSArrayBuffer*>(vm, callFrame->thisValue());
> +    if (!thisObject || thisObject->isShared())
> +        return throwVMTypeError(globalObject, scope, "Receiver must be an ArrayBuffer"_s);
> +    if (thisObject->impl()->isNeutered())
> +        return throwVMTypeError(globalObject, scope, "Buffer has already been detached"_s);

Can you add a test for DFG / FTL?

> Source/JavaScriptCore/runtime/JSDataViewPrototype.cpp:142
> +    if (dataView->isNeutered())
> +        return throwVMTypeError(globalObject, scope, "Underlying ArrayBuffer has been detached from the view"_s);

Can you add a test which works in DFG / FTL? Like this.
Because DFG / FTL handles DataView Get / Set functions.

function test(dataView){ dataView.getXXX(...); }

for (...)
    test(dataView);
detach(dataView);
test(dataView);

> Source/JavaScriptCore/runtime/JSDataViewPrototype.cpp:197
> +    if (dataView->isNeutered())
> +        return throwVMTypeError(globalObject, scope, "Underlying ArrayBuffer has been detached from the view"_s);

Ditto.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewConstructorInlines.h:57
> +    auto length = ViewClass::TypedArrayStorageType == TypeDataView ? 1 : 3;

We can just put it into the line below. (jsNumber(ViewClass::TypedArrayStorageType == TypeDataView ? 1 : 3)).

Created attachment 407782 [details]
Patch for landing

There is a neighboring issue here that DataView's byteLength and byteOffset getters have defined property descriptors:

  λ eshost -se "JSON.stringify(Object.getOwnPropertyDescriptor(new DataView(new ArrayBuffer(1)), 'byteLength'))"
  #### ch, sm, v8, xs
  undefined

  #### jsc
  {"value":1,"writable":false,"enumerable":false,"configurable":true}

But this is a separate matter from detached buffers and also appears to need test262 cases, so I will handle it separately.

Created attachment 407791 [details]
Patch for landing

Wow, it appears that the ArrayBuffer fix here is actually something that V8 and SM intentionally fail because it's almost certain to break web compatibility (since engines are in agreement). I'll jettison that part of this patch and bring up changing the spec in the next TC39 meeting.

(In reply to Ross Kirsling from comment #6)
> There is a neighboring issue here that DataView's byteLength and byteOffset
> getters have defined property descriptors:
> 
>   λ eshost -se "JSON.stringify(Object.getOwnPropertyDescriptor(new
> DataView(new ArrayBuffer(1)), 'byteLength'))"
>   #### ch, sm, v8, xs
>   undefined
> 
>   #### jsc
>   {"value":1,"writable":false,"enumerable":false,"configurable":true}
> 
> But this is a separate matter from detached buffers and also appears to need
> test262 cases, so I will handle it separately.

Er rather, this is just as easy to fix as it is to punt on, so I'll address this now and add test262 cases later.

Created attachment 407848 [details]
Patch for landing

Committed r266529: <https://trac.webkit.org/changeset/266529>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407848 [details].

<rdar://problem/68285725>