lib,src: use V8 API for collection inspection

Use a new public V8 API for inspecting weak collections and
collection iterators, rather than using V8-internal functions
to achieve this. This currently comes with a slight modification of
the output for inspecting iterators generated by `Set().entries()`.

Fixes: https://github.com/nodejs/node/issues/20409

PR-URL: https://github.com/nodejs/node/pull/20719
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

lib/console.js

@@ -29,7 +29,7 @@ const {
     ERR_INVALID_ARG_VALUE,
   },
 } = require('internal/errors');
-const { previewMapIterator, previewSetIterator } = require('internal/v8');
+const { previewEntries } = process.binding('util');
 const { Buffer: { isBuffer } } = require('buffer');
 const util = require('util');
 const {
@@ -345,7 +345,7 @@ Console.prototype.table = function(tabularData, properties) {
 
   const mapIter = isMapIterator(tabularData);
   if (mapIter)
-    tabularData = previewMapIterator(tabularData);
+    tabularData = previewEntries(tabularData);
 
   if (mapIter || isMap(tabularData)) {
     const keys = [];
@@ -367,7 +367,7 @@ Console.prototype.table = function(tabularData, properties) {
 
   const setIter = isSetIterator(tabularData);
   if (setIter)
-    tabularData = previewSetIterator(tabularData);
+    tabularData = previewEntries(tabularData);
 
   const setlike = setIter || isSet(tabularData);
   if (setlike) {

lib/internal/bootstrap/node.js

@@ -29,7 +29,6 @@
     // Do this good and early, since it handles errors.
     setupProcessFatal();
 
-    setupV8();
     setupProcessICUVersions();
 
     setupGlobalVariables();
@@ -479,22 +478,6 @@
     };
   }
 
-  function setupV8() {
-    // Warm up the map and set iterator preview functions.  V8 compiles
-    // functions lazily (unless --nolazy is set) so we need to do this
-    // before we turn off --allow_natives_syntax again.
-    const v8 = NativeModule.require('internal/v8');
-    v8.previewMapIterator(new Map().entries());
-    v8.previewSetIterator(new Set().entries());
-    v8.previewWeakMap(new WeakMap(), 1);
-    v8.previewWeakSet(new WeakSet(), 1);
-    // Disable --allow_natives_syntax again unless it was explicitly
-    // specified on the command line.
-    const re = /^--allow[-_]natives[-_]syntax$/;
-    if (!process.execArgv.some((s) => re.test(s)))
-      process.binding('v8').setFlagsFromString('--noallow_natives_syntax');
-  }
-
   function setupProcessICUVersions() {
     const icu = process.binding('config').hasIntl ?
       process.binding('icu') : undefined;

lib/internal/v8.js

@@ -1,39 +0,0 @@
-'use strict';
-
-// This file provides access to some of V8's native runtime functions. See
-// https://github.com/v8/v8/wiki/Built-in-functions for further information
-// about their implementation.
-// They have to be loaded before anything else to make sure we deactivate them
-// before executing any other code. Gaining access is achieved by using a
-// specific flag that is used internally in the startup phase.
-
-// Clone the provided Map Iterator.
-function previewMapIterator(it) {
-  return %MapIteratorClone(it);
-}
-
-// Clone the provided Set Iterator.
-function previewSetIterator(it) {
-  return %SetIteratorClone(it);
-}
-
-// Retrieve all WeakMap instance key / value pairs up to `max`. `max` limits the
-// number of key / value pairs returned. Make sure it is a positive number,
-// otherwise V8 aborts. Passing through `0` returns all elements.
-function previewWeakMap(weakMap, max) {
-  return %GetWeakMapEntries(weakMap, max);
-}
-
-// Retrieve all WeakSet instance values up to `max`. `max` limits the
-// number of key / value pairs returned. Make sure it is a positive number,
-// otherwise V8 aborts. Passing through `0` returns all elements.
-function previewWeakSet(weakSet, max) {
-  return %GetWeakSetValues(weakSet, max);
-}
-
-module.exports = {
-  previewMapIterator,
-  previewSetIterator,
-  previewWeakMap,
-  previewWeakSet
-};

lib/util.js

@@ -30,18 +30,12 @@ const {
 const { TextDecoder, TextEncoder } = require('internal/encoding');
 const { isBuffer } = require('buffer').Buffer;
 
-const {
-  previewMapIterator,
-  previewSetIterator,
-  previewWeakMap,
-  previewWeakSet
-} = require('internal/v8');
-
 const {
   getPromiseDetails,
   getProxyDetails,
   kPending,
   kRejected,
+  previewEntries
 } = process.binding('util');
 
 const { internalBinding } = require('internal/bootstrap/loaders');
@@ -912,7 +906,7 @@ function formatMap(ctx, value, recurseTimes, keys) {
 
 function formatWeakSet(ctx, value, recurseTimes, keys) {
   const maxArrayLength = Math.max(ctx.maxArrayLength, 0);
-  const entries = previewWeakSet(value, maxArrayLength + 1);
+  const entries = previewEntries(value).slice(0, maxArrayLength + 1);
   const maxLength = Math.min(maxArrayLength, entries.length);
   let output = new Array(maxLength);
   for (var i = 0; i < maxLength; ++i)
@@ -929,16 +923,14 @@ function formatWeakSet(ctx, value, recurseTimes, keys) {
 
 function formatWeakMap(ctx, value, recurseTimes, keys) {
   const maxArrayLength = Math.max(ctx.maxArrayLength, 0);
-  const entries = previewWeakMap(value, maxArrayLength + 1);
-  // Entries exist as [key1, val1, key2, val2, ...]
-  const remainder = entries.length / 2 > maxArrayLength;
-  const len = entries.length / 2 - (remainder ? 1 : 0);
+  const entries = previewEntries(value).slice(0, maxArrayLength + 1);
+  const remainder = entries.length > maxArrayLength;
+  const len = entries.length - (remainder ? 1 : 0);
   const maxLength = Math.min(maxArrayLength, len);
   let output = new Array(maxLength);
   for (var i = 0; i < len; i++) {
-    const pos = i * 2;
-    output[i] = `${formatValue(ctx, entries[pos], recurseTimes)} => ` +
-      formatValue(ctx, entries[pos + 1], recurseTimes);
+    output[i] = `${formatValue(ctx, entries[i][0], recurseTimes)} => ` +
+      formatValue(ctx, entries[i][1], recurseTimes);
   }
   // Sort all entries to have a halfway reliable output (if more entries than
   // retrieved ones exist, we can not reliably return the same output).
@@ -950,9 +942,9 @@ function formatWeakMap(ctx, value, recurseTimes, keys) {
   return output;
 }
 
-function formatCollectionIterator(preview, ctx, value, recurseTimes, keys) {
+function formatCollectionIterator(ctx, value, recurseTimes, keys) {
   const output = [];
-  for (const entry of preview(value)) {
+  for (const entry of previewEntries(value)) {
     if (ctx.maxArrayLength === output.length) {
       output.push('... more items');
       break;
@@ -966,13 +958,11 @@ function formatCollectionIterator(preview, ctx, value, recurseTimes, keys) {
 }
 
 function formatMapIterator(ctx, value, recurseTimes, keys) {
-  return formatCollectionIterator(previewMapIterator, ctx, value, recurseTimes,
-                                  keys);
+  return formatCollectionIterator(ctx, value, recurseTimes, keys);
 }
 
 function formatSetIterator(ctx, value, recurseTimes, keys) {
-  return formatCollectionIterator(previewSetIterator, ctx, value, recurseTimes,
-                                  keys);
+  return formatCollectionIterator(ctx, value, recurseTimes, keys);
 }
 
 function formatPromise(ctx, value, recurseTimes, keys) {

node.gyp

@@ -146,7 +146,6 @@
       'lib/internal/http2/core.js',
       'lib/internal/http2/compat.js',
       'lib/internal/http2/util.js',
-      'lib/internal/v8.js',
       'lib/internal/v8_prof_polyfill.js',
       'lib/internal/v8_prof_processor.js',
       'lib/internal/validators.js',

src/node.cc

@@ -4355,12 +4355,6 @@ void Init(int* argc,
   }
 #endif
 
-  // Needed for access to V8 intrinsics.  Disabled again during bootstrapping,
-  // see lib/internal/bootstrap/node.js.
-  const char allow_natives_syntax[] = "--allow_natives_syntax";
-  V8::SetFlagsFromString(allow_natives_syntax,
-                         sizeof(allow_natives_syntax) - 1);
-
   // We should set node_is_initialized here instead of in node::Start,
   // otherwise embedders using node::Init to initialize everything will not be
   // able to set it and native modules will not load for them.

src/node_util.cc

@@ -49,6 +49,35 @@ static void GetProxyDetails(const FunctionCallbackInfo<Value>& args) {
   args.GetReturnValue().Set(ret);
 }
 
+static void PreviewEntries(const FunctionCallbackInfo<Value>& args) {
+  if (!args[0]->IsObject())
+    return;
+
+  bool is_key_value;
+  Local<Array> entries;
+  if (!args[0].As<Object>()->PreviewEntries(&is_key_value).ToLocal(&entries))
+    return;
+  if (!is_key_value)
+    return args.GetReturnValue().Set(entries);
+
+  uint32_t length = entries->Length();
+  CHECK_EQ(length % 2, 0);
+
+  Environment* env = Environment::GetCurrent(args);
+  Local<Context> context = env->context();
+
+  Local<Array> pairs = Array::New(env->isolate(), length / 2);
+  for (uint32_t i = 0; i < length / 2; i++) {
+    Local<Array> pair = Array::New(env->isolate(), 2);
+    pair->Set(context, 0, entries->Get(context, i * 2).ToLocalChecked())
+        .FromJust();
+    pair->Set(context, 1, entries->Get(context, i * 2 + 1).ToLocalChecked())
+        .FromJust();
+    pairs->Set(context, i, pair).FromJust();
+  }
+  args.GetReturnValue().Set(pairs);
+}
+
 // Side effect-free stringification that will never throw exceptions.
 static void SafeToString(const FunctionCallbackInfo<Value>& args) {
   auto context = args.GetIsolate()->GetCurrentContext();
@@ -188,6 +217,7 @@ void Initialize(Local<Object> target,
   env->SetMethod(target, "getPromiseDetails", GetPromiseDetails);
   env->SetMethod(target, "getProxyDetails", GetProxyDetails);
   env->SetMethod(target, "safeToString", SafeToString);
+  env->SetMethod(target, "previewEntries", PreviewEntries);
 
   env->SetMethod(target, "startSigintWatchdog", StartSigintWatchdog);
   env->SetMethod(target, "stopSigintWatchdog", StopSigintWatchdog);

test/parallel/test-util-inspect.js

@@ -19,14 +19,13 @@
 // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
-// Flags: --expose_internals
 'use strict';
 const common = require('../common');
 const assert = require('assert');
 const JSStream = process.binding('js_stream').JSStream;
 const util = require('util');
 const vm = require('vm');
-const { previewMapIterator } = require('internal/v8');
+const { previewEntries } = process.binding('util');
 
 assert.strictEqual(util.inspect(1), '1');
 assert.strictEqual(util.inspect(false), 'false');
@@ -448,7 +447,7 @@ assert.strictEqual(util.inspect(-5e-324), '-5e-324');
 {
   const map = new Map();
   map.set(1, 2);
-  const vals = previewMapIterator(map.entries());
+  const vals = previewEntries(map.entries());
   const valsOutput = [];
   for (const o of vals) {
     valsOutput.push(o);
@@ -935,8 +934,7 @@ if (typeof Symbol !== 'undefined') {
   const aSet = new Set([1, 3]);
   assert.strictEqual(util.inspect(aSet.keys()), '[Set Iterator] { 1, 3 }');
   assert.strictEqual(util.inspect(aSet.values()), '[Set Iterator] { 1, 3 }');
-  assert.strictEqual(util.inspect(aSet.entries()),
-                     '[Set Iterator] { [ 1, 1 ], [ 3, 3 ] }');
+  assert.strictEqual(util.inspect(aSet.entries()), '[Set Iterator] { 1, 3 }');
   // Make sure the iterator doesn't get consumed.
   const keys = aSet.keys();
   assert.strictEqual(util.inspect(keys), '[Set Iterator] { 1, 3 }');