Skip to content

Conversation

me-it-is
Copy link

@me-it-is me-it-is commented Sep 8, 2025

when constructing WebAssembly.Memory if initial is greater than maximum a range error will be thrown.

fixes
https://wpt.fyi/results/wasm/jsapi/memory/constructor.any.worker.html?product=ladybird Initial value exceeds maximum

@me-it-is me-it-is requested a review from alimpfard as a code owner September 8, 2025 00:57
when constructing WebAssembly.Memory if initial is greater than maximum
a range error will be thrown.

fixes
https://wpt.fyi/results/wasm/jsapi/memory/constructor.any.worker.html?product=ladybird Initial value exceeds maximum
@me-it-is me-it-is force-pushed the fix-construct-out-of-range branch from 90378aa to 5873129 Compare September 8, 2025 01:04
@me-it-is
Copy link
Author

me-it-is commented Sep 8, 2025

It seems like the sanitizers failing is unrelated to this pr.

Comment on lines +26 to +31
// 3. If maximum is not empty and maximum < initial, throw a RangeError exception.
if (descriptor.maximum.has_value()) {
if (descriptor.maximum.value() < descriptor.initial) {
return vm.throw_completion<JS::RangeError>("Initial is larger than maximum."sv);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need to nest these conditions:

if (descriptor.maximum.has_value() && descriptor.maximum.value() < descriptor.initial)
    return vm.throw_completion<JS::RangeError>("Initial is larger than maximum."sv);

Copy link
Contributor

@alimpfard alimpfard left a comment

Choose a reason for hiding this comment

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

Jelle already commented the one useful thing I intended to say, so here's the nitpick:

@@ -23,6 +23,13 @@ WebIDL::ExceptionOr<GC::Ref<Memory>> Memory::construct_impl(JS::Realm& realm, Me
auto& vm = realm.vm();
Copy link
Contributor

@alimpfard alimpfard Sep 9, 2025

Choose a reason for hiding this comment

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

Please rewrap the commit message and use proper capitalisation.
(maybe something like "Fixes the 'Initial value exceeds maximum' test in\n<that url>"?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants