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

Calls to v8::Template::Set must not pass in non-primitive values #6216

Closed
jeisinger opened this issue Apr 15, 2016 · 6 comments
Closed

Calls to v8::Template::Set must not pass in non-primitive values #6216

jeisinger opened this issue Apr 15, 2016 · 6 comments
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency.

Comments

@jeisinger
Copy link
Contributor

Steps to repro: apply https://codereview.chromium.org/1839983002/ to v8, attempt to run node.js

Templates are globally shared objects, if a template references a non-primitive value (i.e. anything but numbers, strings, or other templates), two things happen

  • the non-primitive value will never day and so will the context it was created in, or anything referenced from that context
  • all contexts the template is instantiated in can access each other via the shared non-primitive value

To fix this, you can either introduce an accessor with the same name, and have the getter return the value (Template::SetAccessorProperty), or you can install a native data property. That will look like a regular value to JS, but under the hood, a getter is invoked (Template::SetNativeDataProperty)

/cc @ofrobots @nodejs/v8
@ChALkeR ChALkeR added c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency. labels Apr 15, 2016
@ofrobots
Copy link
Contributor

I can investigate this next week. If anyone else is interested in investigating, feel free (but let me know).

@bnoordhuis
Copy link
Member

I checked earlier today. It looks like it's fairly straightforward to fix in core (don't know yet how or if it affects performance) but it's going to create a lot of fallout in the module ecosystem. Again. Le sigh.

@jeisinger
Copy link
Contributor Author

Yeah, if only node had a proper API ...

@Fishrock123 Fishrock123 added this to the 7.0.0 milestone Apr 15, 2016
@bnoordhuis bnoordhuis mentioned this issue Apr 15, 2016
Merged
@bnoordhuis
Copy link
Member

#6228

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Apr 18, 2016
@bnoordhuis
src: don't set non-primitive values on templates
642076f 
V8 is going to disallow non-primitive values on v8::FunctionTemplate
and v8::ObjectTemplate because those can't be shared across contexts.

Fixes: nodejs#6216
PR-URL: nodejs#6228
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
@Fishrock123
Copy link
Contributor

When is the patch that causes this landing? Not in v6 right?

@Fishrock123 Fishrock123 removed this from the 7.0.0 milestone Apr 19, 2016
@jeisinger
Copy link
Contributor Author

It's part of v8 5.2 which @bnoordhuis tells me is v7 timeframe

@bnoordhuis bnoordhuis mentioned this issue Apr 19, 2016
Merged
bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Apr 22, 2016
@bnoordhuis
v8: warn in Template::Set() on improper use
7940ecf 
The next major release will make it a fatal error to use non-primitive
values in function templates and object templates.

Print a warning that includes the C and JS stack trace to tell people to
upgrade their add-ons.  The C stack trace is only printed on platforms
that support it (the BSDs, OS X and Linux+glibc.)

The warning can be disabled with the new `--nowarn_template_set` flag.

Refs: nodejs#6216
PR-URL: nodejs#6277
Reviewed-By: James M Snell <jasnell@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this issue Apr 25, 2016
@bnoordhuis @joelostrowski
src: don't set non-primitive values on templates
64176de 
V8 is going to disallow non-primitive values on v8::FunctionTemplate
and v8::ObjectTemplate because those can't be shared across contexts.

Fixes: nodejs#6216
PR-URL: nodejs#6228
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
joelostrowski pushed a commit to joelostrowski/node that referenced this issue Apr 25, 2016
@bnoordhuis @joelostrowski
v8: warn in Template::Set() on improper use
5c9bb87 
The next major release will make it a fatal error to use non-primitive
values in function templates and object templates.

Print a warning that includes the C and JS stack trace to tell people to
upgrade their add-ons.  The C stack trace is only printed on platforms
that support it (the BSDs, OS X and Linux+glibc.)

The warning can be disabled with the new `--nowarn_template_set` flag.

Refs: nodejs#6216
PR-URL: nodejs#6277
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this issue Apr 26, 2016
@bnoordhuis @jasnell
src: don't set non-primitive values on templates
2a4998c 
V8 is going to disallow non-primitive values on v8::FunctionTemplate
and v8::ObjectTemplate because those can't be shared across contexts.

Fixes: #6216
PR-URL: #6228
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
jasnell pushed a commit that referenced this issue Apr 26, 2016
@bnoordhuis @jasnell
v8: warn in Template::Set() on improper use
967a155 
The next major release will make it a fatal error to use non-primitive
values in function templates and object templates.

Print a warning that includes the C and JS stack trace to tell people to
upgrade their add-ons.  The C stack trace is only printed on platforms
that support it (the BSDs, OS X and Linux+glibc.)

The warning can be disabled with the new `--nowarn_template_set` flag.

