Skip to content
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

src: fix implementation of PropertySetterCallback #53576

Closed
wants to merge 1 commit into from
Closed

src: fix implementation of PropertySetterCallback #53576

wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Jun 25, 2024

V8 does not allow returning arbitrary values from the interceptor
setter callbacks, only a boolean return value is allowed. Since
default return value is true, it's not even necessary to set
the return value on a successful path.

Refs: https://crbug.com/348660658

This was adapted from v8#194

@isheludko @targos
src: fix implementation of PropertySetterCallback
faa464c 
V8 does not allow returning arbitrary values from the interceptor
setter callbacks, only a boolean return value is allowed. Since
default return value is `true`, it's not even necessary to set
the return value on a successful path.

Refs: https://crbug.com/348660658
@targos targos added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Jun 25, 2024
@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. vm Issues and PRs related to the vm subsystem. labels Jun 25, 2024
@targos targos added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. labels Jun 25, 2024
@aduh95
Copy link
Contributor

aduh95 commented Jun 25, 2024

V8 does not allow returning arbitrary values from the interceptor
setter callbacks

I suppose that doesn't apply to the version of V8 we're currently compiling, otherwise I don't understand how we're able to build. Or is it only a warning for now?

@targos
Copy link
Member Author

targos commented Jun 25, 2024

Maybe @isheludko has the answer?

@isheludko
Copy link
Contributor

isheludko commented Jun 25, 2024 •
edited
Loading

The call to args.GetReturnValue().Set(value); wasn't actually necessary since v8#180, it was necessary for the old interceptors Api to indicate that the request was intercepted while the actual value was ignored.

This CL is a preparation for V8 CL which actually bans misuses of the ReturnValue: https://crrev.com/c/5647894.
See https://chromium-review.googlesource.com/c/v8/v8/+/5647894/13/include/v8-function-callback.h.
When v8_imminent_deprecation_warnings gn arg is true then the v8::ReturnValue<void>::Set(Local<S>) methods will trigger a static_assert failure with a descriptive message.

Heads-up: at some point V8 will support returning a boolean value from Setter/Definer callbacks for real : https://crbug.com/348660658.

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

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

@nodejs-github-bot
Copy link
Collaborator

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

@targos targos added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 10, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 10, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in e849dd6...eaffed6

nodejs-github-bot pushed a commit that referenced this pull request Jul 10, 2024
@isheludko @nodejs-github-bot
src: fix implementation of PropertySetterCallback
eaffed6 
V8 does not allow returning arbitrary values from the interceptor
setter callbacks, only a boolean return value is allowed. Since
default return value is `true`, it's not even necessary to set
the return value on a successful path.

Refs: https://crbug.com/348660658
PR-URL: #53576
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
@targos targos deleted the property-setter-callback branch July 10, 2024 07:35
aduh95 pushed a commit that referenced this pull request Jul 12, 2024
@isheludko @aduh95
src: fix implementation of PropertySetterCallback
071bf5e 
V8 does not allow returning arbitrary values from the interceptor
setter callbacks, only a boolean return value is allowed. Since
default return value is `true`, it's not even necessary to set
the return value on a successful path.

Refs: https://crbug.com/348660658
PR-URL: #53576
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
@aduh95 aduh95 mentioned this pull request Jul 12, 2024
Merged
aduh95 pushed a commit that referenced this pull request Jul 16, 2024
@isheludko @aduh95
src: fix implementation of PropertySetterCallback
9bde9b2 
V8 does not allow returning arbitrary values from the interceptor
setter callbacks, only a boolean return value is allowed. Since
default return value is `true`, it's not even necessary to set
the return value on a successful path.

Refs: https://crbug.com/348660658
PR-URL: #53576
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
ehsankhfr pushed a commit to ehsankhfr/node that referenced this pull request Jul 18, 2024
@isheludko @ehsankhfr
src: fix implementation of PropertySetterCallback
5d675ee 
V8 does not allow returning arbitrary values from the interceptor
setter callbacks, only a boolean return value is allowed. Since
default return value is `true`, it's not even necessary to set
the return value on a successful path.

Refs: https://crbug.com/348660658
PR-URL: nodejs#53576
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Mohammed Keyvanzadeh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VoltrexKeyva VoltrexKeyva VoltrexKeyva approved these changes

@legendecas legendecas legendecas approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@targos @aduh95 @isheludko @nodejs-github-bot @lpinca @legendecas @VoltrexKeyva