Closed Bug 1673867 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, task, P3)

Product:
Core
Component:
JavaScript Engine
Type:
task

Tracking

()

Status:
RESOLVED FIXED
Milestone:
84 Branch
Tracking Flags:
Tracking Status
firefox84 --- fixed

People

(Reporter: jandem, Assigned: jandem)

References

Details

Attachments

(10 files)

Bug 1673867 part 1 - Use CheckArrayBufferTooLarge more. r?sfink!
(deleted), text/x-phabricator-request
Details
Bug 1673867 part 2 - Use MaxBufferByteLength instead of INT32_MAX in SAB code. r?lth!
(deleted), text/x-phabricator-request
Details
Bug 1673867 part 3 - Simplify OutlineTypedObject::attach. r?lth!
(deleted), text/x-phabricator-request
Details
Bug 1673867 part 4 - Return BufferSize from ArrayBuffer byteLength methods. r?lth!,sfink!
(deleted), text/x-phabricator-request
Details
Bug 1673867 part 5 - Return BufferSize from ArrayBufferView byteOffset method. r?sfink!
(deleted), text/x-phabricator-request
Details
Bug 1673867 part 6 - Return BufferSize from ArrayBufferView byteLength methods. r?sfink!
(deleted), text/x-phabricator-request
Details
Bug 1673867 part 7 - Return BufferSize from TypedArray length method. r?sfink!
(deleted), text/x-phabricator-request
Details
Bug 1673867 part 8 - Use BufferSize in more ArrayBuffer code. r?lth!,sfink!
(deleted), text/x-phabricator-request
Details
Bug 1673867 part 9 - Use BufferSize in more ArrayBufferView code. r?sfink!
(deleted), text/x-phabricator-request
Details
Bug 1673867 part 10 - Remove now-redundant check in fromLength. r?sfink!
(deleted), text/x-phabricator-request
Details

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.

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

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

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

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

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

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

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

Depends on D95283

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

Depends on D95284

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

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

Pushed by jdemooij@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: