Skip to content

Commit 78f46ee

Browse files
bnoordhuisMichael Scovetta
bnoordhuis
authored and
Michael Scovetta
committed
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: nodejs#5045 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>
1 parent 5356a5c commit 78f46ee

6 files changed

Lines changed: 178 additions & 94 deletions

File tree

‎src/env-inl.h‎

Lines changed: 36 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -50,12 +50,25 @@ inline Environment::IsolateData::IsolateData(v8::Isolate* isolate,
5050
: event_loop_(loop),
5151
isolate_(isolate),
5252
#define V(PropertyName, StringValue) \
53-
PropertyName ## _(isolate, \
54-
v8::String::NewFromOneByte( \
55-
isolate, \
56-
reinterpret_cast<const uint8_t*>(StringValue), \
57-
v8::NewStringType::kInternalized, \
58-
sizeof(StringValue) - 1).ToLocalChecked()),
53+
PropertyName ## _( \
54+
isolate, \
55+
v8::Private::ForApi( \
56+
isolate, \
57+
v8::String::NewFromOneByte( \
58+
isolate, \
59+
reinterpret_cast<const uint8_t*>(StringValue), \
60+
v8::NewStringType::kInternalized, \
61+
sizeof(StringValue) - 1).ToLocalChecked())),
62+
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V)
63+
#undef V
64+
#define V(PropertyName, StringValue) \
65+
PropertyName ## _( \
66+
isolate, \
67+
v8::String::NewFromOneByte( \
68+
isolate, \
69+
reinterpret_cast<const uint8_t*>(StringValue), \
70+
v8::NewStringType::kInternalized, \
71+
sizeof(StringValue) - 1).ToLocalChecked()),
5972
PER_ISOLATE_STRING_PROPERTIES(V)
6073
#undef V
6174
ref_count_(0) {}
@@ -525,21 +538,31 @@ inline v8::Local<v8::Object> Environment::NewInternalFieldObject() {
525538
return m_obj.ToLocalChecked();
526539
}
527540

528-
#define V(PropertyName, StringValue) \
541+
#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue)
542+
#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue)
543+
#define V(TypeName, PropertyName, StringValue) \
529544
inline \
530-
v8::Local<v8::String> Environment::IsolateData::PropertyName() const { \
545+
v8::Local<TypeName> Environment::IsolateData::PropertyName() const { \
531546
/* Strings are immutable so casting away const-ness here is okay. */ \
532547
return const_cast<IsolateData*>(this)->PropertyName ## _.Get(isolate()); \
533548
}
534-
PER_ISOLATE_STRING_PROPERTIES(V)
549+
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP)
550+
PER_ISOLATE_STRING_PROPERTIES(VS)
535551
#undef V
552+
#undef VS
553+
#undef VP
536554

537-
#define V(PropertyName, StringValue) \
538-
inline v8::Local<v8::String> Environment::PropertyName() const { \
555+
#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue)
556+
#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue)
557+
#define V(TypeName, PropertyName, StringValue) \
558+
inline v8::Local<TypeName> Environment::PropertyName() const { \
539559
return isolate_data()->PropertyName(); \
540560
}
541-
PER_ISOLATE_STRING_PROPERTIES(V)
561+
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP)
562+
PER_ISOLATE_STRING_PROPERTIES(VS)
542563
#undef V
564+
#undef VS
565+
#undef VP
543566

