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

src: replace ->To*(isolate) with ->To*(context).ToLocalChecked() #17343

Closed
wants to merge 8 commits into from
+56 −17
Closed

src: replace ->To*(isolate) with ->To*(context).ToLocalChecked() #17343

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e41c511
src: replace ->To*(isolate) with ->To*(context).ToLocalChecked()
Leko Nov 27, 2017
f9107cc
test: use .As<Object> on Exception::Error
Leko Nov 28, 2017
60f4711
test: use .As<Object> instead of ->ToObject
Leko Nov 28, 2017
19a1b2e
test: add test case that crashes when throwing an exception with vm.S…
Leko Nov 28, 2017
1926bcf
src: use FromMaybe instead of ToLocalChecked
Leko Nov 28, 2017
096cbbf
src: pass context to Get()
Leko Nov 28, 2017
dbee7b0
test: add test case that crashes when process.dlopen with undefined
Leko Nov 30, 2017
d7cc341
src: return if Get or ToObject return an empty before call ToLocalChe…
Leko Nov 30, 2017
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
Next Next commit
src: replace ->To*(isolate) with ->To*(context).ToLocalChecked() 
See #17244
@Leko
Leko committed Nov 27, 2017

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
commit e41c511dcaa5a9ca3f2a3356986579a43e342c00
17 changes: 10 additions & 7 deletions src/node.cc
View file
Original file line number Diff line number Diff line change
@@ -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();
Copy link
Member

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)

Copy link
Contributor Author

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!

obj->Set(env->errno_string(), Integer::New(env->isolate(), errorno));
obj->Set(env->code_string(), estring);

@@ -751,7 +751,7 @@ Local<Value> WinapiErrnoException(Isolate* isolate,
e = Exception::Error(message);
}

Local<Object> obj = e->ToObject(env->isolate());
Local<Object> obj = e->ToObject(env->context()).ToLocalChecked();
obj->Set(env->errno_string(), Integer::New(isolate, errorno));

