script: Rework on dictionary conversion in SubtleCrypto#43519
Merged
Conversation
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>
Member
Author
|
cc @Taym95 Eventually the fix doesn't need to include any change in |
TimvdLippe
approved these changes
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Our existing
SubtleCrypto::normalize_algorithmimplementation converts the input dictionary from a JavaScript object to a Rust struct twice. The first conversion (Step 2 to 4) under WebIDL dictionaryAlgorithmis for retrieving thenameproperty 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
nameproperty 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 thenameproperty 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.servo/tests/wpt/tests/WebCryptoAPI/digest/cshake.tentative.https.any.js
Lines 215 to 219 in 5bb4f9f
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_objectin Step 9 to 10 so that we don't need to read thenameproperty and trigger the getter method again.Second, we no longer use
dictionary_from_jsvalfor converting the dictionary, since it reads the whole dictionary including thenameproperty, 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 thenameproperty 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.