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

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

Closed
wants to merge 8 commits into from
17 changes: 10 additions & 7 deletions src/node.cc
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
Member

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
Contributor Author

@Leko Leko Nov 29, 2017

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
Member

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
Contributor Author

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
Member

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
Contributor Author

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