if (path != nullptr) {
@@ -1482,7 +1482,7 @@ static void ReportException(Environment* env,
if (er->IsUndefined() || er->IsNull()) {
trace_value = Undefined(env->isolate());
} else {
Local<Object> err_obj = er->ToObject(env->isolate());
Local<Object> err_obj = er->ToObject(env->context()).ToLocalChecked();

trace_value = err_obj->Get(env->stack_string());
arrow =
@@ -2301,7 +2301,8 @@ static void DLOpen(const FunctionCallbackInfo<Value>& args) {
return env->ThrowTypeError("flag argument must be an integer.");
}

Local<Object> module = args[0]->ToObject(env->isolate()); // Cast
Local<Object> module =
args[0]->ToObject(env->context()).ToLocalChecked(); // Cast
node::Utf8Value filename(env->isolate(), args[1]); // Cast
DLib dlib;
dlib.filename_ = *filename;
@@ -2319,7 +2320,8 @@ static void DLOpen(const FunctionCallbackInfo<Value>& args) {
dlib.Close();
#ifdef _WIN32
// Windows needs to add the filename into the error message
errmsg = String::Concat(errmsg, args[1]->ToString(env->isolate()));
errmsg = String::Concat(errmsg,
args[1]->ToString(env->context()).ToLocalChecked());
#endif // _WIN32
env->isolate()->ThrowException(Exception::Error(errmsg));
return;
@@ -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();
Copy link
Member

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

Copy link
Contributor Author

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>();

Copy link
Member

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

node/deps/v8/include/v8.h

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.

Copy link
Contributor Author

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 !


if (mp->nm_context_register_func != nullptr) {
mp->nm_context_register_func(exports, module, env->context(), mp->nm_priv);
@@ -4337,7 +4340,7 @@ void EmitBeforeExit(Environment* env) {
Local<String> exit_code = FIXED_ONE_BYTE_STRING(env->isolate(), "exitCode");
Local<Value> args[] = {
FIXED_ONE_BYTE_STRING(env->isolate(), "beforeExit"),
process_object->Get(exit_code)->ToInteger(env->isolate())
process_object->Get(exit_code)->ToInteger(env->context()).ToLocalChecked()
};
MakeCallback(env->isolate(),
process_object, "emit", arraysize(args), args,
4 changes: 2 additions & 2 deletions src/node_buffer.cc
View file
Original file line number Diff line number Diff line change
@@ -606,7 +606,7 @@ void Fill(const FunctionCallbackInfo<Value>& args) {
return;
}

str_obj = args[1]->ToString(env->isolate());
str_obj = args[1]->ToString(env->context()).ToLocalChecked();
enc = ParseEncoding(env->isolate(), args[4], UTF8);
str_length =
enc == UTF8 ? str_obj->Utf8Length() :
@@ -677,7 +677,7 @@ void StringWrite(const FunctionCallbackInfo<Value>& args) {
if (!args[0]->IsString())
return env->ThrowTypeError("Argument must be a string");

Local<String> str = args[0]->ToString(env->isolate());
Local<String> str = args[0]->ToString(env->context()).ToLocalChecked();

size_t offset;
size_t max_length;
2 changes: 1 addition & 1 deletion src/node_contextify.cc
View file
Original file line number Diff line number Diff line change
@@ -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();
Copy link
Member

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

Copy link
Contributor Author

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.


Local<Value> options = args[1];
MaybeLocal<String> filename = GetFilenameArg(env, options);
2 changes: 1 addition & 1 deletion src/node_file.cc
View file
Original file line number Diff line number Diff line change
@@ -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();
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

char *buffer_data = Buffer::Data(buffer_obj);
size_t buffer_length = Buffer::Length(buffer_obj);

2 changes: 1 addition & 1 deletion src/node_http_parser.cc
View file
Original file line number Diff line number Diff line change
@@ -466,7 +466,7 @@ class Parser : public AsyncWrap {
enum http_errno err = HTTP_PARSER_ERRNO(&parser->parser_);

Local<Value> e = Exception::Error(env->parse_error_string());
Local<Object> obj = e->ToObject(env->isolate());
Local<Object> obj = e->ToObject(env->context()).ToLocalChecked();
obj->Set(env->bytes_parsed_string(), Integer::New(env->isolate(), 0));
obj->Set(env->code_string(),
OneByteString(env->isolate(), http_errno_name(err)));
4 changes: 2 additions & 2 deletions src/node_zlib.cc
View file
Original file line number Diff line number Diff line change
@@ -178,7 +178,7 @@ class ZCtx : public AsyncWrap {
} else {
CHECK(Buffer::HasInstance(args[1]));
Local<Object> in_buf;
in_buf = args[1]->ToObject(env->isolate());
in_buf = args[1]->ToObject(env->context()).ToLocalChecked();
in_off = args[2]->Uint32Value();
in_len = args[3]->Uint32Value();

@@ -187,7 +187,7 @@ class ZCtx : public AsyncWrap {
}

CHECK(Buffer::HasInstance(args[4]));
Local<Object> out_buf = args[4]->ToObject(env->isolate());
Local<Object> out_buf = args[4]->ToObject(env->context()).ToLocalChecked();
out_off = args[5]->Uint32Value();
out_len = args[6]->Uint32Value();
CHECK(Buffer::IsWithinBounds(out_off, out_len, Buffer::Length(out_buf)));
3 changes: 2 additions & 1 deletion src/process_wrap.cc
View file
Original file line number Diff line number Diff line change
@@ -143,7 +143,8 @@ class ProcessWrap : public HandleWrap {
ProcessWrap* wrap;
ASSIGN_OR_RETURN_UNWRAP(&wrap, args.Holder());

Local<Object> js_options = args[0]->ToObject(env->isolate());
Local<Object> js_options =
args[0]->ToObject(env->context()).ToLocalChecked();

uv_process_options_t options;
memset(&options, 0, sizeof(uv_process_options_t));
4 changes: 2 additions & 2 deletions src/stream_base.cc
View file
Original file line number Diff line number Diff line change
@@ -127,7 +127,7 @@ int StreamBase::Writev(const FunctionCallbackInfo<Value>& args) {
// Buffer chunk, no additional storage required

// String chunk
Local<String> string = chunk->ToString(env->isolate());
Local<String> string = chunk->ToString(env->context()).ToLocalChecked();
enum encoding encoding = ParseEncoding(env->isolate(),
chunks->Get(i * 2 + 1));
size_t chunk_size;
@@ -179,7 +179,7 @@ int StreamBase::Writev(const FunctionCallbackInfo<Value>& args) {
char* str_storage = req_wrap->Extra(offset);
size_t str_size = storage_size - offset;

Local<String> string = chunk->ToString(env->isolate());
Local<String> string = chunk->ToString(env->context()).ToLocalChecked();
enum encoding encoding = ParseEncoding(env->isolate(),
chunks->Get(i * 2 + 1));
str_size = StringBytes::Write(env->isolate(),