From 334ef4f19dc0bbdf9a9fde9f4a6e7042a6c6e2a3 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 31 May 2016 19:58:31 +0200 Subject: [PATCH] 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: https://github.com/nodejs/node/pull/7082 Reviewed-By: James M Snell Reviewed-By: Trevor Norris --- lib/internal/util.js | 9 ++++--- src/debug-agent.cc | 8 +----- src/env-inl.h | 6 +---- src/node.cc | 8 +----- src/node_util.cc | 38 ++++++++++++++++++++++------- test/parallel/test-util-internal.js | 38 ++++++++++++++++++----------- 6 files changed, 62 insertions(+), 45 deletions(-) diff --git a/lib/internal/util.js b/lib/internal/util.js index 9ecdf17ecda571..a0c2412dce3bdd 100644 --- a/lib/internal/util.js +++ b/lib/internal/util.js @@ -3,6 +3,9 @@ const binding = process.binding('util'); const prefix = `(${process.release.name}:${process.pid}) `; +const kArrowMessagePrivateSymbolIndex = binding['arrow_message_private_symbol']; +const kDecoratedPrivateSymbolIndex = binding['decorated_private_symbol']; + exports.getHiddenValue = binding.getHiddenValue; exports.setHiddenValue = binding.setHiddenValue; @@ -65,14 +68,14 @@ exports._deprecate = function(fn, msg) { exports.decorateErrorStack = function decorateErrorStack(err) { if (!(exports.isError(err) && err.stack) || - exports.getHiddenValue(err, 'node:decorated') === true) + exports.getHiddenValue(err, kDecoratedPrivateSymbolIndex) === true) return; - const arrow = exports.getHiddenValue(err, 'node:arrowMessage'); + const arrow = exports.getHiddenValue(err, kArrowMessagePrivateSymbolIndex); if (arrow) { err.stack = arrow + err.stack; - exports.setHiddenValue(err, 'node:decorated', true); + exports.setHiddenValue(err, kDecoratedPrivateSymbolIndex, true); } }; diff --git a/src/debug-agent.cc b/src/debug-agent.cc index 78eacf49ab2fc8..a8797a5bcf4887 100644 --- a/src/debug-agent.cc +++ b/src/debug-agent.cc @@ -169,16 +169,10 @@ void Agent::WorkerRun() { Isolate::Scope isolate_scope(isolate); HandleScope handle_scope(isolate); + IsolateData isolate_data(isolate, &child_loop_); Local context = Context::New(isolate); Context::Scope context_scope(context); - - // FIXME(bnoordhuis) Work around V8 bug: v8::Private::ForApi() dereferences - // a nullptr when a context hasn't been entered first. The symbol registry - // is a lazily created JS object but object instantiation does not work - // without a context. - IsolateData isolate_data(isolate, &child_loop_); - Environment* env = CreateEnvironment( &isolate_data, context, diff --git a/src/env-inl.h b/src/env-inl.h index cf62ca7eb481db..9836f14da42fbc 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -29,7 +29,7 @@ inline IsolateData::IsolateData(v8::Isolate* isolate, uv_loop_t* event_loop) #define V(PropertyName, StringValue) \ PropertyName ## _( \ isolate, \ - v8::Private::ForApi( \ + v8::Private::New( \ isolate, \ v8::String::NewFromOneByte( \ isolate, \ @@ -545,10 +545,6 @@ inline v8::Local Environment::NewInternalFieldObject() { ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES(V) #undef V -#undef ENVIRONMENT_STRONG_PERSISTENT_PROPERTIES -#undef PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES -#undef PER_ISOLATE_STRING_PROPERTIES - } // namespace node #endif // defined(NODE_WANT_INTERNALS) && NODE_WANT_INTERNALS diff --git a/src/node.cc b/src/node.cc index 025c1a921a884b..2f64e36e5935e6 100644 --- a/src/node.cc +++ b/src/node.cc @@ -4385,15 +4385,9 @@ static void StartNodeInstance(void* arg) { Locker locker(isolate); Isolate::Scope isolate_scope(isolate); HandleScope handle_scope(isolate); + IsolateData isolate_data(isolate, instance_data->event_loop()); Local context = Context::New(isolate); Context::Scope context_scope(context); - - // FIXME(bnoordhuis) Work around V8 bug: v8::Private::ForApi() dereferences - // a nullptr when a context hasn't been entered first. The symbol registry - // is a lazily created JS object but object instantiation does not work - // without a context. - IsolateData isolate_data(isolate, instance_data->event_loop()); - Environment* env = CreateEnvironment(&isolate_data, context, instance_data->argc(), diff --git a/src/node_util.cc b/src/node_util.cc index 9fc94acf094426..5089188d502bf2 100644 --- a/src/node_util.cc +++ b/src/node_util.cc @@ -9,11 +9,11 @@ namespace util { using v8::Array; using v8::Context; using v8::FunctionCallbackInfo; +using v8::Integer; using v8::Local; using v8::Object; using v8::Private; using v8::Proxy; -using v8::String; using v8::Value; @@ -53,18 +53,28 @@ static void GetProxyDetails(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(ret); } +inline Local IndexToPrivateSymbol(Environment* env, uint32_t index) { +#define V(name, _) &Environment::name, + static Local (Environment::*const methods[])() const = { + PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) + }; +#undef V + CHECK_LT(index, arraysize(methods)); + return (env->*methods[index])(); +} + static void GetHiddenValue(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); if (!args[0]->IsObject()) return env->ThrowTypeError("obj must be an object"); - if (!args[1]->IsString()) - return env->ThrowTypeError("name must be a string"); + if (!args[1]->IsUint32()) + return env->ThrowTypeError("index must be an uint32"); Local obj = args[0].As(); - Local name = args[1].As(); - auto private_symbol = Private::ForApi(env->isolate(), name); + auto index = args[1]->Uint32Value(env->context()).FromJust(); + auto private_symbol = IndexToPrivateSymbol(env, index); auto maybe_value = obj->GetPrivate(env->context(), private_symbol); args.GetReturnValue().Set(maybe_value.ToLocalChecked()); @@ -76,12 +86,12 @@ static void SetHiddenValue(const FunctionCallbackInfo& args) { if (!args[0]->IsObject()) return env->ThrowTypeError("obj must be an object"); - if (!args[1]->IsString()) - return env->ThrowTypeError("name must be a string"); + if (!args[1]->IsUint32()) + return env->ThrowTypeError("index must be an uint32"); Local obj = args[0].As(); - Local name = args[1].As(); - auto private_symbol = Private::ForApi(env->isolate(), name); + auto index = args[1]->Uint32Value(env->context()).FromJust(); + auto private_symbol = IndexToPrivateSymbol(env, index); auto maybe_value = obj->SetPrivate(env->context(), private_symbol, args[2]); args.GetReturnValue().Set(maybe_value.FromJust()); @@ -97,6 +107,16 @@ void Initialize(Local target, VALUE_METHOD_MAP(V) #undef V +#define V(name, _) \ + target->Set(context, \ + FIXED_ONE_BYTE_STRING(env->isolate(), #name), \ + Integer::NewFromUnsigned(env->isolate(), index++)); + { + uint32_t index = 0; + PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) + } +#undef V + env->SetMethod(target, "getHiddenValue", GetHiddenValue); env->SetMethod(target, "setHiddenValue", SetHiddenValue); env->SetMethod(target, "getProxyDetails", GetProxyDetails); diff --git a/test/parallel/test-util-internal.js b/test/parallel/test-util-internal.js index 2110b1785131e9..0a6621e3ec0c81 100644 --- a/test/parallel/test-util-internal.js +++ b/test/parallel/test-util-internal.js @@ -6,15 +6,18 @@ const assert = require('assert'); const internalUtil = require('internal/util'); const spawnSync = require('child_process').spawnSync; -function getHiddenValue(obj, name) { +const binding = process.binding('util'); +const kArrowMessagePrivateSymbolIndex = binding['arrow_message_private_symbol']; + +function getHiddenValue(obj, index) { return function() { - internalUtil.getHiddenValue(obj, name); + internalUtil.getHiddenValue(obj, index); }; } -function setHiddenValue(obj, name, val) { +function setHiddenValue(obj, index, val) { return function() { - internalUtil.setHiddenValue(obj, name, val); + internalUtil.setHiddenValue(obj, index, val); }; } @@ -23,29 +26,36 @@ assert.throws(getHiddenValue(null, 'foo'), /obj must be an object/); assert.throws(getHiddenValue(undefined, 'foo'), /obj must be an object/); assert.throws(getHiddenValue('bar', 'foo'), /obj must be an object/); assert.throws(getHiddenValue(85, 'foo'), /obj must be an object/); -assert.throws(getHiddenValue({}), /name must be a string/); -assert.throws(getHiddenValue({}, null), /name must be a string/); -assert.throws(getHiddenValue({}, []), /name must be a string/); -assert.deepStrictEqual(internalUtil.getHiddenValue({}, 'foo'), undefined); +assert.throws(getHiddenValue({}), /index must be an uint32/); +assert.throws(getHiddenValue({}, null), /index must be an uint32/); +assert.throws(getHiddenValue({}, []), /index must be an uint32/); +assert.deepStrictEqual( + internalUtil.getHiddenValue({}, kArrowMessagePrivateSymbolIndex), + undefined); assert.throws(setHiddenValue(), /obj must be an object/); assert.throws(setHiddenValue(null, 'foo'), /obj must be an object/); assert.throws(setHiddenValue(undefined, 'foo'), /obj must be an object/); assert.throws(setHiddenValue('bar', 'foo'), /obj must be an object/); assert.throws(setHiddenValue(85, 'foo'), /obj must be an object/); -assert.throws(setHiddenValue({}), /name must be a string/); -assert.throws(setHiddenValue({}, null), /name must be a string/); -assert.throws(setHiddenValue({}, []), /name must be a string/); +assert.throws(setHiddenValue({}), /index must be an uint32/); +assert.throws(setHiddenValue({}, null), /index must be an uint32/); +assert.throws(setHiddenValue({}, []), /index must be an uint32/); const obj = {}; -assert.strictEqual(internalUtil.setHiddenValue(obj, 'foo', 'bar'), true); -assert.strictEqual(internalUtil.getHiddenValue(obj, 'foo'), 'bar'); +assert.strictEqual( + internalUtil.setHiddenValue(obj, kArrowMessagePrivateSymbolIndex, 'bar'), + true); +assert.strictEqual( + internalUtil.getHiddenValue(obj, kArrowMessagePrivateSymbolIndex), + 'bar'); let arrowMessage; try { require('../fixtures/syntax/bad_syntax'); } catch (err) { - arrowMessage = internalUtil.getHiddenValue(err, 'node:arrowMessage'); + arrowMessage = + internalUtil.getHiddenValue(err, kArrowMessagePrivateSymbolIndex); } assert(/bad_syntax\.js:1/.test(arrowMessage));