script: Re-implement evaluate_key_path_on_value in IndexedDB#38847
Merged
jdm merged 14 commits intoservo:mainfrom Sep 5, 2025
Merged
script: Re-implement evaluate_key_path_on_value in IndexedDB#38847jdm merged 14 commits intoservo:mainfrom
jdm merged 14 commits intoservo:mainfrom
Conversation
Member
Author
|
cc @arihant2math (Overlapped with #38835) |
Taym95
reviewed
Aug 22, 2025
arihant2math
approved these changes
Aug 22, 2025
|
🔨 Triggering try run (#17163441907) for Linux (WPT) |
|
Test results for linux-wpt from try job (#17163441907): Flaky unexpected result (17)
Stable unexpected results that are known to be intermittent (20)
Stable unexpected results (4)
|
|
|
|
🔨 Triggering try run (#17170731753) for Linux (WPT) |
|
Test results for linux-wpt from try job (#17170731753): Flaky unexpected result (25)
Stable unexpected results that are known to be intermittent (20)
|
|
✨ Try run (#17170731753) succeeded. |
jdm
reviewed
Aug 27, 2025
| @@ -1,5 +1,5 @@ | |||
| [idb-binary-key-detached.any.html] | |||
| expected: TIMEOUT | |||
| expected: CRASH | |||
Member
There was a problem hiding this comment.
Do we know why these tests crash now?
Member
Author
There was a problem hiding this comment.
This test panics when we call structuredclone::write here:
servo/components/script/indexed_db.rs
Lines 146 to 150 in 0089e65
which is used to handle this part of the spec:
If input is a buffer source type
- Let octets be the result of running the steps to get a copy of the bytes held by the buffer source input. Rethrow any exceptions.
- Return a new key with type binary and value octets.
My guess is that we didn't run the get-a-copy-of-the-bytes-held-by-the-buffer-source algorithm in the first step, which help us check whether the key is a detached buffer. The WPT test file has a spec URL (https://webidl.spec.whatwg.org/#dfn-get-buffer-source-copy) directly pointing to this algorithm.
This PR only eliminates crashes caused by the previously unimplemented part of evaluate_key_path_on_value. This explains why this PR can't fix this test.
Member
Author
There was a problem hiding this comment.
Yes, it would be better to use ? instead of expect. However, since this PR isn't related to convert_value_to_key function, I'd suggest fixing it in a separate PR.
Contributor
There was a problem hiding this comment.
I've done this in #39078. The expectations should be updated when you get a chance.
This was referenced Sep 1, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
Sep 2, 2025
…ure (#39078) Related to failures in #38847 Testing: WPT --------- Signed-off-by: Ashwin Naren <[email protected]>
kkoyung
force-pushed
the
idb-extract-key-from-vale-using-key-path
branch
from
05259e8 to
1199449
Compare
Member
Author
|
Rebased on the |
|
🔨 Triggering try run (#17394153815) for Linux (WPT) |
|
Test results for linux-wpt from try job (#17394153815): Flaky unexpected result (22)
Stable unexpected results that are known to be intermittent (27)
|
|
✨ Try run (#17394153815) succeeded. |
Contributor
|
@kkoyung there seem to be merge conflicts at the moment. |
Signed-off-by: Kingsley Yung <[email protected]>
Signed-off-by: Kingsley Yung <[email protected]>
Signed-off-by: Kingsley Yung <[email protected]>
Signed-off-by: Kingsley Yung <[email protected]>
Signed-off-by: Kingsley Yung <[email protected]>
kkoyung
force-pushed
the
idb-extract-key-from-vale-using-key-path
branch
from
1199449 to
7716c7b
Compare
Member
Author
Rebased to resolve the conflict. |
jdm
requested changes
Sep 4, 2025
Member
jdm
left a comment
jdm
left a comment
There was a problem hiding this comment.
Looking good; we just have a few areas where we can get closer to the specification!
components/script/indexed_db.rs
Outdated
| // Step 1.3.6. Assert: status is true. | ||
| unsafe { | ||
| set_dictionary_property(*cx, result.handle(), &i.to_string(), key.handle()) | ||
| .map_err(|_| Error::Data)?; |
Member
There was a problem hiding this comment.
This should be Error::JSFailed, since it's a failure inside the JS engine.
components/script/indexed_db.rs
Outdated
| unsafe { | ||
| let mut is_array = false; | ||
| if IsArrayObject(*cx, current_value.handle(), &mut is_array) { | ||
| return Err(Error::Data); |
components/script/indexed_db.rs
Outdated
| } | ||
| } // All tokens processed | ||
| // Otherwise | ||
| else { |
Member
There was a problem hiding this comment.
The current structure means that any checks like root_from_handlevalue:: or root_from_handlevalue:: that fail will silently return an undefined value, rather than falling through to this else block. Can we use explicit returns for the successful branches and make this else block unconditional instead?
Member
Author
There was a problem hiding this comment.
Since these if-then-else blocks are inside a for loop, I simply took away else to break up the if-then-else, and call continue when we find a matched one.
components/script/indexed_db.rs
Outdated
| unsafe { | ||
| // If Type(value) is not Object, return failure. | ||
| if !current_value.is_object() { | ||
| return Err(Error::Data); |
Member
There was a problem hiding this comment.
From the spec:
The result of these steps is an ECMAScript value or failure, or the steps may throw an exception.
We need to create a result enum like:
enum EvaluationResult {
Success,
Failure,
Error(Error),
}and this case should be EvaluationResult::Failure.
Member
Author
There was a problem hiding this comment.
How about only including Success and Failure in the enum, and making the function return Result<EvaluationResult, Error> so that the caller can easily rethrow the error? It's also more close to the spec.
Member
Author
There was a problem hiding this comment.
I found that the other functions in components/script/indexeddb_db.rs have the same issue. Since this PR only touches evaluate_key_path_on_value and extract_key functions, I fixed these two functions for now. We can fix the others separately.
components/script/indexed_db.rs
Outdated
Comment on lines
+347
to
+348
| // Let hop be ! HasOwnProperty(value, identifier). | ||
| // If hop is false, return failure. |
Member
There was a problem hiding this comment.
We're missing the HasOwnProperty check like
servo/components/script/webdriver_handlers.rs
Line 298 in aac6aa6
components/script/indexed_db.rs
Outdated
| // Let hop be ! HasOwnProperty(value, identifier). | ||
| // If hop is false, return failure. | ||
| // Let value be ! Get(value, identifier). | ||
| // If value is undefined, return failure. |
components/script/indexed_db.rs
Outdated
| *cx, | ||
| object.handle(), | ||
| identifier, | ||
| property.handle_mut(), |
Member
There was a problem hiding this comment.
We can pass current_value.handle_mut() here directly.
Signed-off-by: Kingsley Yung <[email protected]>
Signed-off-by: Kingsley Yung <[email protected]>
Signed-off-by: Kingsley Yung <[email protected]>
jdm
added
the
S-needs-code-changes
Signed-off-by: Kingsley Yung <[email protected]>
Signed-off-by: Kingsley Yung <[email protected]>
Signed-off-by: Kingsley Yung <[email protected]>
Signed-off-by: Kingsley Yung <[email protected]>
Member
Author
|
We call the I merged the main branch to here, to get the new function signature, and then fixed the type mismatch. |
arihant2math
added
T-linux-wpt
|
🔨 Triggering try run (#17486230829) for Linux (WPT) |
|
Test results for linux-wpt from try job (#17486230829): Flaky unexpected result (24)
Stable unexpected results that are known to be intermittent (28)
|
|
✨ Try run (#17486230829) succeeded. |
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
4 participants
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.
The current implementation of evaluate_key_path_on_value was translated from gecko, and it is incomplete. The unimplemented part occurs many crashes in WPT tests.
This PR re-implements it according to the spec. It should eliminate many crashed WPT tests, and increase the code readability.
Testing: Update WPT test expectation
Fixes: #38817 partially, and #25325