1673867 - Change ArrayBuffer{View} offset/length accessors to return a wrapped size_t

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P3)

Description

[Jan de Mooij [:jandem]]

Bug 1673604 makes it possible for the slots to contain values larger than INT32_MAX, but many places dealing with array buffers/views are still using uint32_t or make similar assumptions about the maximum value.

Goal for this bug is to add a class that wraps a size_t and has two methods: get() that just returns the value (for callers that don't need any work) and deprecatedGetUint32 that asserts the value fits in an int32_t (for callers that need to be fixed or audited still). We can then return that from the length/offset accessors to mark the places that still need work.

Comment 1

[Lars T Hansen [:lth]]

It would be best if you keep changes in wasm/ separate, as I have another patch there.

Comment 2

[Jan de Mooij [:jandem]]

(In reply to Lars T Hansen [:lth] from comment #1)

It would be best if you keep changes in wasm/ separate, as I have another patch there.

Sorry I can't easily separate this, I'm touching every caller of byteLength() etc because the return type changes...

Comment 3

[Lars T Hansen [:lth]]

(In reply to Jan de Mooij [:jandem] from comment #2)

(In reply to Lars T Hansen [:lth] from comment #1)

It would be best if you keep changes in wasm/ separate, as I have another patch there.

Sorry I can't easily separate this, I'm touching every caller of byteLength() etc because the return type changes...

Be that as it may, those changes can't land... I guess it's a discussion for another day.

Comment 4

[Lars T Hansen [:lth]]

I guess what I should say is, I will probably land my patch tomorrow and at that point the places you need to update will have changed (and will all have moved into the API layer in WasmJS.cpp, and there will be fewer).

Comment 5

[Jan de Mooij [:jandem]]

We chatted about this. The Wasm patch will land soon, I can just rebase over that.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1673867 part 1 - Use CheckArrayBufferTooLarge more. r?sfink!

Note that the argument changes from uint32_t to uint64_t because that's what the
new caller has.

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1673867 part 2 - Use MaxBufferByteLength instead of INT32_MAX in SAB code. r?lth!

Depends on D95282

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1673867 part 3 - Simplify OutlineTypedObject::attach. r?lth!

Delete an unused overload. Remove the offset argument from the other.

Depends on D95283

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1673867 part 4 - Return BufferSize from ArrayBuffer byteLength methods. r?lth!,sfink!

Most places use the deprecatedGetUint32 method so it's clear they still need to be
audited, tested and/or converted.

Depends on D95284

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Bug 1673867 part 5 - Return BufferSize from ArrayBufferView byteOffset method. r?sfink!

Depends on D95285

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Bug 1673867 part 6 - Return BufferSize from ArrayBufferView byteLength methods. r?sfink!

Note that get() is used in a few places where we know the data is stored inline
in the object so can't be very large.

Depends on D95286

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Bug 1673867 part 7 - Return BufferSize from TypedArray length method. r?sfink!

Depends on D95287

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Bug 1673867 part 8 - Use BufferSize in more ArrayBuffer code. r?lth!,sfink!

Depends on D95288

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Bug 1673867 part 9 - Use BufferSize in more ArrayBufferView code. r?sfink!

Depends on D95289

Comment 15

[Jan de Mooij [:jandem]]

Attachment: Bug 1673867 part 10 - Remove now-redundant check in fromLength. r?sfink!

The previous patch changed maybeCreateArrayBuffer to take uint64_t instead of
uint32_t so we no longer have to guard against uint32_t overflow first.

Depends on D95290

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0ce1bd55470
part 1 - Use CheckArrayBufferTooLarge more. r=sfink
https://hg.mozilla.org/integration/autoland/rev/90c6caf4d55f
part 2 - Use MaxBufferByteLength instead of INT32_MAX in SAB code. r=lth
https://hg.mozilla.org/integration/autoland/rev/39eaaa0f1dc8
part 3 - Simplify OutlineTypedObject::attach. r=lth
https://hg.mozilla.org/integration/autoland/rev/a41ca50a7bac
part 4 - Return BufferSize from ArrayBuffer byteLength methods. r=lth,sfink
https://hg.mozilla.org/integration/autoland/rev/70220d6a4792
part 5 - Return BufferSize from ArrayBufferView byteOffset method. r=sfink
https://hg.mozilla.org/integration/autoland/rev/0774accaf945
part 6 - Return BufferSize from ArrayBufferView byteLength methods. r=sfink
https://hg.mozilla.org/integration/autoland/rev/a637c801470b
part 7 - Return BufferSize from TypedArray length method. r=sfink
https://hg.mozilla.org/integration/autoland/rev/19108edf0f70
part 8 - Use BufferSize in more ArrayBuffer code. r=lth,sfink
https://hg.mozilla.org/integration/autoland/rev/487d37f789a2
part 9 - Use BufferSize in more ArrayBufferView code. r=sfink
https://hg.mozilla.org/integration/autoland/rev/afc5cf162b5a
part 10 - Remove now-redundant check in fromLength. r=sfink

Comment 17

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/b0ce1bd55470
https://hg.mozilla.org/mozilla-central/rev/90c6caf4d55f
https://hg.mozilla.org/mozilla-central/rev/39eaaa0f1dc8
https://hg.mozilla.org/mozilla-central/rev/a41ca50a7bac
https://hg.mozilla.org/mozilla-central/rev/70220d6a4792
https://hg.mozilla.org/mozilla-central/rev/0774accaf945
https://hg.mozilla.org/mozilla-central/rev/a637c801470b
https://hg.mozilla.org/mozilla-central/rev/19108edf0f70
https://hg.mozilla.org/mozilla-central/rev/487d37f789a2
https://hg.mozilla.org/mozilla-central/rev/afc5cf162b5a