From 924cc6c6335e58f61b04d2f41d348bd6b8be98a1 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 2 Feb 2016 23:50:07 +0100 Subject: src: upgrade to new v8::Private api Stop using the deprecated `GetHiddenValue()` and `SetHiddenValue()` methods, start using `GetPrivate()` and `SetPrivate()` instead. This commit turns some of the entries in the per-isolate string table into private symbols. PR-URL: https://github.com/nodejs/node/pull/5045 Reviewed-By: Colin Ihrig Reviewed-By: Trevor Norris --- src/env-inl.h | 48 +++++++++++++++++++++++++++---------- src/env.h | 55 +++++++++++++++++++++++++++++------------- src/node.cc | 35 +++++++++++++++++---------- src/node_contextify.cc | 60 +++++++++++++++++++++++++++------------------- src/node_crypto.cc | 65 ++++++++++++++++++++++++++++++-------------------- src/node_util.cc | 9 +++++-- 6 files changed, 178 insertions(+), 94 deletions(-) (limited to 'src') diff --git a/src/env-inl.h b/src/env-inl.h index d2c0e048a6..46272585d5 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -50,12 +50,25 @@ inline Environment::IsolateData::IsolateData(v8::Isolate* isolate, : event_loop_(loop), isolate_(isolate), #define V(PropertyName, StringValue) \ - PropertyName ## _(isolate, \ - v8::String::NewFromOneByte( \ - isolate, \ - reinterpret_cast(StringValue), \ - v8::NewStringType::kInternalized, \ - sizeof(StringValue) - 1).ToLocalChecked()), + PropertyName ## _( \ + isolate, \ + v8::Private::ForApi( \ + isolate, \ + v8::String::NewFromOneByte( \ + isolate, \ + reinterpret_cast(StringValue), \ + v8::NewStringType::kInternalized, \ + sizeof(StringValue) - 1).ToLocalChecked())), + PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) +#undef V +#define V(PropertyName, StringValue) \ + PropertyName ## _( \ + isolate, \ + v8::String::NewFromOneByte( \ + isolate, \ + reinterpret_cast(StringValue), \ + v8::NewStringType::kInternalized, \ + sizeof(StringValue) - 1).ToLocalChecked()), PER_ISOLATE_STRING_PROPERTIES(V) #undef V ref_count_(0) {} @@ -525,21 +538,31 @@ inline v8::Local Environment::NewInternalFieldObject() { return m_obj.ToLocalChecked(); } -#define V(PropertyName, StringValue) \ +#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue) +#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) +#define V(TypeName, PropertyName, StringValue) \ inline \ - v8::Local Environment::IsolateData::PropertyName() const { \ + v8::Local Environment::IsolateData::PropertyName() const { \ /* Strings are immutable so casting away const-ness here is okay. */ \ return const_cast(this)->PropertyName ## _.Get(isolate()); \ } - PER_ISOLATE_STRING_PROPERTIES(V) + PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP) + PER_ISOLATE_STRING_PROPERTIES(VS) #undef V +#undef VS +#undef VP -#define V(PropertyName, StringValue) \ - inline v8::Local Environment::PropertyName() const { \ +#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue) +#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) +#define V(TypeName, PropertyName, StringValue) \ + inline v8::Local Environment::PropertyName() const { \ return isolate_data()->PropertyName(); \ } - PER_ISOLATE_STRING_PROPERTIES(V) + PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP) + PER_ISOLATE_STRING_PROPERTIES(VS) #undef V +#undef VS +#undef VP #define V(PropertyName, TypeName) \ inline v8::Local Environment::PropertyName() const { \ @@ -552,6 +575,7 @@ inline v8::Local Environment::NewInternalFieldObject() { #undef V #undef ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES +#undef PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES #undef PER_ISOLATE_STRING_PROPERTIES } // namespace node diff --git a/src/env.h b/src/env.h index 254f0c8fa1..e160e62310 100644 --- a/src/env.h +++ b/src/env.h @@ -44,14 +44,24 @@ namespace node { #define NODE_PUSH_VAL_TO_ARRAY_MAX 8 #endif +// Private symbols are per-isolate primitives but Environment proxies them +// for the sake of convenience. Strings should be ASCII-only and have a +// "node:" prefix to avoid name clashes with third-party code. +#define PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) \ + V(alpn_buffer_private_symbol, "node:alpnBuffer") \ + V(arrow_message_private_symbol, "node:arrowMessage") \ + V(contextify_private_symbol, "node:contextify") \ + V(decorated_private_symbol, "node:decorated") \ + V(npn_buffer_private_symbol, "node:npnBuffer") \ + V(processed_private_symbol, "node:processed") \ + V(selected_npn_buffer_private_symbol, "node:selectedNpnBuffer") \ + // Strings are per-isolate primitives but Environment proxies them // for the sake of convenience. Strings should be ASCII-only. #define PER_ISOLATE_STRING_PROPERTIES(V) \ V(address_string, "address") \ - V(alpn_buffer_string, "alpnBuffer") \ V(args_string, "args") \ V(argv_string, "argv") \ - V(arrow_message_string, "node:arrowMessage") \ V(async, "async") \ V(async_queue_string, "_asyncQueue") \ V(atime_string, "atime") \ @@ -73,7 +83,6 @@ namespace node { V(cwd_string, "cwd") \ V(debug_port_string, "debugPort") \ V(debug_string, "debug") \ - V(decorated_string, "node:decorated") \ V(dest_string, "dest") \ V(detached_string, "detached") \ V(dev_string, "dev") \ @@ -140,7 +149,6 @@ namespace node { V(netmask_string, "netmask") \ V(nice_string, "nice") \ V(nlink_string, "nlink") \ - V(npn_buffer_string, "npnBuffer") \ V(nsname_string, "nsname") \ V(ocsp_request_string, "OCSPRequest") \ V(offset_string, "offset") \ @@ -176,7 +184,6 @@ namespace node { V(port_string, "port") \ V(preference_string, "preference") \ V(priority_string, "priority") \ - V(processed_string, "processed") \ V(produce_cached_data_string, "produceCachedData") \ V(prototype_string, "prototype") \ V(raw_string, "raw") \ @@ -192,7 +199,6 @@ namespace node { V(serial_string, "serial") \ V(scavenge_string, "scavenge") \ V(scopeid_string, "scopeid") \ - V(selected_npn_buffer_string, "selectedNpnBuffer") \ V(sent_shutdown_string, "sentShutdown") \ V(serial_number_string, "serialNumber") \ V(service_string, "service") \ @@ -507,12 +513,17 @@ class Environment { inline v8::Local NewInternalFieldObject(); - // Strings are shared across shared contexts. The getters simply proxy to - // the per-isolate primitive. -#define V(PropertyName, StringValue) \ - inline v8::Local PropertyName() const; - PER_ISOLATE_STRING_PROPERTIES(V) + // Strings and private symbols are shared across shared contexts + // The getters simply proxy to the per-isolate primitive. +#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue) +#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) +#define V(TypeName, PropertyName, StringValue) \ + inline v8::Local PropertyName() const; + PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP) + PER_ISOLATE_STRING_PROPERTIES(VS) #undef V +#undef VS +#undef VP #define V(PropertyName, TypeName) \ inline v8::Local PropertyName() const; \ @@ -585,10 +596,15 @@ class Environment { inline void Put(); inline uv_loop_t* event_loop() const; -#define V(PropertyName, StringValue) \ - inline v8::Local PropertyName() const; - PER_ISOLATE_STRING_PROPERTIES(V) +#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue) +#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) +#define V(TypeName, PropertyName, StringValue) \ + inline v8::Local PropertyName() const; + PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP) + PER_ISOLATE_STRING_PROPERTIES(VS) #undef V +#undef VS +#undef VP private: inline static IsolateData* Get(v8::Isolate* isolate); @@ -598,10 +614,15 @@ class Environment { uv_loop_t* const event_loop_; v8::Isolate* const isolate_; -#define V(PropertyName, StringValue) \ - v8::Eternal PropertyName ## _; - PER_ISOLATE_STRING_PROPERTIES(V) +#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue) +#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue) +#define V(TypeName, PropertyName, StringValue) \ + v8::Eternal PropertyName ## _; + PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP) + PER_ISOLATE_STRING_PROPERTIES(VS) #undef V +#undef VS +#undef VP unsigned int ref_count_; diff --git a/src/node.cc b/src/node.cc index 63cde12683..ab586272d3 100644 --- a/src/node.cc +++ b/src/node.cc @@ -1402,8 +1402,10 @@ ssize_t DecodeWrite(Isolate* isolate, bool IsExceptionDecorated(Environment* env, Local er) { if (!er.IsEmpty() && er->IsObject()) { Local err_obj = er.As(); - Local decorated = err_obj->GetHiddenValue(env->decorated_string()); - return !decorated.IsEmpty() && decorated->IsTrue(); + auto maybe_value = + err_obj->GetPrivate(env->context(), env->decorated_private_symbol()); + Local decorated; + return maybe_value.ToLocal(&decorated) && decorated->IsTrue(); } return false; } @@ -1419,10 +1421,15 @@ void AppendExceptionLine(Environment* env, if (!er.IsEmpty() && er->IsObject()) { err_obj = er.As(); + auto context = env->context(); + auto processed_private_symbol = env->processed_private_symbol(); // Do it only once per message - if (!err_obj->GetHiddenValue(env->processed_string()).IsEmpty()) + if (err_obj->HasPrivate(context, processed_private_symbol).FromJust()) return; - err_obj->SetHiddenValue(env->processed_string(), True(env->isolate())); + err_obj->SetPrivate( + context, + processed_private_symbol, + True(env->isolate())); } // Print (filename):(line number): (message). @@ -1492,14 +1499,15 @@ void AppendExceptionLine(Environment* env, Local arrow_str = String::NewFromUtf8(env->isolate(), arrow); - // Allocation failed, just print it out - if (arrow_str.IsEmpty() || err_obj.IsEmpty() || !err_obj->IsNativeError()) - goto print; - - err_obj->SetHiddenValue(env->arrow_message_string(), arrow_str); - return; + if (!arrow_str.IsEmpty() && !err_obj.IsEmpty() && err_obj->IsNativeError()) { + err_obj->SetPrivate( + env->context(), + env->arrow_message_private_symbol(), + arrow_str); + return; + } - print: + // Allocation failed, just print it out. if (env->printed_error()) return; env->set_printed_error(true); @@ -1525,7 +1533,10 @@ static void ReportException(Environment* env, Local err_obj = er->ToObject(env->isolate()); trace_value = err_obj->Get(env->stack_string()); - arrow = err_obj->GetHiddenValue(env->arrow_message_string()); + arrow = + err_obj->GetPrivate( + env->context(), + env->arrow_message_private_symbol()).ToLocalChecked(); } node::Utf8Value trace(env->isolate(), trace_value); diff --git a/src/node_contextify.cc b/src/node_contextify.cc index c2ff517abc..4eca5cc53a 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -288,11 +288,11 @@ class ContextifyContext { } Local sandbox = args[0].As(); - Local hidden_name = - FIXED_ONE_BYTE_STRING(env->isolate(), "_contextifyHidden"); - // Don't allow contextifying a sandbox multiple times. - CHECK(sandbox->GetHiddenValue(hidden_name).IsEmpty()); + CHECK( + !sandbox->HasPrivate( + env->context(), + env->contextify_private_symbol()).FromJust()); TryCatch try_catch; ContextifyContext* context = new ContextifyContext(env, sandbox); @@ -305,8 +305,10 @@ class ContextifyContext { if (context->context().IsEmpty()) return; - Local hidden_context = External::New(env->isolate(), context); - sandbox->SetHiddenValue(hidden_name, hidden_context); + sandbox->SetPrivate( + env->context(), + env->contextify_private_symbol(), + External::New(env->isolate(), context)); } @@ -319,10 +321,9 @@ class ContextifyContext { } Local sandbox = args[0].As(); - Local hidden_name = - FIXED_ONE_BYTE_STRING(env->isolate(), "_contextifyHidden"); - - args.GetReturnValue().Set(!sandbox->GetHiddenValue(hidden_name).IsEmpty()); + auto result = + sandbox->HasPrivate(env->context(), env->contextify_private_symbol()); + args.GetReturnValue().Set(result.FromJust()); } @@ -342,17 +343,17 @@ class ContextifyContext { static ContextifyContext* ContextFromContextifiedSandbox( - Isolate* isolate, + Environment* env, const Local& sandbox) { - Local hidden_name = - FIXED_ONE_BYTE_STRING(isolate, "_contextifyHidden"); - Local context_external_v = sandbox->GetHiddenValue(hidden_name); - if (context_external_v.IsEmpty() || !context_external_v->IsExternal()) { - return nullptr; + auto maybe_value = + sandbox->GetPrivate(env->context(), env->contextify_private_symbol()); + Local context_external_v; + if (maybe_value.ToLocal(&context_external_v) && + context_external_v->IsExternal()) { + Local context_external = context_external_v.As(); + return static_cast(context_external->Value()); } - Local context_external = context_external_v.As(); - - return static_cast(context_external->Value()); + return nullptr; } @@ -612,8 +613,7 @@ class ContextifyScript : public BaseObject { // Get the context from the sandbox ContextifyContext* contextify_context = - ContextifyContext::ContextFromContextifiedSandbox(env->isolate(), - sandbox); + ContextifyContext::ContextFromContextifiedSandbox(env, sandbox); if (contextify_context == nullptr) { return env->ThrowTypeError( "sandbox argument must have been converted to a context."); @@ -654,15 +654,25 @@ class ContextifyScript : public BaseObject { AppendExceptionLine(env, exception, try_catch.Message()); Local stack = err_obj->Get(env->stack_string()); - Local arrow = err_obj->GetHiddenValue(env->arrow_message_string()); - - if (!(stack->IsString() && arrow->IsString())) + auto maybe_value = + err_obj->GetPrivate( + env->context(), + env->arrow_message_private_symbol()); + + Local arrow; + if (!(maybe_value.ToLocal(&arrow) && + arrow->IsString() && + stack->IsString())) { return; + } Local decorated_stack = String::Concat(arrow.As(), stack.As()); err_obj->Set(env->stack_string(), decorated_stack); - err_obj->SetHiddenValue(env->decorated_string(), True(env->isolate())); + err_obj->SetPrivate( + env->context(), + env->decorated_private_symbol(), + True(env->isolate())); } static int64_t GetTimeoutArg(const FunctionCallbackInfo& args, diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 2fb6e887d1..c1afb6a4ed 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -1965,10 +1965,12 @@ int SSLWrap::AdvertiseNextProtoCallback(SSL* s, HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); - Local npn_buffer = - w->object()->GetHiddenValue(env->npn_buffer_string()); + auto npn_buffer = + w->object()->GetPrivate( + env->context(), + env->npn_buffer_private_symbol()).ToLocalChecked(); - if (npn_buffer.IsEmpty()) { + if (npn_buffer->IsUndefined()) { // No initialization - no NPN protocols *data = reinterpret_cast(""); *len = 0; @@ -1994,19 +1996,23 @@ int SSLWrap::SelectNextProtoCallback(SSL* s, HandleScope handle_scope(env->isolate()); Context::Scope context_scope(env->context()); - Local npn_buffer = - w->object()->GetHiddenValue(env->npn_buffer_string()); + auto npn_buffer = + w->object()->GetPrivate( + env->context(), + env->npn_buffer_private_symbol()).ToLocalChecked(); - if (npn_buffer.IsEmpty()) { + if (npn_buffer->IsUndefined()) { // We should at least select one protocol // If server is using NPN *out = reinterpret_cast(const_cast("http/1.1")); *outlen = 8; // set status: unsupported - bool r = w->object()->SetHiddenValue(env->selected_npn_buffer_string(), - False(env->isolate())); - CHECK(r); + CHECK( + w->object()->SetPrivate( + env->context(), + env->selected_npn_buffer_private_symbol(), + False(env->isolate())).FromJust()); return SSL_TLSEXT_ERR_OK; } @@ -2032,9 +2038,11 @@ int SSLWrap::SelectNextProtoCallback(SSL* s, break; } - bool r = w->object()->SetHiddenValue(env->selected_npn_buffer_string(), - result); - CHECK(r); + CHECK( + w->object()->SetPrivate( + env->context(), + env->selected_npn_buffer_private_symbol(), + result).FromJust()); return SSL_TLSEXT_ERR_OK; } @@ -2044,14 +2052,14 @@ template void SSLWrap::GetNegotiatedProto( const FunctionCallbackInfo& args) { Base* w = Unwrap(args.Holder()); + Environment* env = w->env(); if (w->is_client()) { - Local selected_npn_buffer = - w->object()->GetHiddenValue(w->env()->selected_npn_buffer_string()); - - if (selected_npn_buffer.IsEmpty() == false) - args.GetReturnValue().Set(selected_npn_buffer); - + auto selected_npn_buffer = + w->object()->GetPrivate( + env->context(), + env->selected_npn_buffer_private_symbol()).ToLocalChecked(); + args.GetReturnValue().Set(selected_npn_buffer); return; } @@ -2076,9 +2084,11 @@ void SSLWrap::SetNPNProtocols(const FunctionCallbackInfo& args) { if (args.Length() < 1 || !Buffer::HasInstance(args[0])) return env->ThrowTypeError("Must give a Buffer as first argument"); - Local npn_buffer = Local::New(env->isolate(), args[0]); - bool r = w->object()->SetHiddenValue(env->npn_buffer_string(), npn_buffer); - CHECK(r); + CHECK( + w->object()->SetPrivate( + env->context(), + env->npn_buffer_private_symbol(), + args[0]).FromJust()); } #endif // OPENSSL_NPN_NEGOTIATED @@ -2101,7 +2111,9 @@ int SSLWrap::SelectALPNCallback(SSL* s, Context::Scope context_scope(env->context()); Local alpn_buffer = - w->object()->GetHiddenValue(env->alpn_buffer_string()); + w->object()->GetPrivate( + env->context(), + env->alpn_buffer_private_symbol()).ToLocalChecked(); CHECK(Buffer::HasInstance(alpn_buffer)); const unsigned char* alpn_protos = reinterpret_cast(Buffer::Data(alpn_buffer)); @@ -2164,10 +2176,11 @@ void SSLWrap::SetALPNProtocols( int r = SSL_set_alpn_protos(w->ssl_, alpn_protos, alpn_protos_len); CHECK_EQ(r, 0); } else { - Local alpn_buffer = Local::New(env->isolate(), args[0]); - bool ret = w->object()->SetHiddenValue(env->alpn_buffer_string(), - alpn_buffer); - CHECK(ret); + CHECK( + w->object()->SetPrivate( + env->context(), + env->alpn_buffer_private_symbol(), + args[0]).FromJust()); // Server should select ALPN protocol from list of advertised by client SSL_CTX_set_alpn_select_cb(w->ssl_->ctx, SelectALPNCallback, nullptr); } diff --git a/src/node_util.cc b/src/node_util.cc index 8475468c1f..1c5a2fa9a1 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -10,6 +10,7 @@ using v8::Context; using v8::FunctionCallbackInfo; using v8::Local; using v8::Object; +using v8::Private; using v8::String; using v8::Value; @@ -48,8 +49,10 @@ static void GetHiddenValue(const FunctionCallbackInfo& args) { Local obj = args[0].As(); Local name = args[1].As(); + auto private_symbol = Private::ForApi(env->isolate(), name); + auto maybe_value = obj->GetPrivate(env->context(), private_symbol); - args.GetReturnValue().Set(obj->GetHiddenValue(name)); + args.GetReturnValue().Set(maybe_value.ToLocalChecked()); } static void SetHiddenValue(const FunctionCallbackInfo& args) { @@ -63,8 +66,10 @@ static void SetHiddenValue(const FunctionCallbackInfo& args) { Local obj = args[0].As(); Local name = args[1].As(); + auto private_symbol = Private::ForApi(env->isolate(), name); + auto maybe_value = obj->SetPrivate(env->context(), private_symbol, args[2]); - args.GetReturnValue().Set(obj->SetHiddenValue(name, args[2])); + args.GetReturnValue().Set(maybe_value.FromJust()); } -- cgit v1.2.3