Skip to content

Commit ecdae5b

Browse files
ChALkeRChALkeR
committed
Revert "lib,src: drop dependency on v8::Private::ForApi()"
This reverts commit 334ef4f. nodejs#7082 was landed too fast and did not have sufficient review time. That PR also broke some things (testcases will follow separately).
1 parent 5dc1f6f commit ecdae5b

6 files changed

Lines changed: 45 additions & 62 deletions

File tree

‎lib/internal/util.js‎

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,6 @@
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-
96
exports.getHiddenValue = binding.getHiddenValue;
107
exports.setHiddenValue = binding.setHiddenValue;
118

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

6966
exports.decorateErrorStack = function decorateErrorStack(err) {
7067
if (!(exports.isError(err) && err.stack) ||
71-
exports.getHiddenValue(err, kDecoratedPrivateSymbolIndex) === true)
68+
exports.getHiddenValue(err, 'node:decorated') === true)
7269
return;
7370

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

7673
if (arrow) {
7774
err.stack = arrow + err.stack;
78-
exports.setHiddenValue(err, kDecoratedPrivateSymbolIndex, true);
75+
exports.setHiddenValue(err, 'node:decorated', true);
7976
}
8077
};
8178

‎src/debug-agent.cc‎

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

171171
HandleScope handle_scope(isolate);
172-
IsolateData isolate_data(isolate, &child_loop_);
173172
Local<Context> context = Context::New(isolate);
174173

175174
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+
176182
Environment* env = CreateEnvironment(
177183
&isolate_data,
178184
context,

‎src/env-inl.h‎

Lines changed: 5 additions & 1 deletion
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::New( \
32+
v8::Private::ForApi( \
3333
isolate, \
3434
v8::String::NewFromOneByte( \
3535
isolate, \
@@ -545,6 +545,10 @@ 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+
548552
} // namespace node
549553

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

‎src/node.cc‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4385,9 +4385,15 @@ 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());
43894388
Local<Context> context = Context::New(isolate);
43904389
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+
43914397
Environment* env = CreateEnvironment(&isolate_data,
43924398
context,
43934399
instance_data->argc(),

‎src/node_util.cc‎

Lines changed: 9 additions & 29 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;
1312
using v8::Local;
1413
using v8::Object;
1514
using v8::Private;
1615
using v8::Proxy;
16+
using v8::String;
1717
using v8::Value;
1818

1919

@@ -53,28 +53,18 @@ 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-
6656
static void GetHiddenValue(const FunctionCallbackInfo<Value>& args) {
6757
Environment* env = Environment::GetCurrent(args);
6858

6959
if (!args[0]->IsObject())
7060
return env->ThrowTypeError("obj must be an object");
7161

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

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

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

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

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

9787
args.GetReturnValue().Set(maybe_value.FromJust());
@@ -107,16 +97,6 @@ void Initialize(Local<Object> target,
10797
VALUE_METHOD_MAP(V)
10898
#undef V
10999

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-
120100
env->SetMethod(target, "getHiddenValue", GetHiddenValue);
121101
env->SetMethod(target, "setHiddenValue", SetHiddenValue);
122102
env->SetMethod(target, "getProxyDetails", GetProxyDetails);

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

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

9-
const binding = process.binding('util');
10-
const kArrowMessagePrivateSymbolIndex = binding['arrow_message_private_symbol'];
11-
12-
function getHiddenValue(obj, index) {
9+
function getHiddenValue(obj, name) {
1310
return function() {
14-
internalUtil.getHiddenValue(obj, index);
11+
internalUtil.getHiddenValue(obj, name);
1512
};
1613
}
1714

18-
function setHiddenValue(obj, index, val) {
15+
function setHiddenValue(obj, name, val) {
1916
return function() {
20-
internalUtil.setHiddenValue(obj, index, val);
17+
internalUtil.setHiddenValue(obj, name, val);
2118
};
2219
}
2320

@@ -26,36 +23,29 @@ assert.throws(getHiddenValue(null, 'foo'), /obj must be an object/);
2623
assert.throws(getHiddenValue(undefined, 'foo'), /obj must be an object/);
2724
assert.throws(getHiddenValue('bar', 'foo'), /obj must be an object/);
2825
assert.throws(getHiddenValue(85, 'foo'), /obj must be an object/);
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);
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);
3530

3631
assert.throws(setHiddenValue(), /obj must be an object/);
3732
assert.throws(setHiddenValue(null, 'foo'), /obj must be an object/);
3833
assert.throws(setHiddenValue(undefined, 'foo'), /obj must be an object/);
3934
assert.throws(setHiddenValue('bar', 'foo'), /obj must be an object/);
4035
assert.throws(setHiddenValue(85, 'foo'), /obj must be an object/);
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/);
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/);
4439
const obj = {};
45-
assert.strictEqual(
46-
internalUtil.setHiddenValue(obj, kArrowMessagePrivateSymbolIndex, 'bar'),
47-
true);
48-
assert.strictEqual(
49-
internalUtil.getHiddenValue(obj, kArrowMessagePrivateSymbolIndex),
50-
'bar');
40+
assert.strictEqual(internalUtil.setHiddenValue(obj, 'foo', 'bar'), true);
41+
assert.strictEqual(internalUtil.getHiddenValue(obj, 'foo'), 'bar');
5142

5243
let arrowMessage;
5344

5445
try {
5546
require('../fixtures/syntax/bad_syntax');
5647
} catch (err) {
57-
arrowMessage =
58-
internalUtil.getHiddenValue(err, kArrowMessagePrivateSymbolIndex);
48+
arrowMessage = internalUtil.getHiddenValue(err, 'node:arrowMessage');
5949
}
6050

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

0 commit comments

Comments
 (0)