544567
#define V(PropertyName, TypeName) \
545568
inline v8::Local<TypeName> Environment::PropertyName() const { \
@@ -552,6 +575,7 @@ inline v8::Local<v8::Object> Environment::NewInternalFieldObject() {
552575
#undef V
553576

554577
#undef ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES
578+
#undef PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES
555579
#undef PER_ISOLATE_STRING_PROPERTIES
556580

557581
} // namespace node

‎src/env.h‎

Lines changed: 38 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -44,14 +44,24 @@ namespace node {
4444
#define NODE_PUSH_VAL_TO_ARRAY_MAX 8
4545
#endif
4646

47+
// Private symbols are per-isolate primitives but Environment proxies them
48+
// for the sake of convenience. Strings should be ASCII-only and have a
49+
// "node:" prefix to avoid name clashes with third-party code.
50+
#define PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) \
51+
V(alpn_buffer_private_symbol, "node:alpnBuffer") \
52+
V(arrow_message_private_symbol, "node:arrowMessage") \
53+
V(contextify_private_symbol, "node:contextify") \
54+
V(decorated_private_symbol, "node:decorated") \
55+
V(npn_buffer_private_symbol, "node:npnBuffer") \
56+
V(processed_private_symbol, "node:processed") \
57+
V(selected_npn_buffer_private_symbol, "node:selectedNpnBuffer") \
58+
4759
// Strings are per-isolate primitives but Environment proxies them
4860
// for the sake of convenience. Strings should be ASCII-only.
4961
#define PER_ISOLATE_STRING_PROPERTIES(V) \
5062
V(address_string, "address") \
51-
V(alpn_buffer_string, "alpnBuffer") \
5263
V(args_string, "args") \
5364
V(argv_string, "argv") \
54-
V(arrow_message_string, "node:arrowMessage") \
5565
V(async, "async") \
5666
V(async_queue_string, "_asyncQueue") \
5767
V(atime_string, "atime") \
@@ -73,7 +83,6 @@ namespace node {
7383
V(cwd_string, "cwd") \
7484
V(debug_port_string, "debugPort") \
7585
V(debug_string, "debug") \
76-
V(decorated_string, "node:decorated") \
7786
V(dest_string, "dest") \
7887
V(detached_string, "detached") \
7988
V(dev_string, "dev") \
@@ -140,7 +149,6 @@ namespace node {
140149
V(netmask_string, "netmask") \
141150
V(nice_string, "nice") \
142151
V(nlink_string, "nlink") \
143-
V(npn_buffer_string, "npnBuffer") \
144152
V(nsname_string, "nsname") \
145153
V(ocsp_request_string, "OCSPRequest") \
146154
V(offset_string, "offset") \
@@ -176,7 +184,6 @@ namespace node {
176184
V(port_string, "port") \
177185
V(preference_string, "preference") \
178186
V(priority_string, "priority") \
179-
V(processed_string, "processed") \
180187
V(produce_cached_data_string, "produceCachedData") \
181188
V(prototype_string, "prototype") \
182189
V(raw_string, "raw") \
@@ -192,7 +199,6 @@ namespace node {
192199
V(serial_string, "serial") \
193200
V(scavenge_string, "scavenge") \
194201
V(scopeid_string, "scopeid") \
195-
V(selected_npn_buffer_string, "selectedNpnBuffer") \
196202
V(sent_shutdown_string, "sentShutdown") \
197203
V(serial_number_string, "serialNumber") \
198204
V(service_string, "service") \
@@ -507,12 +513,17 @@ class Environment {
507513

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

510-
// Strings are shared across shared contexts. The getters simply proxy to
511-
// the per-isolate primitive.
512-
#define V(PropertyName, StringValue) \
513-
inline v8::Local<v8::String> PropertyName() const;
514-
PER_ISOLATE_STRING_PROPERTIES(V)
516+
// Strings and private symbols are shared across shared contexts
517+
// The getters simply proxy to the per-isolate primitive.
518+
#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue)
519+
#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue)
520+
#define V(TypeName, PropertyName, StringValue) \
521+
inline v8::Local<TypeName> PropertyName() const;
522+
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP)
523+
PER_ISOLATE_STRING_PROPERTIES(VS)
515524
#undef V
525+
#undef VS
526+
#undef VP
516527

517528
#define V(PropertyName, TypeName) \
518529
inline v8::Local<TypeName> PropertyName() const; \
@@ -585,10 +596,15 @@ class Environment {
585596
inline void Put();
586597
inline uv_loop_t* event_loop() const;
587598

588-
#define V(PropertyName, StringValue) \
589-
inline v8::Local<v8::String> PropertyName() const;
590-
PER_ISOLATE_STRING_PROPERTIES(V)
599+
#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue)
600+
#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue)
601+
#define V(TypeName, PropertyName, StringValue) \
602+
inline v8::Local<TypeName> PropertyName() const;
603+
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP)
604+
PER_ISOLATE_STRING_PROPERTIES(VS)
591605
#undef V
606+
#undef VS
607+
#undef VP
592608

593609
private:
594610
inline static IsolateData* Get(v8::Isolate* isolate);
@@ -598,10 +614,15 @@ class Environment {
598614
uv_loop_t* const event_loop_;
599615
v8::Isolate* const isolate_;
600616

601-
#define V(PropertyName, StringValue) \
602-
v8::Eternal<v8::String> PropertyName ## _;
603-
PER_ISOLATE_STRING_PROPERTIES(V)
617+
#define VP(PropertyName, StringValue) V(v8::Private, PropertyName, StringValue)
618+
#define VS(PropertyName, StringValue) V(v8::String, PropertyName, StringValue)
619+
#define V(TypeName, PropertyName, StringValue) \
620+
v8::Eternal<TypeName> PropertyName ## _;
621+
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(VP)
622+
PER_ISOLATE_STRING_PROPERTIES(VS)
604623
#undef V
624+
#undef VS
625+
#undef VP
605626

606627
unsigned int ref_count_;
607628

‎src/node.cc‎

Lines changed: 23 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1402,8 +1402,10 @@ ssize_t DecodeWrite(Isolate* isolate,
14021402
bool IsExceptionDecorated(Environment* env, Local<Value> er) {
14031403
if (!er.IsEmpty() && er->IsObject()) {
14041404
Local<Object> err_obj = er.As<Object>();
1405-
Local<Value> decorated = err_obj->GetHiddenValue(env->decorated_string());
1406-
return !decorated.IsEmpty() && decorated->IsTrue();
1405+
auto maybe_value =
1406+
err_obj->GetPrivate(env->context(), env->decorated_private_symbol());
1407+
Local<Value> decorated;
1408+
return maybe_value.ToLocal(&decorated) && decorated->IsTrue();
14071409
}
14081410
return false;
14091411
}
@@ -1419,10 +1421,15 @@ void AppendExceptionLine(Environment* env,
14191421
if (!er.IsEmpty() && er->IsObject()) {
14201422
err_obj = er.As<Object>();
14211423

1424+
auto context = env->context();
1425+
auto processed_private_symbol = env->processed_private_symbol();
14221426
// Do it only once per message
1423-
if (!err_obj->GetHiddenValue(env->processed_string()).IsEmpty())
1427+
if (err_obj->HasPrivate(context, processed_private_symbol).FromJust())
14241428
return;
1425-
err_obj->SetHiddenValue(env->processed_string(), True(env->isolate()));
1429+
err_obj->SetPrivate(
1430+
context,
1431+
processed_private_symbol,
1432+
True(env->isolate()));
14261433
}
14271434

14281435
// Print (filename):(line number): (message).
@@ -1492,14 +1499,15 @@ void AppendExceptionLine(Environment* env,
14921499

14931500
Local<String> arrow_str = String::NewFromUtf8(env->isolate(), arrow);
14941501

1495-
// Allocation failed, just print it out
1496-
if (arrow_str.IsEmpty() || err_obj.IsEmpty() || !err_obj->IsNativeError())
1497-
goto print;
1498-
1499-
err_obj->SetHiddenValue(env->arrow_message_string(), arrow_str);
1500-
return;
1502+
if (!arrow_str.IsEmpty() && !err_obj.IsEmpty() && err_obj->IsNativeError()) {
1503+
err_obj->SetPrivate(
1504+
env->context(),
1505+
env->arrow_message_private_symbol(),
1506+
arrow_str);
1507+
return;
1508+
}
15011509

1502-
print:
1510+
// Allocation failed, just print it out.
15031511
if (env->printed_error())
15041512
return;
15051513
env->set_printed_error(true);
@@ -1525,7 +1533,10 @@ static void ReportException(Environment* env,
15251533
Local<Object> err_obj = er->ToObject(env->isolate());
15261534

15271535
trace_value = err_obj->Get(env->stack_string());
1528-
arrow = err_obj->GetHiddenValue(env->arrow_message_string());
1536+
arrow =
1537+
err_obj->GetPrivate(
1538+
env->context(),
1539+
env->arrow_message_private_symbol()).ToLocalChecked();
15291540
}
15301541

15311542
node::Utf8Value trace(env->isolate(), trace_value);

‎src/node_contextify.cc‎

Lines changed: 35 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -288,11 +288,11 @@ class ContextifyContext {
288288
}
289289
Local<Object> sandbox = args[0].As<Object>();
290290

291-
Local<String> hidden_name =
292-
FIXED_ONE_BYTE_STRING(env->isolate(), "_contextifyHidden");
293-
294291
// Don't allow contextifying a sandbox multiple times.
295-
CHECK(sandbox->GetHiddenValue(hidden_name).IsEmpty());
292+
CHECK(
293+
!sandbox->HasPrivate(
294+
env->context(),
295+
env->contextify_private_symbol()).FromJust());
296296

297297
TryCatch try_catch;
298298
ContextifyContext* context = new ContextifyContext(env, sandbox);
@@ -305,8 +305,10 @@ class ContextifyContext {
305305
if (context->context().IsEmpty())
306306
return;
307307

308-
Local<External> hidden_context = External::New(env->isolate(), context);
309-
sandbox->SetHiddenValue(hidden_name, hidden_context);
308+
sandbox->SetPrivate(
309+
env->context(),
310+
env->contextify_private_symbol(),
311+
External::New(env->isolate(), context));
310312
}
311313

312314

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

322-
Local<String> hidden_name =
323-
FIXED_ONE_BYTE_STRING(env->isolate(), "_contextifyHidden");
324-
325-
args.GetReturnValue().Set(!sandbox->GetHiddenValue(hidden_name).IsEmpty());
324+
auto result =
325+
sandbox->HasPrivate(env->context(), env->contextify_private_symbol());
326+
args.GetReturnValue().Set(result.FromJust());
326327
}
327328

328329

@@ -342,17 +343,17 @@ class ContextifyContext {
342343

343344

344345
static ContextifyContext* ContextFromContextifiedSandbox(
345-
Isolate* isolate,
346+
Environment* env,
346347
const Local<Object>& sandbox) {
347-
Local<String> hidden_name =
348-
FIXED_ONE_BYTE_STRING(isolate, "_contextifyHidden");
349-
Local<Value> context_external_v = sandbox->GetHiddenValue(hidden_name);
350-
if (context_external_v.IsEmpty() || !context_external_v->IsExternal()) {
351-
return nullptr;
348+
auto maybe_value =
349+
sandbox->GetPrivate(env->context(), env->contextify_private_symbol());
350+
Local<Value> context_external_v;
351+
if (maybe_value.ToLocal(&context_external_v) &&
352+
context_external_v->IsExternal()) {
353+
Local<External> context_external = context_external_v.As<External>();
354+
return static_cast<ContextifyContext*>(context_external->Value());
352355
}
353-
Local<External> context_external = context_external_v.As<External>();
354-
355-
return static_cast<ContextifyContext*>(context_external->Value());
356+
return nullptr;
356357
}
357358

358359

@@ -612,8 +613,7 @@ class ContextifyScript : public BaseObject {
612613

613614
// Get the context from the sandbox
614615
ContextifyContext* contextify_context =
615-
ContextifyContext::ContextFromContextifiedSandbox(env->isolate(),
616-
sandbox);
616+
ContextifyContext::ContextFromContextifiedSandbox(env, sandbox);
617617
if (contextify_context == nullptr) {
618618
return env->ThrowTypeError(
619619
"sandbox argument must have been converted to a context.");
@@ -654,15 +654,25 @@ class ContextifyScript : public BaseObject {
654654

655655
AppendExceptionLine(env, exception, try_catch.Message());
656656
Local<Value> stack = err_obj->Get(env->stack_string());
657-
Local<Value> arrow = err_obj->GetHiddenValue(env->arrow_message_string());
658-
659-
if (!(stack->IsString() && arrow->IsString()))
657+
auto maybe_value =
658+
err_obj->GetPrivate(
659+
env->context(),
660+
env->arrow_message_private_symbol());
661+
662+
Local<Value> arrow;
663+
if (!(maybe_value.ToLocal(&arrow) &&
664+
arrow->IsString() &&
665+
stack->IsString())) {
660666
return;
667+
}
661668

662669
Local<String> decorated_stack = String::Concat(arrow.As<String>(),
663670
stack.As<String>());
664671
err_obj->Set(env->stack_string(), decorated_stack);
665-
err_obj->SetHiddenValue(env->decorated_string(), True(env->isolate()));
672+
err_obj->SetPrivate(
673+
env->context(),
674+
env->decorated_private_symbol(),
675+
True(env->isolate()));
666676
}
667677

668678
static int64_t GetTimeoutArg(const FunctionCallbackInfo<Value>& args,

0 commit comments

Comments
 (0)