Skip to content

Commit 334ef4f

Browse files
bnoordhuisbnoordhuis
committed
lib,src: drop dependency on v8::Private::ForApi()
Said function requires that a v8::Context has been entered first, introducing a chicken-and-egg problem when creating the first context. PR-URL: nodejs#7082 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com>
1 parent c3cd453 commit 334ef4f

6 files changed

Lines changed: 62 additions & 45 deletions

File tree

‎lib/internal/util.js‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,9 @@
33
const binding = process.binding('util');
44
const prefix = `(${process.release.name}:${process.pid}) `;
55

6+
const kArrowMessagePrivateSymbolIndex = binding['arrow_message_private_symbol'];
7+
const kDecoratedPrivateSymbolIndex = binding['decorated_private_symbol'];
8+
69
exports.getHiddenValue = binding.getHiddenValue;
710
exports.setHiddenValue = binding.setHiddenValue;
811

@@ -65,14 +68,14 @@ exports._deprecate = function(fn, msg) {
6568

6669
exports.decorateErrorStack = function decorateErrorStack(err) {
6770
if (!(exports.isError(err) && err.stack) ||
68-
exports.getHiddenValue(err, 'node:decorated') === true)
71+
exports.getHiddenValue(err, kDecoratedPrivateSymbolIndex) === true)
6972
return;
7073

71-
const arrow = exports.getHiddenValue(err, 'node:arrowMessage');
74+
const arrow = exports.getHiddenValue(err, kArrowMessagePrivateSymbolIndex);
7275

7376
if (arrow) {
7477
err.stack = arrow + err.stack;
75-
exports.setHiddenValue(err, 'node:decorated', true);
78+
exports.setHiddenValue(err, kDecoratedPrivateSymbolIndex, true);
7679
}
7780
};
7881

