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.

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

Unused prototype objects from V8 FunctionTemplate initialization #17668

Closed
TimothyGu opened this issue Dec 14, 2017 · 5 comments
Closed

Unused prototype objects from V8 FunctionTemplate initialization #17668

TimothyGu opened this issue Dec 14, 2017 · 5 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.

Comments

@TimothyGu
Copy link
Member

  • Version: all
  • Platform: all
  • Subsystem: src
> typeof process.binding('udp_wrap').UDP.prototype.bind6.prototype
'object'

We don't need these prototypes, and we can get rid of them either through v8::FunctionTemplate::RemovePrototype() or indirectly, by specifying v8::ConstructorBehavior::kThrow when creating the v8::FunctionTemplate.

@TimothyGu TimothyGu added the c++ Issues and PRs that require attention from people who are familiar with C++. label Dec 14, 2017
@maclover7 maclover7 added the discuss Issues opened for discussions and feedbacks. label Dec 25, 2017
@Trott Trott removed the discuss Issues opened for discussions and feedbacks. label Mar 11, 2018
@apapirovski apapirovski added the help wanted Issues that need assistance from volunteers or PRs that need help to proceed. label Apr 12, 2018
@brandontruggles
Copy link
Contributor

brandontruggles commented Apr 12, 2018

Hello, I'd like to work on this! Any tips on where I should look?

@addaleax
Copy link
Member

@brandonrninefive I think Environment::SetMethod, Environment::SetProtoMethod and Environment::SetTemplateMethod in env-inl.h are the relevant places where this would apply.

@brandontruggles
Copy link
Contributor

@addaleax Great! Thank you. I think I've figured out how this works. The udp_wrap binding and its associated prototypes/functions are defined within udp_wrap.cc. From there, I can use RemovePrototype() on FunctionTemplate objects to remove their prototype objects. Furthermore, I can use the Environment methods that you listed above to define methods outside or within prototype objects.

For clarification regarding this exact issue, am I just getting rid of the last prototype object listed in
> typeof process.binding('udp_wrap').UDP.prototype.bind6.prototype ?

Or am getting rid of that prototype and also the prototype object after UDP, and replacing its prototype methods with regular methods defined either by Environment::SetMethod or Environment::SetTemplateMethod?

I appreciate the help, and please let me know if any of my above conclusions are incorrect.

@TimothyGu
Copy link
Member Author

TimothyGu commented Apr 20, 2018

@brandonrninefive My recommendation is that you do not use RemovePrototype(), instead using v8::ConstructorBehavior::kThrow when creating such methods in Environment::SetProtoMethod. (You will need to tweak Environment::NewFunctionTemplate() (decl, impl) to accept a third optional parameter of type v8::ConstructorBehavior, to allow passing this information to V8.) There are several reasons for this:

  • This way, not only udp_wrap's classes are fixed but also other bindings. In the original issue, I only used udp_wrap as an example. The same issue applies to all other bindings.
  • As a bonus, this would make new (process.binding('udp_wrap').UDP.prototype.bind6)() throw. One should never do something like that in the first place, and since process.binding is considered internal API this makes the code more robust.

With regards to your last question, you are only getting rid of the last prototype. If you look at built-in methods like Number.prototype.toString, you will notice that they don't have a prototype property. That's what we are trying to accomplish here as well.

@brandontruggles
Copy link
Contributor

@TimothyGu Thank you for all the tips! I'm working on implementing this now, and I should have a PR ready over the next few days.

brandontruggles added a commit to brandontruggles/node that referenced this issue Apr 29, 2018
Added an optional parameter of type v8::ConstructorBehavior to
Environment::NewFunctionTemplate, defaulting to
v8::ConstructorBehavior::kAllow. Also modified
Environment::SetProtoMethod to pass
v8::ConstructorBehavior::kThrow to its
call to Environment::NewFunctionTemplate.

Fixes: nodejs#17668
Refs: nodejs#20321
MylesBorins pushed a commit that referenced this issue May 8, 2018
Added an optional parameter of type v8::ConstructorBehavior to
Environment::NewFunctionTemplate, defaulting to
v8::ConstructorBehavior::kAllow. Also modified
Environment::SetProtoMethod to pass
v8::ConstructorBehavior::kThrow to its
call to Environment::NewFunctionTemplate.

Fixes: #17668
Refs: #20321
PR-URL: #20321
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this issue May 8, 2018
Added an optional parameter of type v8::ConstructorBehavior to
Environment::NewFunctionTemplate, defaulting to
v8::ConstructorBehavior::kAllow. Also modified
Environment::SetProtoMethod to pass
v8::ConstructorBehavior::kThrow to its
call to Environment::NewFunctionTemplate.

Fixes: #17668
Refs: #20321
PR-URL: #20321
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
MylesBorins pushed a commit that referenced this issue May 9, 2018
Added an optional parameter of type v8::ConstructorBehavior to
Environment::NewFunctionTemplate, defaulting to
v8::ConstructorBehavior::kAllow. Also modified
Environment::SetProtoMethod to pass
v8::ConstructorBehavior::kThrow to its
call to Environment::NewFunctionTemplate.

Fixes: #17668
Refs: #20321
PR-URL: #20321
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. help wanted Issues that need assistance from volunteers or PRs that need help to proceed.
Projects
None yet
Development

No branches or pull requests

6 participants