util: use private symbols in JS land directly

Instead of calling into C++ to use the private symbols, use an ObjectTemplate to create an object that holds the symbols and use them directly from JS land. PR-URL: nodejs#45379 Reviewed-By: Chengzhong Wu <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: James M Snell <[email protected]>
m5l14i11 · Nov 17, 2022 · 066e77a · 066e77a
1 parent 490c3d6
commit 066e77a
Show file tree

Hide file tree

Showing 7 changed files with 51 additions and 94 deletions.
diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js
@@ -80,8 +80,9 @@ const {
   validateInteger,
 } = require('internal/validators');
 const {
-  exit_info_private_symbol,
-  getHiddenValue,
+  privateSymbols: {
+    exit_info_private_symbol,
+  },
   kExitCode,
   kExiting,
   kHasExitCode,
@@ -96,8 +97,7 @@ process.domain = null;
 
 // process._exiting and process.exitCode
 {
-  const fields = getHiddenValue(process, exit_info_private_symbol);
-
+  const fields = process[exit_info_private_symbol];
   ObjectDefineProperty(process, '_exiting', {
     __proto__: null,
     get() {

diff --git a/lib/internal/buffer.js b/lib/internal/buffer.js
@@ -32,9 +32,11 @@ const {
   utf8Write,
   getZeroFillToggle
 } = internalBinding('buffer');
+
 const {
-  untransferable_object_private_symbol,
-  setHiddenValue,
+  privateSymbols: {
+    untransferable_object_private_symbol,
+  },
 } = internalBinding('util');
 
 // Temporary buffers to convert numbers.
@@ -1048,7 +1050,7 @@ function addBufferPrototypeMethods(proto) {
 function markAsUntransferable(obj) {
   if ((typeof obj !== 'object' && typeof obj !== 'function') || obj === null)
     return;  // This object is a primitive and therefore already untransferable.
-  setHiddenValue(obj, untransferable_object_private_symbol, true);
+  obj[untransferable_object_private_symbol] = true;
 }
 
 // A toggle used to access the zero fill setting of the array buffer allocator

diff --git a/lib/internal/errors.js b/lib/internal/errors.js
@@ -809,16 +809,14 @@ const fatalExceptionStackEnhancers = {
   }
 };
 
+const {
+  privateSymbols: {
+    arrow_message_private_symbol,
+  }
+} = internalBinding('util');
 // Ensures the printed error line is from user code.
-let _kArrowMessagePrivateSymbol, _setHiddenValue;
 function setArrowMessage(err, arrowMessage) {
-  if (!_kArrowMessagePrivateSymbol) {
-    ({
-      arrow_message_private_symbol: _kArrowMessagePrivateSymbol,
-      setHiddenValue: _setHiddenValue,
-    } = internalBinding('util'));
-  }
-  _setHiddenValue(err, _kArrowMessagePrivateSymbol, arrowMessage);
+  err[arrow_message_private_symbol] = arrowMessage;
 }
 
 // Hide stack lines before the first user code line.

diff --git a/lib/internal/util.js b/lib/internal/util.js
@@ -42,10 +42,10 @@ const {
 } = require('internal/errors');
 const { signals } = internalBinding('constants').os;
 const {
-  getHiddenValue,
-  setHiddenValue,
-  arrow_message_private_symbol: kArrowMessagePrivateSymbolIndex,
-  decorated_private_symbol: kDecoratedPrivateSymbolIndex,
+  privateSymbols: {
+    arrow_message_private_symbol,
+    decorated_private_symbol,
+  },
   sleep: _sleep,
   toUSVString: _toUSVString,
 } = internalBinding('util');
@@ -143,15 +143,14 @@ function deprecate(fn, msg, code, useEmitSync) {
 }
 
 function decorateErrorStack(err) {
-  if (!(isError(err) && err.stack) ||
-      getHiddenValue(err, kDecoratedPrivateSymbolIndex) === true)
+  if (!(isError(err) && err.stack) || err[decorated_private_symbol])
     return;
 
-  const arrow = getHiddenValue(err, kArrowMessagePrivateSymbolIndex);
+  const arrow = err[arrow_message_private_symbol];
 
   if (arrow) {
     err.stack = arrow + err.stack;
-    setHiddenValue(err, kDecoratedPrivateSymbolIndex, true);
+    err[decorated_private_symbol] = true;
   }
 }
 

diff --git a/src/node_util.cc b/src/node_util.cc
@@ -26,7 +26,6 @@ using v8::Object;
 using v8::ONLY_CONFIGURABLE;
 using v8::ONLY_ENUMERABLE;
 using v8::ONLY_WRITABLE;
-using v8::Private;
 using v8::Promise;
 using v8::PropertyFilter;
 using v8::Proxy;
@@ -159,44 +158,6 @@ static void PreviewEntries(const FunctionCallbackInfo<Value>& args) {
       Array::New(env->isolate(), ret, arraysize(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);
-
-  CHECK(args[0]->IsObject());
-  CHECK(args[1]->IsUint32());
-
-  Local<Object> obj = args[0].As<Object>();
-  uint32_t index = args[1].As<Uint32>()->Value();
-  Local<Private> private_symbol = IndexToPrivateSymbol(env, index);
-  Local<Value> ret;
-  if (obj->GetPrivate(env->context(), private_symbol).ToLocal(&ret))
-    args.GetReturnValue().Set(ret);
-}
-
-static void SetHiddenValue(const FunctionCallbackInfo<Value>& args) {
-  Environment* env = Environment::GetCurrent(args);
-
-  CHECK(args[0]->IsObject());
-  CHECK(args[1]->IsUint32());
-
-  Local<Object> obj = args[0].As<Object>();
-  uint32_t index = args[1].As<Uint32>()->Value();
-  Local<Private> private_symbol = IndexToPrivateSymbol(env, index);
-  bool ret;
-  if (obj->SetPrivate(env->context(), private_symbol, args[2]).To(&ret))
-    args.GetReturnValue().Set(ret);
-}
-
 static void Sleep(const FunctionCallbackInfo<Value>& args) {
   CHECK(args[0]->IsUint32());
   uint32_t msec = args[0].As<Uint32>()->Value();
@@ -379,8 +340,6 @@ static void ToUSVString(const FunctionCallbackInfo<Value>& args) {
 }
 
 void RegisterExternalReferences(ExternalReferenceRegistry* registry) {
-  registry->Register(GetHiddenValue);
-  registry->Register(SetHiddenValue);
   registry->Register(GetPromiseDetails);
   registry->Register(GetProxyDetails);
   registry->Register(PreviewEntries);
@@ -404,16 +363,22 @@ void Initialize(Local<Object> target,
   Environment* env = Environment::GetCurrent(context);
   Isolate* isolate = env->isolate();
 
-#define V(name, _)                                                            \
-  target->Set(context,                                                        \
-              FIXED_ONE_BYTE_STRING(env->isolate(), #name),                   \
-              Integer::NewFromUnsigned(env->isolate(), index++)).Check();
   {
-    uint32_t index = 0;
+    Local<v8::ObjectTemplate> tmpl = v8::ObjectTemplate::New(isolate);
+#define V(PropertyName, _)                                                     \
+  tmpl->Set(FIXED_ONE_BYTE_STRING(env->isolate(), #PropertyName),              \
+            env->PropertyName());
+
     PER_ISOLATE_PRIVATE_SYMBOL_PROPERTIES(V)
-  }
 #undef V
 
+    target
+        ->Set(context,
+              FIXED_ONE_BYTE_STRING(isolate, "privateSymbols"),
+              tmpl->NewInstance(context).ToLocalChecked())
+        .Check();
+  }
+
 #define V(name)                                                               \
   target->Set(context,                                                        \
               FIXED_ONE_BYTE_STRING(env->isolate(), #name),                   \
@@ -435,8 +400,6 @@ void Initialize(Local<Object> target,
   V(kHasExitCode);
 #undef V
 
-  SetMethodNoSideEffect(context, target, "getHiddenValue", GetHiddenValue);
-  SetMethod(context, target, "setHiddenValue", SetHiddenValue);
   SetMethodNoSideEffect(
       context, target, "getPromiseDetails", GetPromiseDetails);
   SetMethodNoSideEffect(context, target, "getProxyDetails", GetProxyDetails);

diff --git a/test/parallel/test-internal-util-decorate-error-stack.js b/test/parallel/test-internal-util-decorate-error-stack.js
@@ -5,12 +5,14 @@ const fixtures = require('../common/fixtures');
 const assert = require('assert');
 const internalUtil = require('internal/util');
 const { internalBinding } = require('internal/test/binding');
-const binding = internalBinding('util');
+const {
+  privateSymbols: {
+    arrow_message_private_symbol,
+    decorated_private_symbol,
+  }
+} = internalBinding('util');
 const spawnSync = require('child_process').spawnSync;
 
-const kArrowMessagePrivateSymbolIndex = binding.arrow_message_private_symbol;
-const kDecoratedPrivateSymbolIndex = binding.decorated_private_symbol;
-
 const decorateErrorStack = internalUtil.decorateErrorStack;
 
 // Verify that decorateErrorStack does not throw with non-objects.
@@ -73,9 +75,8 @@ const arrowMessage = 'arrow_message';
 err = new Error('foo');
 originalStack = err.stack;
 
-binding.setHiddenValue(err, kArrowMessagePrivateSymbolIndex, arrowMessage);
+err[arrow_message_private_symbol] = arrowMessage;
 decorateErrorStack(err);
 
 assert.strictEqual(err.stack, `${arrowMessage}${originalStack}`);
-assert.strictEqual(
-  binding.getHiddenValue(err, kDecoratedPrivateSymbolIndex), true);
+assert.strictEqual(err[decorated_private_symbol], true);
diff --git a/test/parallel/test-util-internal.js b/test/parallel/test-util-internal.js
@@ -7,30 +7,24 @@ const fixtures = require('../common/fixtures');
 const { internalBinding } = require('internal/test/binding');
 
 const {
-  getHiddenValue,
-  setHiddenValue,
-  arrow_message_private_symbol: kArrowMessagePrivateSymbolIndex
+  privateSymbols: {
+    arrow_message_private_symbol,
+  },
 } = internalBinding('util');
 
-assert.strictEqual(
-  getHiddenValue({}, kArrowMessagePrivateSymbolIndex),
-  undefined);
-
 const obj = {};
-assert.strictEqual(
-  setHiddenValue(obj, kArrowMessagePrivateSymbolIndex, 'bar'),
-  true);
-assert.strictEqual(
-  getHiddenValue(obj, kArrowMessagePrivateSymbolIndex),
-  'bar');
+assert.strictEqual(obj[arrow_message_private_symbol], undefined);
+
+obj[arrow_message_private_symbol] = 'bar';
+assert.strictEqual(obj[arrow_message_private_symbol], 'bar');
+assert.deepStrictEqual(Reflect.ownKeys(obj), []);
 
 let arrowMessage;
 
 try {
   require(fixtures.path('syntax', 'bad_syntax'));
 } catch (err) {
-  arrowMessage =
-      getHiddenValue(err, kArrowMessagePrivateSymbolIndex);
+  arrowMessage = err[arrow_message_private_symbol];
 }
 
 assert.match(arrowMessage, /bad_syntax\.js:1/);