‎src/debug-agent.cc‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -169,16 +169,10 @@ void Agent::WorkerRun() {
169169
Isolate::Scope isolate_scope(isolate);
170170

171171
HandleScope handle_scope(isolate);
172+
IsolateData isolate_data(isolate, &child_loop_);
172173
Local<Context> context = Context::New(isolate);
173174

174175
Context::Scope context_scope(context);
175-
176-
// FIXME(bnoordhuis) Work around V8 bug: v8::Private::ForApi() dereferences
177-
// a nullptr when a context hasn't been entered first. The symbol registry
178-
// is a lazily created JS object but object instantiation does not work
179-
// without a context.
180-
IsolateData isolate_data(isolate, &child_loop_);
181-
182176
Environment* env = CreateEnvironment(
183177
&isolate_data,
184178
context,

‎src/env-inl.h‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop)
2929
#define V(PropertyName, StringValue) \
3030
PropertyName ## _( \
3131
isolate, \
32-
v8::Private::ForApi( \
32+
v8::Private::New( \
3333
isolate, \
3434
v8::String::NewFromOneByte( \
3535
isolate, \
@@ -545,10 +545,6 @@ inline v8::Local<v8::Object> Environment::NewInternalFieldObject() {
545545
ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V)
546546
#undef V
547547

548-
#undef ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES
549-
#undef PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES
550-
#undef PER_ISOLATE_STRING_PROPERTIES
551-
552548
} // namespace node
553549

554550
#endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS

‎src/node.cc‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4385,15 +4385,9 @@ static void StartNodeInstance(void* arg) {
43854385
Locker locker(isolate);
43864386
Isolate::Scope isolate_scope(isolate);
43874387
HandleScope handle_scope(isolate);
4388+
IsolateData isolate_data(isolate, instance_data->event_loop());
43884389
Local<Context> context = Context::New(isolate);
43894390
Context::Scope context_scope(context);
4390-
4391-
// FIXME(bnoordhuis) Work around V8 bug: v8::Private::ForApi() dereferences
4392-
// a nullptr when a context hasn't been entered first. The symbol registry
4393-
// is a lazily created JS object but object instantiation does not work
4394-
// without a context.
4395-
IsolateData isolate_data(isolate, instance_data->event_loop());
4396-
43974391
Environment* env = CreateEnvironment(&isolate_data,
43984392
context,
43994393
instance_data->argc(),

‎src/node_util.cc‎

Lines changed: 29 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,11 @@ namespace util {
99
using v8::Array;
1010
using v8::Context;
1111
using v8::FunctionCallbackInfo;
12+
using v8::Integer;
1213
using v8::Local;
1314
using v8::Object;
1415
using v8::Private;
1516
using v8::Proxy;
16-
using v8::String;
1717
using v8::Value;
1818

1919

@@ -53,18 +53,28 @@ static void GetProxyDetails(const FunctionCallbackInfo<Value>& args) {
5353
args.GetReturnValue().Set(ret);
5454
}
5555

56+
inline Local<Private> IndexToPrivateSymbol(Environment* env, uint32_t index) {
57+
#define V(name, _) &Environment::name,
58+
static Local<Private> (Environment::*const methods[])() const = {
59+
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V)
60+
};
61+
#undef V
62+
CHECK_LT(index, arraysize(methods));
63+
return (env->*methods[index])();
64+
}
65+
5666
static void GetHiddenValue(const FunctionCallbackInfo<Value>& args) {
5767
Environment* env = Environment::GetCurrent(args);
5868

5969
if (!args[0]->IsObject())
6070
return env->ThrowTypeError("obj must be an object");
6171

62-
if (!args[1]->IsString())
63-
return env->ThrowTypeError("name must be a string");
72+
if (!args[1]->IsUint32())
73+
return env->ThrowTypeError("index must be an uint32");
6474

6575
Local<Object> obj = args[0].As<Object>();
66-
Local<String> name = args[1].As<String>();
67-
auto private_symbol = Private::ForApi(env->isolate(), name);
76+
auto index = args[1]->Uint32Value(env->context()).FromJust();
77+
auto private_symbol = IndexToPrivateSymbol(env, index);
6878
auto maybe_value = obj->GetPrivate(env->context(), private_symbol);
6979

7080
args.GetReturnValue().Set(maybe_value.ToLocalChecked());
@@ -76,12 +86,12 @@ static void SetHiddenValue(const FunctionCallbackInfo<Value>& args) {
7686
if (!args[0]->IsObject())
7787
return env->ThrowTypeError("obj must be an object");
7888

79-
if (!args[1]->IsString())
80-
return env->ThrowTypeError("name must be a string");
89+
if (!args[1]->IsUint32())
90+
return env->ThrowTypeError("index must be an uint32");
8191

8292
Local<Object> obj = args[0].As<Object>();
83-
Local<String> name = args[1].As<String>();
84-
auto private_symbol = Private::ForApi(env->isolate(), name);
93+
auto index = args[1]->Uint32Value(env->context()).FromJust();
94+
auto private_symbol = IndexToPrivateSymbol(env, index);
8595
auto maybe_value = obj->SetPrivate(env->context(), private_symbol, args[2]);
8696

8797
args.GetReturnValue().Set(maybe_value.FromJust());
@@ -97,6 +107,16 @@ void Initialize(Local<Object> target,
97107
VALUE_METHOD_MAP(V)
98108
#undef V
99109

110+
#define V(name, _) \
111+
target->Set(context, \
112+
FIXED_ONE_BYTE_STRING(env->isolate(), #name), \
113+
Integer::NewFromUnsigned(env->isolate(), index++));
114+
{
115+
uint32_t index = 0;
116+
PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V)
117+
}
118+
#undef V
119+
100120
env->SetMethod(target, "getHiddenValue", GetHiddenValue);
101121
env->SetMethod(target, "setHiddenValue", SetHiddenValue);
102122
env->SetMethod(target, "getProxyDetails", GetProxyDetails);

‎test/parallel/test-util-internal.js‎

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,18 @@ const assert = require('assert');
66
const internalUtil = require('internal/util');
77
const spawnSync = require('child_process').spawnSync;
88

9-
function getHiddenValue(obj, name) {
9+
const binding = process.binding('util');
10+
const kArrowMessagePrivateSymbolIndex = binding['arrow_message_private_symbol'];
11+
12+
function getHiddenValue(obj, index) {
1013
return function() {
11-
internalUtil.getHiddenValue(obj, name);
14+
internalUtil.getHiddenValue(obj, index);
1215
};
1316
}
1417

15-
function setHiddenValue(obj, name, val) {
18+
function setHiddenValue(obj, index, val) {
1619
return function() {
17-
internalUtil.setHiddenValue(obj, name, val);
20+
internalUtil.setHiddenValue(obj, index, val);
1821
};
1922
}
2023

@@ -23,29 +26,36 @@ assert.throws(getHiddenValue(null, 'foo'), /obj must be an object/);
2326
assert.throws(getHiddenValue(undefined, 'foo'), /obj must be an object/);
2427
assert.throws(getHiddenValue('bar', 'foo'), /obj must be an object/);
2528
assert.throws(getHiddenValue(85, 'foo'), /obj must be an object/);
26-
assert.throws(getHiddenValue({}), /name must be a string/);
27-
assert.throws(getHiddenValue({}, null), /name must be a string/);
28-
assert.throws(getHiddenValue({}, []), /name must be a string/);
29-
assert.deepStrictEqual(internalUtil.getHiddenValue({}, 'foo'), undefined);
29+
assert.throws(getHiddenValue({}), /index must be an uint32/);
30+
assert.throws(getHiddenValue({}, null), /index must be an uint32/);
31+
assert.throws(getHiddenValue({}, []), /index must be an uint32/);
32+
assert.deepStrictEqual(
33+
internalUtil.getHiddenValue({}, kArrowMessagePrivateSymbolIndex),
34+
undefined);
3035

3136
assert.throws(setHiddenValue(), /obj must be an object/);
3237
assert.throws(setHiddenValue(null, 'foo'), /obj must be an object/);
3338
assert.throws(setHiddenValue(undefined, 'foo'), /obj must be an object/);
3439
assert.throws(setHiddenValue('bar', 'foo'), /obj must be an object/);
3540
assert.throws(setHiddenValue(85, 'foo'), /obj must be an object/);
36-
assert.throws(setHiddenValue({}), /name must be a string/);
37-
assert.throws(setHiddenValue({}, null), /name must be a string/);
38-
assert.throws(setHiddenValue({}, []), /name must be a string/);
41+
assert.throws(setHiddenValue({}), /index must be an uint32/);
42+
assert.throws(setHiddenValue({}, null), /index must be an uint32/);
43+
assert.throws(setHiddenValue({}, []), /index must be an uint32/);
3944
const obj = {};
40-
assert.strictEqual(internalUtil.setHiddenValue(obj, 'foo', 'bar'), true);
41-
assert.strictEqual(internalUtil.getHiddenValue(obj, 'foo'), 'bar');
45+
assert.strictEqual(
46+
internalUtil.setHiddenValue(obj, kArrowMessagePrivateSymbolIndex, 'bar'),
47+
true);
48+
assert.strictEqual(
49+
internalUtil.getHiddenValue(obj, kArrowMessagePrivateSymbolIndex),
50+
'bar');
4251

4352
let arrowMessage;
4453

4554
try {
4655
require('../fixtures/syntax/bad_syntax');
4756
} catch (err) {
48-
arrowMessage = internalUtil.getHiddenValue(err, 'node:arrowMessage');
57+
arrowMessage =
58+
internalUtil.getHiddenValue(err, kArrowMessagePrivateSymbolIndex);
4959
}
5060

5161
assert(/bad_syntax\.js:1/.test(arrowMessage));

0 commit comments

Comments
 (0)