1079853 - Throw on indexing into detached ArrayBuffers instead of returning `undefined`

Status: NEW

Description

[Till Schneidereit [:till]]

Section 9.4.5.8, step 5 now specifies that indexing should throw instead of failing silently on detached buffers.

I have my doubts regarding the web compatibility of this change. Ideally we should get telemetry data on this first.

Comment 1

[Tom Schuster [:evilpie]]

Edge actually throws for at least [[Get]], [[Set]] and [[DefineProperty] on detached arrays. So it seems like this is something we could actually implement.

Comment 2

[Tom Schuster [:evilpie]]

Created attachment 8989041 [details] [diff] [review]
patch

So, this was basically my first idea, throwing for typed arrays lookups in LookupOwnPropertyInline. I think this would basically work, but is so far from how the spec works, so I am not sure if this will always be correct. I feel like it would be better to do this more explicitly. André what do you think?

In IonMonkey LoadTypedArrayElementHole and StoreTypedArrayElementHole need code on the OOB to fail when the array is detached.

In CacheIR at least StoreTypedElement, LoadTypedElementExistsResult.

Comment 3

[Tom Schuster [:evilpie]]

This patch would violate the execution order defined in the spec, so we need to instead manually patch all the different operations.

Comment 4

[Tom Schuster [:evilpie]]

Created attachment 8997606 [details] [diff] [review]
Patch v2

This is a new approach. I added special handling for typed arrays to Get, Set, Has, GetOwnPropertyDescriptor and disallowed typed array indexes in LookupOwnPropertyInline.

While this handles probably handles like 90% of the code, there is _a lot_ of code that ends up calling LookupOwnPropertyInline. For example direct callers of  NativeLookupOwnProperty, like HasOwnProperty.

I am really not sure what the best way forward is here.

Comment 5

[Tom Schuster [:evilpie]]

Comment on attachment 8997606 [details] [diff] [review]
Patch v2

Afte

Comment 6

[Tom Schuster [:evilpie]]

After sleeping on this for a bit I realized we should actually be okay with throwing for detached buffers in LookupOwnProperty descriptor! Only [[Set]] and [[DefineOwnProperty]] don't check for IsDetachedBuffer in their first observable step. So in theory it should be ok to keep the custom implementation for those two and remove all the other ones and just start throwing.

Comment 7

[Tom Schuster [:evilpie]]

I think I found another interesting difference between our implementation and the spec: Because we set [[ArrayLength]] to zero, stuff like Reflect.ownKeys() will return an empty array, but from my understanding the Spec wants those to still be returned.

Comment 8

[André Bargull [:anba]]

Correct.

There are actually bad people who know and abuse this difference (bug 1381474, bug 1381469). :-) *goes away whistling*