Bug 216062 – [JSC] Add missing detached buffer errors for DataView

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ross Kirsling]

[JSC] Add missing detached buffer errors for ArrayBuffer and DataView

[Ross Kirsling]

Created attachment 407730 [details]
Patch

[Ross Kirsling]

Created attachment 407731 [details]
Patch

[Yusuke Suzuki]

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)).

[Ross Kirsling]

Created attachment 407739 [details]
Patch

[Ross Kirsling]

Created attachment 407782 [details]
Patch for landing

[Ross Kirsling]

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.

[Ross Kirsling]

Created attachment 407791 [details]
Patch for landing

[Ross Kirsling]

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.

[Ross Kirsling]

(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.

[Ross Kirsling]

Created attachment 407848 [details]
Patch for landing

[EWS]

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

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

[Radar WebKit Bug Importer]

<rdar://problem/68285725>