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

n-api: use Maybe version of SetPrototype #13513

Closed
wants to merge 1 commit into from
Closed

n-api: use Maybe version of SetPrototype #13513

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/node_api.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1963,8 +1963,8 @@ napi_status napi_wrap(napi_env env,

// Insert the wrapper into the object's prototype chain.
v8::Local<v8::Value> proto = obj->GetPrototype();
wrapper->SetPrototype(proto);
obj->SetPrototype(wrapper);
wrapper->SetPrototype(context, proto).ToChecked();
obj->SetPrototype(context, wrapper).ToChecked();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong to me; not your changes, but the logic itself. What happens when proto->IsNull() is true? What happens when obj is a proxy object?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when proto->IsNull() is true?

That should be fine. If it was correct for the original object to have a null prototype, then it is correct for the object inserted in the prototype chain to have a null prototype.

What happens when obj is a proxy object?

Is it not valid to set the prototype of a proxy object? If V8 doesn't allow it, then the SetPrototype() call will presumably return an empty Maybe, and this code should check for that and return an error status in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasongin Thanks for the details. @bnoordhuis Does this seems reasonable to you too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis If you get a chance could you take a look and see if you think I can land this, thanks!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point about proxy objects still stand. Since they can intercept (and fail) almost any operation, you have to be prepared for exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see. Let me take a look and come up with a suggestion. Thanks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danbev Any luck? That warning irritates me more by the day. Happy to take over if you don't have time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis Sorry, I've not had time yet so please take over if you do have time. Thanks


if (result != nullptr) {
// The returned reference should be deleted via napi_delete_reference()
Expand Down