Don't panic when retrieving slices from detached typed arrays

[jdm]

By using Option<NonNull<[..]>> instead of *mut [..] we can ensure that we always check for detached buffers and don't need to panic.

Testing: New unit test for detached ArrayBuffer.
Servo PR: servo/servo#46504

[sagudev]

BTW Here are some notes about this from other PR: #775 (comment)

[jdm]

I believe this is addressing that note; we now always return an empty slice for detached buffers, and servo/servo#46069 helps centralize that logic even more.

[sagudev]

I believe this is addressing that note; we now always return an empty slice for detached buffers, and servo/servo#46069 helps centralize that logic even more.

My only worry is that we collapse detached/not computed into empty slice, which can hide logical problems elsewhere (like using detached buffer) which should not occur if we follow the spec (and the spec is correctly written).

[jdm]

I believe this is addressing that note; we now always return an empty slice for detached buffers, and servo/servo#46069 helps centralize that logic even more.

My only worry is that we collapse detached/not computed into empty slice, which can hide logical problems elsewhere (like using detached buffer) which should not occur if we follow the spec (and the spec is correctly written).

I'm confused, since https://webidl.spec.whatwg.org/#dfn-get-buffer-source-copy already collapses that result. Is the concern that code that doesn't use that algorithm may have logic that treats detached array buffers differently, and we won't be able to implement that correctly?

[sagudev]

I believe this is addressing that note; we now always return an empty slice for detached buffers, and servo/servo#46069 helps centralize that logic even more.

My only worry is that we collapse detached/not computed into empty slice, which can hide logical problems elsewhere (like using detached buffer) which should not occur if we follow the spec (and the spec is correctly written).

I'm confused, since https://webidl.spec.whatwg.org/#dfn-get-buffer-source-copy already collapses that result. Is the concern that code that doesn't use that algorithm may have logic that treats detached array buffers differently, and we won't be able to implement that correctly?

Yeah, but maybe I am just paranoid. Wasn't there a website where one could find all IsDetachedBuffer in all specs?

[jdm]

I'm trying to find out what that site is on Matrix, because I have forgotten it. However, I did come across https://w3c.github.io/IndexedDB/#convert-a-value-to-a-key

[jdm]

And https://html.spec.whatwg.org/multipage/canvas.html#dom-context2d-putimagedata-common ;_;

[jdm]

Perhaps it's enough to add an API to determine if an array buffer is detached, though.

[sagudev]

Perhaps it's enough to add an API to determine if an array buffer is detached, though.

but this bring us again to

can hide logical problems elsewhere (like using detached buffer) which should not occur if we follow the spec (and the spec is correctly written).

I consider such crashes as good as one could easily see what is wrong (missing detached buffer check) and how it needs to be fixed.

[jdm]

I don't like relying on a panic for this, because it only manifests if test code or a webpage triggers it. If we want to force callers to consider this case, we should change as_slice/as_slice_mut to return an enum instead. servo/servo#46069 means that won't be a huge annoyance for callsites any more.

[sagudev]

I don't like relying on a panic for this, because it only manifests if test code or a webpage triggers it.

Fair.

If we want to force callers to consider this case, we should change as_slice/as_slice_mut to return an enum instead. servo/servo#46069 means that won't be a huge annoyance for callsites any more.

That would be ideal.

[TimvdLippe]

Yeah, but maybe I am just paranoid. Wasn't there a website where one could find all IsDetachedBuffer in all specs?

Yes that website is https://dontcallmedom.github.io/webdex/i.html although I don't see it listed

[sagudev]

Yeah, but maybe I am just paranoid. Wasn't there a website where one could find all IsDetachedBuffer in all specs?

Yes that website is https://dontcallmedom.github.io/webdex/i.html although I don't see it listed

Thanks, bookmarked. It's here: https://dontcallmedom.github.io/webdex/i.html#IsDetachedBuffer%40%40ecmascript%25%25abstract-op although references are very vague.