Skip to content

Commit

Permalink
lib,src: drop dependency on v8::Private::ForApi()
Browse files Browse the repository at this point in the history 
Said function requires that a v8::Context has been entered first,
introducing a chicken-and-egg problem when creating the first context.

PR-URL: #7082
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
  • Loading branch information
@bnoordhuis
bnoordhuis committed Jun 1, 2016
1 parent c3cd453 commit 334ef4f
Show file tree
Hide file tree
Showing 6 changed files with 62 additions and 45 deletions.
9 changes: 6 additions & 3 deletions lib/internal/util.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);
}
};

Expand Down
8 changes: 1 addition & 7 deletions src/debug-agent.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = 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,
Expand Down
6 changes: 1 addition & 5 deletions src/env-inl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, \
Expand Down Expand Up @@ -545,10 +545,6 @@ inline v8::Local<v8::Object> 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
Expand Down
8 changes: 1 addition & 7 deletions src/node.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = 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(),
Expand Down
38 changes: 29 additions & 9 deletions src/node_util.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;


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

inline Local<Private> IndexToPrivateSymbol(Environment* env, uint32_t index) {
#define V(name, _) &Environment::name,
static Local<Private> (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<Value>& 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<Object> obj = args[0].As<Object>();
Local<String> name = args[1].As<String>();
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());
Expand All @@ -76,12 +86,12 @@ static void SetHiddenValue(const FunctionCallbackInfo<Value>& 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<Object> obj = args[0].As<Object>();
Local<String> name = args[1].As<String>();
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());
Expand All @@ -97,6 +107,16 @@ void Initialize(Local<Object> target,
VALUE_METHOD_MAP(V)
#undef V

#define V(name, _) \
target->Set(context, \

This comment has been minimized.

Sign in to view

Copy link
@trevnorris

trevnorris Aug 29, 2016

Contributor

Bothered that I didn't notice this before, but since this uses the Maybe API it should end with .FromJust(). Currently I'm getting a stack of warnings every time I compile.
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);
Expand Down
38 changes: 24 additions & 14 deletions test/parallel/test-util-internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
};
}

Expand All @@ -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));
Expand Down

3 comments on commit 334ef4f

@trevnorris
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis Does PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V) in src/node_util.cc give you an:

warning: ignoring return value of function declared with warn_unused_result attribute [-Wunused-result]

@bnoordhuis
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see that warning locally, no.

@trevnorris
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm. maybe it's because of ccache. thanks.

Please sign in to comment.