Skip to content

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

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

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

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

Show file tree
Hide file tree
Changes from 6 commits
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
17 changes: 10 additions & 7 deletions src/node.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@ Local<Value> ErrnoException(Isolate* isolate,
}
e = Exception::Error(cons);

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

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

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

if (path != nullptr) {
Expand Down Expand Up @@ -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 =
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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>();
Copy link
Copy Markdown
Member

@joyeecheung joyeecheung Nov 29, 2017

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.

Copy link
Copy Markdown
Contributor Author

@Leko Leko Nov 29, 2017 •
edited
Loading

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.

Copy link
Copy Markdown
Member

@addaleax addaleax Nov 30, 2017

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.

Copy link
Copy Markdown
Contributor Author

@Leko Leko Nov 30, 2017

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 ?

Copy link
Copy Markdown
Member

@addaleax addaleax Nov 30, 2017

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!

Copy link
Copy Markdown
Contributor Author

@Leko Leko Nov 30, 2017

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.


if (mp->nm_context_register_func != nullptr) {
mp->nm_context_register_func(exports, module, env->context(), mp->nm_priv);
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions src/node_buffer.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() :
Expand Down Expand Up @@ -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;
Expand Down
3 changes: 2 additions & 1 deletion src/node_contextify.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,7 +621,8 @@ 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()).FromMaybe(Local<String>());

Local<Value> options = args[1];
MaybeLocal<String> filename = GetFilenameArg(env, options);
Expand Down
2 changes: 1 addition & 1 deletion src/node_file.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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].As<Object>();
char *buffer_data = Buffer::Data(buffer_obj);
size_t buffer_length = Buffer::Length(buffer_obj);

Expand Down
2 changes: 1 addition & 1 deletion src/node_http_parser.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.As<Object>();
obj->Set(env->bytes_parsed_string(), Integer::New(env->isolate(), 0));
obj->Set(env->code_string(),
OneByteString(env->isolate(), http_errno_name(err)));
Expand Down
4 changes: 2 additions & 2 deletions src/node_zlib.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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)));
Expand Down
3 changes: 2 additions & 1 deletion src/process_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
4 changes: 2 additions & 2 deletions src/stream_base.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(),
Expand Down
14 changes: 14 additions & 0 deletions test/parallel/test-vm-script-throw-in-tostring.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
'use strict';

require('../common');
const assert = require('assert');

const vm = require('vm');

assert.throws(() => {
new vm.Script({
toString() {
throw new Error();
}
});
}, Error);