-
Notifications
You must be signed in to change notification settings - Fork 30k
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
src: replace ->To*(isolate) with ->To*(context).ToLocalChecked() #17343
Conversation
/cc @joyeecheung |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/11746/
src/node.cc
Outdated
@@ -589,7 +589,7 @@ Local<Value> ErrnoException(Isolate* isolate, | |||
} | |||
e = Exception::Error(cons); | |||
|
|||
Local<Object> obj = e->ToObject(env->isolate()); | |||
Local<Object> obj = e->ToObject(env->context()).ToLocalChecked(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think Exception::Error
always returns an object, so e.As<Object>()
should also work fine (and is a bit simpler/a bit more performant)
(This applies to a lot of other places in the PR too)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your review.
I see.
I didn't know that you could do it like that!
src/node.cc
Outdated
@@ -2364,7 +2366,8 @@ static void DLOpen(const FunctionCallbackInfo<Value>& args) { | |||
modlist_addon = mp; | |||
|
|||
Local<String> exports_string = env->exports_string(); | |||
Local<Object> exports = module->Get(exports_string)->ToObject(env->isolate()); | |||
Local<Object> exports = | |||
module->Get(exports_string)->ToObject(env->context()).ToLocalChecked(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're already here, you can feel free to also change the Get()
call to take a context argument :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get() call to take a context argument
I didn’t understand what you said.
Do you expect this change ?
Local<Object> exports = module->Get(exports_string).As<Object>();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you look at v8.h, you’ll see that there are two overloads of ->Get()
, one that takes a context
argument and one that doesn’t (and it deprecated):
Lines 3166 to 3168 in cbaf59c
V8_DEPRECATE_SOON("Use maybe version", Local<Value> Get(Local<Value> key)); | |
V8_WARN_UNUSED_RESULT MaybeLocal<Value> Get(Local<Context> context, | |
Local<Value> key); |
At some point, we’re going to use the latter here instead of module->Get(exports_string)
. Whether you want to do the conversion in this PR or not is up to you :)
If you want to see an example of how these conversions look like, you can take a look at #16247.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for detailed information.
If you look at v8.h,
I will check the header file properly after this.
I update to add Local<Context>
argument.
Please check my changes !
src/node_contextify.cc
Outdated
@@ -621,7 +621,7 @@ class ContextifyScript : public BaseObject { | |||
new ContextifyScript(env, args.This()); | |||
|
|||
TryCatch try_catch(env->isolate()); | |||
Local<String> code = args[0]->ToString(env->isolate()); | |||
Local<String> code = args[0]->ToString(env->context()).ToLocalChecked(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is actually incorrect, I think (didn't try this out yet, so I might be wrong):
new vm.Script({
toString() {
throw new Error();
}
});
would make ToString()
return an empty MaybeLocal
, right? That's fine because we have a try_catch
here to check for those kinds of conditions.
So I think the two options are:
- Use
.FromMaybe(Local<String>())
to get the previous behaviour - Move this line to before the
TryCatch
and return from the function immediately if the result is empty
Either way, it would be nice if you could add a test that the code snippet above doesn't crash the process :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would make ToString() return an empty MaybeLocal, right?
Yes it is.
I confirmed that it will crash when running that code.
./node test.js
FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal.
1: node::Abort() [/Users/leko/.ghq/github.com/Leko/node/./node]
2: node::FatalException(v8::Isolate*, v8::Local<v8::Value>, v8::Local<v8::Message>) [/Users/leko/.ghq/github.com/Leko/node/./node]
3: v8::V8::ToLocalEmpty() [/Users/leko/.ghq/github.com/Leko/node/./node]
4: node::(anonymous namespace)::ContextifyScript::New(v8::FunctionCallbackInfo<v8::Value> const&) [/Users/leko/.ghq/github.com/Leko/node/./node]
5: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo<v8::Value> const&)) [/Users/leko/.ghq/github.com/Leko/node/./node]
6: v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<true>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) [/Users/leko/.ghq/github.com/Leko/node/./node]
7: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/leko/.ghq/github.com/Leko/node/./node]
8: 0x32b64f842fd
Abort trap: 6
So I added test case and fix it.
Please check it again.
src/node_file.cc
Outdated
@@ -1219,7 +1219,7 @@ static void Read(const FunctionCallbackInfo<Value>& args) { | |||
|
|||
char * buf = nullptr; | |||
|
|||
Local<Object> buffer_obj = args[1]->ToObject(env->isolate()); | |||
Local<Object> buffer_obj = args[1]->ToObject(env->context()).ToLocalChecked(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be .As<Object>()
as well since we already checked that it's a buffer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean I should update as follow:
Local<Object> buffer_obj = args[1]->ToObject(env->context()).ToLocalChecked();
to
Local<Object> buffer_obj = args[1].As<Object>();
It is OK ?
I changed as described above, the test passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, exactly. If you know that something is an Object
instance, I would recommend using .As<Object>()
, because that boils down to a simple pointer cast (i.e. has zero runtime cost), unlike ToObject()
which always calls into the JS engine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for confirmation.
because that boils down to a simple pointer cast (i.e. has zero runtime cost), unlike ToObject() which always calls into the JS engine.
I didn't know that ! Thanks for letting me know.
> Exception::Error always returns an object, so e.As<Object>() should also work fine See #17343 (comment)
we already checked that its a buffer
…cript It failed with `FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal`.
It fixed this test case: 19a1b2e
Dont pass Local<Context> is deprecated soon. So we migrate to maybe version.
src/node.cc
Outdated
@@ -2364,7 +2366,8 @@ static void DLOpen(const FunctionCallbackInfo<Value>& args) { | |||
modlist_addon = mp; | |||
|
|||
Local<String> exports_string = env->exports_string(); | |||
Local<Object> exports = module->Get(exports_string)->ToObject(env->isolate()); | |||
Local<Object> exports = | |||
module->Get(env->context(), exports_string).ToLocalChecked().As<Object>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here we don't know if module.exports
is an object yet? The Local<Value>
returned by .ToLocalChecked()
should go through ->ToObject(context).ToLocalChecked()
again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joyeecheung Thank you for review.
You mean this changes (A)
Local<Object> exports =
module->Get(env->context(), exports_string).ToLocalChecked()
->ToObject(env->context()).ToLocalChecked();
or this(B) ?
Local<Object> exports =
module->Get(exports_string)->ToObject(env->context()).ToLocalChecked();
I think A is better because Local<Value> Get(Local<Value> key)
will be deprecated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Leko (A) seems better, but I think this is a situation similar to the vm
one: You can actually get this to crash with the current code, and .ToLocalChecked()
is not going to change that:
// The path here is just an example, any addon will work
process.dlopen({ exports: undefined }, './test/addons/hello-world/build/Release/binding.node');
because calling ToObject()
on undefined
gives an exception and therefore an empty exports
variable.
I think we want to do a similar thing to what we're doing in the vm module: If Get()
or ToObject()
return an empty MaybeLocal
, we should call dlib.Close()
to clean up the library and return from the function as soon as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @addaleax .
I fixed it by d7cc341 .
Is my understanding correct ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@l2dy yes, this seems about right! Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not at all !
Thanks for your quick reply.
Hi @addaleax, CI seems to be failing. |
New arm-fanned CI: https://ci.nodejs.org/job/node-test-commit-arm-fanned/13075/ |
Looks like everything’s green! Landed in e9e9863...2355c84 Thanks for the awesome PR – please feel invited to do more of this! ✨ 🎉 |
Squashed from multiple commits: - src: replace ->To*(isolate) with ->To*(context).ToLocalChecked() - test: use .As<Object> on Exception::Error > Exception::Error always returns an object, so e.As<Object>() should also work fine See #17343 (comment) - test: use .As<Object> instead of ->ToObject we already checked that its a buffer - src: use FromMaybe instead of ToLocalChecked It fixed this test case: 19a1b2e - src: pass context to Get() Dont pass Local<Context> is deprecated soon. So we migrate to maybe version. - src: return if Get or ToObject return an empty before call ToLocalChecked Refs: #17244 PR-URL: #17343 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
It failed with `FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal`. PR-URL: #17343 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
PR-URL: #17343 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Squashed from multiple commits: - src: replace ->To*(isolate) with ->To*(context).ToLocalChecked() - test: use .As<Object> on Exception::Error > Exception::Error always returns an object, so e.As<Object>() should also work fine See #17343 (comment) - test: use .As<Object> instead of ->ToObject we already checked that its a buffer - src: use FromMaybe instead of ToLocalChecked It fixed this test case: 19a1b2e - src: pass context to Get() Dont pass Local<Context> is deprecated soon. So we migrate to maybe version. - src: return if Get or ToObject return an empty before call ToLocalChecked Refs: #17244 PR-URL: #17343 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
It failed with `FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal`. PR-URL: #17343 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
PR-URL: #17343 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Squashed from multiple commits: - src: replace ->To*(isolate) with ->To*(context).ToLocalChecked() - test: use .As<Object> on Exception::Error > Exception::Error always returns an object, so e.As<Object>() should also work fine See #17343 (comment) - test: use .As<Object> instead of ->ToObject we already checked that its a buffer - src: use FromMaybe instead of ToLocalChecked It fixed this test case: 19a1b2e - src: pass context to Get() Dont pass Local<Context> is deprecated soon. So we migrate to maybe version. - src: return if Get or ToObject return an empty before call ToLocalChecked Refs: #17244 PR-URL: #17343 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
It failed with `FATAL ERROR: v8::ToLocalChecked Empty MaybeLocal`. PR-URL: #17343 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
PR-URL: #17343 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
This comment has been minimized.
This comment has been minimized.
Another backport ping |
This is part of Nodefest's Code and Learn nodejs/code-and-learn#72
Replace
->To*(isolate)
with->To*(context).ToLocalChecked()
insrc
files.See also #17244, and this safe list.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
src