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 <jasnell@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>

6 files changed, 62 insertions, 45 deletions

diff --git a/lib/internal/util.js b/lib/internal/util.js
index 9ecdf17ecd..a0c2412dce 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 78eacf49ab..a8797a5bcf 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 = 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 cf62ca7eb4..9836f14da4 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<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
diff --git a/src/node.cc b/src/node.cc
index 025c1a921a..2f64e36e59 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 = 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 9fc94acf09..5089188d50 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<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());
@@ -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());
@@ -97,6 +107,16 @@ void Initialize(Local<Object> 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 2110b17851..0a6621e3ec 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));