Skip to content

Commit

Permalink
src: upgrade to new v8::Private api
Browse files Browse the repository at this point in the history 
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: #5045
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
  • Loading branch information
@bnoordhuis
bnoordhuis committed Feb 3, 2016
1 parent 7ea34fd commit 924cc6c
Show file tree
Hide file tree
Showing 6 changed files with 178 additions and 94 deletions.
48 changes: 36 additions & 12 deletions src/env-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<const uint8_t*>(StringValue), \
v8::NewStringType::kInternalized, \
sizeof(StringValue) - 1).ToLocalChecked()),
PropertyName ## _( \
isolate, \
v8::Private::ForApi( \
isolate, \
v8::String::NewFromOneByte( \
isolate, \
reinterpret_cast<const uint8_t*>(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<const uint8_t*>(StringValue), \
v8::NewStringType::kInternalized, \
sizeof(StringValue) - 1).ToLocalChecked()),
PER_ISOLATE_STRING_PROPERTIES(V)
#undef V
ref_count_(0) {}
Expand Down Expand Up @@ -525,21 +538,31 @@ inline v8::Local<v8::Object> 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<v8::String> Environment::IsolateData::PropertyName() const { \
v8::Local<TypeName> Environment::IsolateData::PropertyName() const { \
/* Strings are immutable so casting away const-ness here is okay. */ \
return const_cast<IsolateData*>(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<v8::String> 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<TypeName> 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<TypeName> Environment::PropertyName() const { \
Expand All @@ -552,6 +575,7 @@ inline v8::Local<v8::Object> Environment::NewInternalFieldObject() {
#undef V

#undef ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES
#undef PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES
#undef PER_ISOLATE_STRING_PROPERTIES

} // namespace node
Expand Down
55 changes: 38 additions & 17 deletions src/env.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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") \
Expand All @@ -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") \
Expand Down Expand Up @@ -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") \
Expand Down Expand Up @@ -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") \
Expand All @@ -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") \
Expand Down Expand Up @@ -507,12 +513,17 @@ class Environment {

inline v8::Local<v8::Object> NewInternalFieldObject();

// Strings are shared across shared contexts. The getters simply proxy to
// the per-isolate primitive.
#define V(PropertyName, StringValue) \
inline v8::Local<v8::String> 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<TypeName> 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<TypeName> PropertyName() const; \
Expand Down Expand Up @@ -585,10 +596,15 @@ class Environment {
inline void Put();
inline uv_loop_t* event_loop() const;

#define V(PropertyName, StringValue) \
inline v8::Local<v8::String> 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<TypeName> 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);
Expand All @@ -598,10 +614,15 @@ class Environment {
uv_loop_t* const event_loop_;
v8::Isolate* const isolate_;

#define V(PropertyName, StringValue) \
v8::Eternal<v8::String> 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<TypeName> PropertyName ## _;
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP)
PER_ISOLATE_STRING_PROPERTIES(VS)
#undef V
#undef VS
#undef VP

unsigned int ref_count_;

Expand Down
35 changes: 23 additions & 12 deletions src/node.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1402,8 +1402,10 @@ ssize_t DecodeWrite(Isolate* isolate,
bool IsExceptionDecorated(Environment* env, Local<Value> er) {
if (!er.IsEmpty() && er->IsObject()) {
Local<Object> err_obj = er.As<Object>();
Local<Value> 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<Value> decorated;
return maybe_value.ToLocal(&decorated) && decorated->IsTrue();
}
return false;
}
Expand All @@ -1419,10 +1421,15 @@ void AppendExceptionLine(Environment* env,
if (!er.IsEmpty() && er->IsObject()) {
err_obj = er.As<Object>();

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).
Expand Down Expand Up @@ -1492,14 +1499,15 @@ void AppendExceptionLine(Environment* env,

Local<String> 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);
Expand All @@ -1525,7 +1533,10 @@ static void ReportException(Environment* env,
Local<Object> 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);
Expand Down
60 changes: 35 additions & 25 deletions src/node_contextify.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -288,11 +288,11 @@ class ContextifyContext {
}
Local<Object> sandbox = args[0].As<Object>();

Local<String> 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);
Expand All @@ -305,8 +305,10 @@ class ContextifyContext {
if (context->context().IsEmpty())
return;

Local<External> 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));
}


Expand All @@ -319,10 +321,9 @@ class ContextifyContext {
}
Local<Object> sandbox = args[0].As<Object>();

Local<String> 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());
}


Expand All @@ -342,17 +343,17 @@ class ContextifyContext {


static ContextifyContext* ContextFromContextifiedSandbox(
Isolate* isolate,
Environment* env,
const Local<Object>& sandbox) {
Local<String> hidden_name =
FIXED_ONE_BYTE_STRING(isolate, "_contextifyHidden");
Local<Value> 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<Value> context_external_v;
if (maybe_value.ToLocal(&context_external_v) &&
context_external_v->IsExternal()) {
Local<External> context_external = context_external_v.As<External>();
return static_cast<ContextifyContext*>(context_external->Value());
}
Local<External> context_external = context_external_v.As<External>();

return static_cast<ContextifyContext*>(context_external->Value());
return nullptr;
}


Expand Down Expand Up @@ -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.");
Expand Down Expand Up @@ -654,15 +654,25 @@ class ContextifyScript : public BaseObject {

AppendExceptionLine(env, exception, try_catch.Message());
Local<Value> stack = err_obj->Get(env->stack_string());
Local<Value> 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<Value> arrow;
if (!(maybe_value.ToLocal(&arrow) &&
arrow->IsString() &&
stack->IsString())) {
return;
}

Local<String> decorated_stack = String::Concat(arrow.As<String>(),
stack.As<String>());
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<Value>& args,
Expand Down
Loading

0 comments on commit 924cc6c

Please sign in to comment.