Refs: #6216
PR-URL: #6277
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis bnoordhuis mentioned this issue Apr 26, 2016
Merged
This was referenced Apr 26, 2016
Closed
Closed
@bnoordhuis bnoordhuis mentioned this issue Apr 26, 2016
Merged
@mathiasbynens mathiasbynens mentioned this issue Apr 26, 2016
Closed
@xzyfer xzyfer mentioned this issue Apr 28, 2016
Merged
xzyfer added a commit to sass/node-sass that referenced this issue Apr 28, 2016
@xzyfer
Fix final Node 6 deprecation warnings (#1498)
ff17933 
This was partially addressed by bumping NAN in #1496.

nodejs/node#6216
https://codereview.chromium.org/1839983002/patch/1/10002

Fixes #1484
evanlucas added a commit to evanlucas/node-oniguruma that referenced this issue Apr 28, 2016
@evanlucas
src: don't use deprecated v8::Template::Set()
898fc90 
See [0] and [1]: starting with node.js v6, setting non-primitive values
on FunctionTemplate and ObjectTemplate instances is discouraged; v7 will
downright disallow it. Update the code base.

[0] nodejs/node#6216
[1] nodejs/node#6228
@evanlucas evanlucas mentioned this issue Apr 28, 2016
Merged
targos pushed a commit to targos/node that referenced this issue Jun 3, 2016
@bnoordhuis @targos
v8: warn in Template::Set() on improper use
7cb0611 
The next major release will make it a fatal error to use non-primitive
values in function templates and object templates.

Print a warning that includes the C and JS stack trace to tell people to
upgrade their add-ons.  The C stack trace is only printed on platforms
that support it (the BSDs, OS X and Linux+glibc.)

The warning can be disabled with the new `--nowarn_template_set` flag.

Refs: nodejs#6216
PR-URL: nodejs#6277
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this issue Jun 29, 2016
@bnoordhuis @targos
v8: warn in Template::Set() on improper use
35ee1d1 
The next major release will make it a fatal error to use non-primitive
values in function templates and object templates.

Print a warning that includes the C and JS stack trace to tell people to
upgrade their add-ons.  The C stack trace is only printed on platforms
that support it (the BSDs, OS X and Linux+glibc.)

The warning can be disabled with the new `--nowarn_template_set` flag.

Refs: nodejs#6216
PR-URL: nodejs#6277
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit to targos/node that referenced this issue Jul 26, 2016
@bnoordhuis @targos
v8: warn in Template::Set() on improper use
e19f71b 
The next major release will make it a fatal error to use non-primitive
values in function templates and object templates.

Print a warning that includes the C and JS stack trace to tell people to
upgrade their add-ons.  The C stack trace is only printed on platforms
that support it (the BSDs, OS X and Linux+glibc.)

The warning can be disabled with the new `--nowarn_template_set` flag.

Refs: nodejs#6216
PR-URL: nodejs#6277
Reviewed-By: James M Snell <jasnell@gmail.com>
@mkszuba mkszuba mentioned this issue Aug 10, 2016
Open
ofrobots pushed a commit to ofrobots/node that referenced this issue Aug 25, 2016
@bnoordhuis @ofrobots
v8: warn in Template::Set() on improper use
44f781d 
The next major release will make it a fatal error to use non-primitive
values in function templates and object templates.

Print a warning that includes the C and JS stack trace to tell people to
upgrade their add-ons.  The C stack trace is only printed on platforms
that support it (the BSDs, OS X and Linux+glibc.)

The warning can be disabled with the new `--nowarn_template_set` flag.

Refs: nodejs#6216
PR-URL: nodejs#6277
Reviewed-By: James M Snell <jasnell@gmail.com>
pmed added a commit to pmed/v8pp that referenced this issue Oct 5, 2016
@pmed
Add module::set_const() for another module as a read-only property
fbc1776 
According to nodejs/node#6216 only primitive values
and V8 templates are allowed to set in a v8::ObjectTemplate.
mcampa pushed a commit to mcampa/node-sass that referenced this issue Apr 21, 2017
@xzyfer @mcampa
Fix final Node 6 deprecation warnings (sass#1498)
011f2b0 
This was partially addressed by bumping NAN in sass#1496.

nodejs/node#6216
https://codereview.chromium.org/1839983002/patch/1/10002

Fixes sass#1484
mcampa pushed a commit to mcampa/node-sass that referenced this issue Apr 26, 2017
@xzyfer @mcampa
Fix final Node 6 deprecation warnings (sass#1498)
7674509 
This was partially addressed by bumping NAN in sass#1496.

nodejs/node#6216
https://codereview.chromium.org/1839983002/patch/1/10002

Fixes sass#1484
@uzysjung uzysjung mentioned this issue Jun 9, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@ofrobots @jeisinger @bnoordhuis @ChALkeR @Fishrock123