Skip to content

script: Rework on dictionary conversion in SubtleCrypto#43519

Merged
TimvdLippe merged 4 commits into
servo:mainfrom
kkoyung:check-detached-2
Mar 21, 2026
Merged

script: Rework on dictionary conversion in SubtleCrypto#43519
TimvdLippe merged 4 commits into
servo:mainfrom
kkoyung:check-detached-2

Conversation

@kkoyung

@kkoyung kkoyung commented Mar 21, 2026

Copy link
Copy Markdown
Member

Our existing SubtleCrypto::normalize_algorithm implementation converts the input dictionary from a JavaScript object to a Rust struct twice. The first conversion (Step 2 to 4) under WebIDL dictionary Algorithm is for retrieving the name property to determine the desired dictionary type, and the second conversion (Step 9 to 10) is for retrieving the whole dictionary of desired WebIDL dictionary type.

If the name property of the input dictionary is a JavaScript getter method, the getter will be triggered twice and leads to some undesired side effects. For example, WPT contains some tests that the name property is a getter method which transfers a buffer source. Triggering it twice leads to a TypeError of accessing detached buffer at the second time. Those tests then fail.

get name() {
// Transfer the buffer while calling digest
buffer.buffer.transfer();
return alg;
},

This patch includes two changes to fix this problem.

First, the algorithm name retrieved in Step 2 to 4 is now passed to Op::RegisteredAlgorithm::from_object in Step 9 to 10 so that we don't need to read the name property and trigger the getter method again.

Second, we no longer use dictionary_from_jsval for converting the dictionary, since it reads the whole dictionary including the name property, triggering the getter method again. Instead, we add some custom helper functions (get_optional_parameter, get_required_parameter, get_optional_parameter_in_box, get_required_parameter_in_box, get_optional_buffer_source, get_required_buffer_source) to manually read properties other the name property one by one, in a way that matches the specification.

Hence, each property of the input dictionary will only be read once, to avoid this issue.

Testing: Pass some WPT tests that were expected to fail.

kkoyung added 4 commits March 21, 2026 14:21
Signed-off-by: Kingsley Yung <kingsley@kkoyung.dev>
Signed-off-by: Kingsley Yung <kingsley@kkoyung.dev>
Signed-off-by: Kingsley Yung <kingsley@kkoyung.dev>
Signed-off-by: Kingsley Yung <kingsley@kkoyung.dev>
@kkoyung kkoyung requested a review from gterzian as a code owner March 21, 2026 10:00
@servo-highfive servo-highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 21, 2026
@kkoyung

kkoyung commented Mar 21, 2026

Copy link
Copy Markdown
Member Author

cc @Taym95 Eventually the fix doesn't need to include any change in buffer_source.rs since the to_vec approach with ArrayBufferViewOrArrayBuffer already does a great job.

@TimvdLippe TimvdLippe added this pull request to the merge queue Mar 21, 2026
@servo-highfive servo-highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 21, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Mar 21, 2026
@servo-highfive servo-highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Mar 21, 2026
@TimvdLippe TimvdLippe added this pull request to the merge queue Mar 21, 2026
@servo-highfive servo-highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Mar 21, 2026
Merged via the queue into servo:main with commit c23d888 Mar 21, 2026
39 checks passed
@servo-highfive servo-highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 21, 2026
Gae24 pushed a commit to Gae24/servo that referenced this pull request Mar 26, 2026
Our existing `SubtleCrypto::normalize_algorithm` implementation converts
the input dictionary from a JavaScript object to a Rust struct twice.
The first conversion (Step 2 to 4) under WebIDL dictionary `Algorithm`
is for retrieving the `name` property to determine the desired
dictionary type, and the second conversion (Step 9 to 10) is for
retrieving the whole dictionary of desired WebIDL dictionary type.

If the `name` property of the input dictionary is a JavaScript getter
method, the getter will be triggered twice and leads to some undesired
side effects. For example, WPT contains some tests that the `name`
property is a getter method which transfers a buffer source. Triggering
it twice leads to a TypeError of accessing detached buffer at the second
time. Those tests then fail.


https://github.com/servo/servo/blob/5bb4f9f103e629fe38ea28bc67a8543a00cc6e60/tests/wpt/tests/WebCryptoAPI/digest/cshake.tentative.https.any.js#L215-L219

This patch includes two changes to fix this problem.

First, the algorithm name retrieved in Step 2 to 4 is now passed to
`Op::RegisteredAlgorithm::from_object` in Step 9 to 10 so that we don't
need to read the `name` property and trigger the getter method again.

Second, we no longer use `dictionary_from_jsval` for converting the
dictionary, since it reads the whole dictionary including the `name`
property, triggering the getter method again. Instead, we add some
custom helper functions (`get_optional_parameter`,
`get_required_parameter`, `get_optional_parameter_in_box`,
`get_required_parameter_in_box`, `get_optional_buffer_source`,
`get_required_buffer_source`) to manually read properties other the
`name` property one by one, in a way that matches the specification.

Hence, each property of the input dictionary will only be read once, to
avoid this issue.

Testing: Pass some WPT tests that were expected to fail.

---------

Signed-off-by: Kingsley Yung <kingsley@kkoyung.dev>
@kkoyung kkoyung deleted the check-detached-2 branch April 17, 2026 10:03
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