Skip to content

fix(built-ins): permit preventExtensions on fixed-length typed arrays backed by GSABs#5262

Open
AlvinThorn008 wants to merge 7 commits intoboa-dev:mainfrom
AlvinThorn008:main
Open

fix(built-ins): permit preventExtensions on fixed-length typed arrays backed by GSABs#5262
AlvinThorn008 wants to merge 7 commits intoboa-dev:mainfrom
AlvinThorn008:main

Conversation

@AlvinThorn008
Copy link

This Pull Request fixes/closes #5256.

As required by ecma262, Object.preventExtensions may be applied to typed arrays without error if the IsTypedArrayFixedLength is not false. Prior to this commit, this check was incorrectly implemented via ArrayBuffer::is_fixed_len which does not/can not include an AUTO length check. This discrepancy lies in that the requirements of a fixed-length TypedArray slightly differ from a fixed-length arraybuffer.

It adds and replaces the checks as specified by sec-istypedarrayfixedlength. Now, typed_array_exotic_prevent_extensions should

  • return Ok(false) when the TypedArray is auto length or when the backing ArrayBuffer is resizable.
  • failing the above, call ordinary_prevent_extensions if the backing array is a SharedArrayBuffer.

This PR also adds a test to ensure what the title of this issue says. In addition, test262's test/staging/built-ins/preventExtensions should now pass completely.

… by GSABs

As required by ecma262, `Object.preventExtensions` may be applied to typed arrays without error if the `IsTypedArrayFixedLength` is not false. Prior to this commit, this check was incorrectly implemented via `ArrayBuffer::is_fixed_len` which does not/can not include an AUTO length check. This discrepancy lies in that the requirements of a fixed-length `TypedArray` slightly differ from a fixed-length arraybuffer.
@AlvinThorn008 AlvinThorn008 requested a review from a team as a code owner March 26, 2026 15:31
@github-actions github-actions bot added the Waiting On Review Waiting on reviews from the maintainers label Mar 26, 2026
@github-actions github-actions bot added this to the v1.0.0 milestone Mar 26, 2026
@github-actions github-actions bot added C-Tests Issues and PRs related to the tests. C-Builtins PRs and Issues related to builtins/intrinsics labels Mar 26, 2026
Copy link
Contributor

@zhuzhu81998 zhuzhu81998 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be much nicer if you implement the IsTypedArrayFixedLength operation as a function, similar to:

/// Abstract operation `IsValidIntegerIndex ( O, index )`.
///
/// Returns `true` if the index is valid, or `false` otherwise.
///
/// More information:
/// - [ECMAScript reference][spec]
///
/// [spec]: https://tc39.es/ecma262/#sec-isvalidintegerindex
pub(crate) fn is_valid_integer_index(obj: &JsObject, index: f64) -> JsResult<bool> {
let inner = obj.downcast_ref::<TypedArray>().js_expect(
"integer indexed exotic method should only be callable from TypedArray objects",
)?;
let buf = inner.viewed_array_buffer();
let buf = buf.as_buffer();
// 1. If IsDetachedBuffer(O.[[ViewedArrayBuffer]]) is true, return false.
// 4. Let taRecord be MakeTypedArrayWithBufferWitnessRecord(O, unordered).
// 5. NOTE: Bounds checking is not a synchronizing operation when O's backing buffer is a growable SharedArrayBuffer.
let Some(buf_len) = buf.bytes(Ordering::Relaxed).map(|s| s.len()) else {
return Ok(false);
};
Ok(inner.validate_index(index, buf_len).is_some())
}

Note that you should put the spec as comment.

@AlvinThorn008
Copy link
Author

@zhuzhu81998
That is true. I debated whether it was necessary but it's certainly more in line with the codebase's style. I'll refactor it in a couple minutes.

Thanks

…e function for use in `typed_array_exotic_prevent_extensions`.
@AlvinThorn008
Copy link
Author

I've copied the format of the function you linked. Everything seems to be working as before.

/// - [ECMAScript reference][spec]
///
/// [spec]: https://tc39.es/ecma262/#sec-istypedarrayfixedlength
fn is_typed_array_fixed_length(obj: &JsObject) -> JsResult<bool> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to put this in TypedArray, such that we can simplify the name to is_fixed_length just like ArrayBuffer.is_fixed_len()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I'll do that shortly

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into a small borrowing error but it resolved now.

@github-actions
Copy link

Test262 conformance changes

Test result main count PR count difference
Total 52,963 52,963 0
Passed 50,732 50,735 +3
Ignored 1,426 1,426 0
Failed 805 802 -3
Panics 0 0 0
Conformance 95.79% 95.79% +0.01%
Fixed tests (3):
test/staging/built-ins/Reflect/preventExtensions/preventExtensions-variable-length-typed-arrays.js (previously Failed)
test/staging/built-ins/Object/preventExtensions/preventExtensions-variable-length-typed-arrays.js (previously Failed)
test/staging/built-ins/Object/seal/seal-variable-length-typed-arrays.js (previously Failed)

Tested main commit: 2437b6704c4315f6ec8f0766bf229fba23e0e61e
Tested PR commit: 28bf51bd7811a673eab6e538d17933e0eb9ad26c
Compare commits: 2437b67...28bf51b

@codecov
Copy link

codecov bot commented Mar 26, 2026

Codecov Report

❌ Patch coverage is 88.88889% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 59.75%. Comparing base (6ddc2b4) to head (28bf51b).
⚠️ Report is 919 commits behind head on main.

Files with missing lines Patch % Lines
core/engine/src/builtins/typed_array/object.rs 88.88% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #5262       +/-   ##
===========================================
+ Coverage   47.24%   59.75%   +12.50%     
===========================================
  Files         476      589      +113     
  Lines       46892    63413    +16521     
===========================================
+ Hits        22154    37890    +15736     
- Misses      24738    25523      +785     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-Builtins PRs and Issues related to builtins/intrinsics C-Tests Issues and PRs related to the tests. Waiting On Review Waiting on reviews from the maintainers

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix: permit preventExtensions on fixed length TypedArrays backed by GSABs

3 participants