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

v8: warn in Template::Set() on improper use #6277

Merged
merged 1 commit into from Apr 22, 2016

Conversation

Projects
None yet
5 participants
@bnoordhuis
Copy link
Member

commented Apr 19, 2016

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.)

Refs: #6216

Speculative - I'd like some input whether other collaborators think it's a good approach. It can get pretty noisy when native code abuses v8::Template::Set() a lot but OTOH, hopefully that means it gets fixed quickly.

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

@bnoordhuis bnoordhuis added this to the 6.0.0 milestone Apr 19, 2016

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 19, 2016

This LGTM but I'm curious how difficult it would be to have native code level warnings like this make use of the JS-level process.emitWarning() mechanism. .. nevermind! sigh, this is in the v8 code lol.

@@ -939,6 +939,15 @@ void Template::Set(v8::Local<Name> name, v8::Local<Data> value,
i::Isolate* isolate = templ->GetIsolate();
ENTER_V8(isolate);
i::HandleScope scope(isolate);
auto value_obj = Utils::OpenHandle(*value);

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Apr 19, 2016

Contributor

Do we need to leave a to-do here?

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Apr 19, 2016

Author Member

I can add a TODO but what should it say?

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Apr 19, 2016

Contributor

Perhaps TODO: Throw Error instead of warning in v7.0?

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Apr 19, 2016

Author Member

Oh, it's not going to throw; newer V8 versions replace it with a CHECK that aborts when it fails.

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Apr 19, 2016

Contributor

Oh I see. This is the change which you initially had in #6228, right? This? Then, we may not need a TODO I think. Eventually v8 upgrades will take care of this.

@thefourtheye

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2016

Don't we use dep: as the subsystem? Apart from that LGTM.

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2016

Don't we use dep: as the subsystem?

It's a bit of both. Historically, v8: was used for floating patches; deps: for upgrades. I can change it to deps: warn in v8::Template::Set() on improper use if that's preferred.

@thefourtheye

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2016

Historically, v8: was used for floating patches; deps: for upgrades

Oh, I didn't notice that before. Then we can leave it as it is.

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2016

I forgot, cc @nodejs/v8 @jeisinger.

@ofrobots

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2016

I wonder if it makes sense to add a flag (upstream, and floated here) to silence such runtime deprecation warnings. I do think it is good to be noisy by default on such imminent deprecations, but it would also be nice to be able to silence the warning if the user wishes.

One the problems native modules have had is that they never realize that things have been deprecated for a long time, and discover it when next Node.js major w/ new V8 drop them. If such a runtime deprecation warning flag existed, we could potentially use it for more things.

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Apr 19, 2016

I'd be okay with adding a flag but I assume that also calls for some kind of framework inside V8 for logging deprecation notices? Input on what that should look like appreciated; or if you're willing to take that on, I'm happy to step aside.

@ofrobots ofrobots referenced this pull request Apr 21, 2016

Closed

Planning for v6 #5766

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

@ofrobots ... well, we do already have a flags for handling deprecations and warnings in node of course. Perhaps it would be possible to provide an API for embedders that would allow handling of the runtime deprecations/warnings to be deferred to the embedder? We could then insert our own logic and control the output using the existing --no-deprecation|--trace-deprecation|--throw-deprecation|--no-warnings|--trace-warnings mechanisms.

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 21, 2016

Of more immediate concern, however, is whether this particular PR can land as is for v6 or does it need to be tweaked some more?

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2016

If it needs a complete overhaul, then it's not going to be ready for v6.0.0, which would be a shame, IMO.

I can add a flag to deps/v8/src/flag-definitions.h that silences the warning. Suggestions on the name?

@ofrobots

This comment has been minimized.

Copy link
Contributor

commented Apr 21, 2016

Given that this is urgent for v6, deciding on the framework before then might not be feasible, I agree.

How about something specific for this case for now; specially if we are not pushing it upstream first? Perhaps [no_]template_set_warning?

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Apr 21, 2016

Sounds good, that's more or less what I had in mind. I'll update the PR later today.

@bnoordhuis bnoordhuis force-pushed the bnoordhuis:template-set-warning branch Apr 22, 2016

@bnoordhuis

This comment has been minimized.

Copy link
Member Author

commented Apr 22, 2016

@jasnell

This comment has been minimized.

Copy link
Member

commented Apr 22, 2016

LGTM if CI is green

v8: warn in Template::Set() on improper use
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 force-pushed the bnoordhuis:template-set-warning branch to 7940ecf Apr 22, 2016

@bnoordhuis bnoordhuis closed this Apr 22, 2016

@bnoordhuis bnoordhuis deleted the bnoordhuis:template-set-warning branch Apr 22, 2016

@bnoordhuis bnoordhuis merged commit 7940ecf into nodejs:master Apr 22, 2016

joelostrowski added a commit to joelostrowski/node that referenced this pull request Apr 25, 2016

v8: warn in Template::Set() on improper use
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 added a commit that referenced this pull request Apr 26, 2016

v8: warn in Template::Set() on improper use
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>

@olalonde olalonde referenced this pull request Apr 28, 2016

Closed

Error with Node v6.0.0? #824

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jun 1, 2016

@bnoordhuis looks like this is specific to v8 > 5
Set dont-land... please modify if I am incorrect

targos added a commit to targos/node that referenced this pull request Jun 3, 2016

v8: warn in Template::Set() on improper use
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 added a commit to targos/node that referenced this pull request Jun 29, 2016

v8: warn in Template::Set() on improper use
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 added a commit to targos/node that referenced this pull request Jul 26, 2016

v8: warn in Template::Set() on improper use
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>

ofrobots added a commit to ofrobots/node that referenced this pull request Aug 25, 2016

v8: warn in Template::Set() on improper use
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.