Skip to content

[v22.x] src: handle errors correctly in webstorage #54683

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
targos wants to merge 1 commit into from
Closed

[v22.x] src: handle errors correctly in webstorage #54683

targos wants to merge 1 commit into from

Conversation

@targos
Copy link
Member

@targos targos commented Sep 1, 2024

No description provided.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. labels Sep 1, 2024
@targos targos mentioned this pull request Sep 1, 2024
Merged
targos
targos commented Sep 1, 2024
View reviewed changes
@targos targos marked this pull request as draft September 1, 2024 09:38
@targos targos force-pushed the backport-54544-v22 branch from d8416b7 to 170a9ae Compare September 5, 2024 16:07
@targos
Copy link
Member Author

targos commented Sep 5, 2024 •
edited
Loading

I fixed the crash with a fallback to undefined in the interceptor, but I think there's a bug in V8 because that's what the documentation says:

 * If the interceptor handles the request (i.e. the property should not be
 * looked up beyond the interceptor) it should
 *  - (optionally) use info.GetReturnValue().Set()` to set the return value
 *    (by default the result is set to v8::Undefined),
 *  - return `Intercepted::kYes`.
 * If the interceptor does not handle the request it must return
 * `Intercepted::kNo` and it must not produce side effects.

@targos targos marked this pull request as ready for review September 5, 2024 16:09
@targos
Copy link
Member Author

targos commented Sep 6, 2024

I'm not sure what to do with the coverage job. It pretends that JS coverage went down, but this PR doesn't touch any JS file.

@targos
src: handle errors correctly in webstorage
87ece12 
PR-URL: nodejs#54544
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
@targos
Copy link
Member Author

targos commented Sep 16, 2024

Rebased

@targos
Copy link
Member Author

targos commented Sep 17, 2024

#54966 is also affected by the coverage drop

@targos
Copy link
Member Author

targos commented Sep 19, 2024

@RafaelGSS Should I open an issue for the coverage? The difference between v22.x and main is non-negligible.

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 5, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Oct 5, 2024

CI: https://ci.nodejs.org/job/node-test-pull-request/62925/

targos added a commit that referenced this pull request Oct 6, 2024
@targos
src: handle errors correctly in webstorage
0999949 
PR-URL: #54544
Backport-PR-URL: #54683
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Mohammed Keyvanzadeh <mohammadkeyvanzade94@gmail.com>
@targos
Copy link
Member Author

targos commented Oct 6, 2024

Landed in 0999949

@targos targos closed this Oct 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@targos @nodejs-github-bot @jasnell