repl: refactor to avoid unsafe array iteration

[RaisinTen]

This was the only for-let-of loop in lib so I'm trying to make it more consistent with the other for loops.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[aduh95]

An upside of this change is that this code no longer rely on %ArrayIteratorPrototype%.next, which may have been mutated in user-land.

[RaisinTen]

An upside of this change is that this code no longer rely on %ArrayIteratorPrototype%.next, which may have been mutated in user-land.

@aduh95 can u plz point out where %ArrayIteratorPrototype%.next might get mutated in user-land in this loop?

[aduh95]

can u plz point out where %ArrayIteratorPrototype%.next might get mutated in user-land

We don't control user-land, it might happen anywhere. Doing a for-of loop over an array does call %ArrayIteratorPrototype%.next, that's how JS is specd, and users may mutate %ArrayIteratorPrototype%.next; if you want to reproduce it at home:

const ArrayIteratorPrototype = Object.getPrototypeOf([][Symbol.iterator]());
ArrayIteratorPrototype.next = function next() {
  throw new Error('%ArrayIteratorPrototype%.next mutated from user-land');
};
for(const key of [1,2,3]);

[RaisinTen]

Thanks a tonne for the awesome explanation! 🙏

[BridgeAR]

I personally do not see an actual benefit in rewriting this code. Is there a reason that I miss?
We have quite a lot of of for...of loops?
This seems rather personal style and I would rather keep it as is (also to prevent the churn).

[RaisinTen]

@BridgeAR originally in this PR I was changing the one for let of loop in the entirety of lib into a for (let ... loop because all the other for...of loops used consts. But then, @aduh95 mentioned ArrayPrototypeForEach and I thought about replacing all the for loops in repl.js to see if it improves the performance.

[BridgeAR]

I am not certain what's wrong with using let in a for loop?

[RaisinTen]

It's not wrong. It just felt odd that it was the only one out there.

[RaisinTen]

@aduh95 do u think we should benchmark this now?

[aduh95]

We don't have any benchmark for REPL in our benchmark suite 🤔 I may be wrong, but I think it's because REPL is not a very performance-sensitive part of Node.js.

There is no explicit objection to this PR, but I can see from other's reaction that this change seem to be a bit controversial. Could you add a test that deletes Array.iterator[Symbol.iterator] and %ArrayIteratorPrototype%.next in REPL (in the spirit of https://github.com/nodejs/node/blob/9997aebf65f72246106c3d77f093a24b1aeda642/test/parallel/test-require-delete-array-iterator.js)? I think it would outline what is the benefit of this change.

Currently, trying to delete one or the other in REPL crashes the process:

$ node
Welcome to Node.js v15.4.0.
Type ".help" for more information.
> delete Array.prototype[Symbol.iterator]
node:internal/util/inspect:311
      for (const key of optKeys) {
                        ^

TypeError: optKeys is not iterable
    at inspect (node:internal/util/inspect:311:25)
    at REPLServer.writer (node:repl:206:25)
    at Domain.debugDomainError (node:repl:636:25)
    at Domain.emit (node:events:376:20)
    at Domain.EventEmitter.emit (node:domain:470:12)
    at Domain._errorHandler (node:domain:264:23)
    at Object.<anonymous> (node:domain:167:29)
    at process._fatalException (node:internal/process/execution:162:29)

[aduh95]

This throws if you delete Array.prototype[Symbol.iterator], you should either remove that line, or expect an error like TypeError: [1,2,3] is not iterable.

Suggested change

'for(const x of [3, 2, 1]);'

[RaisinTen]

Actually, it just returns true if I remove the loop. Both the errors are thrown right after the loop is streamed in.

[aduh95]

same, you'll get TypeError: undefined is not a function.

Suggested change

'for(const x of [3, 2, 1]);'

[aduh95]

The test passes on Node.js v15.4.0, that's not expected. It seems it doesn't replicate the same behaviour as the actual REPL…

[RaisinTen]

Perhaps this needs to be tested using a node child_process.

[aduh95]

I pulled your branch on my local machine, and I tried to reproduce the bug on REPL. I had to apply #36428 and #36532, and then the following patch for the crash to go away:

diff --git a/lib/internal/repl/utils.js b/lib/internal/repl/utils.js
index e2bda7665a..7a34eca85b 100644
--- a/lib/internal/repl/utils.js
+++ b/lib/internal/repl/utils.js
@@ -244,7 +244,7 @@ function setupPreview(repl, contextSymbol, bufferSymbol, active) {
       }
 
       // Result and the text that was completed.
-      const [rawCompletions, completeOn] = data;
+      const { 0: rawCompletions, 1: completeOn } = data;
 
       if (!rawCompletions || rawCompletions.length === 0) {
         return;
diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js
index 17ca3f3d2b..572e35148f 100644
--- a/lib/internal/util/inspect.js
+++ b/lib/internal/util/inspect.js
@@ -3,6 +3,7 @@
 const {
   Array,
   ArrayIsArray,
+  ArrayPrototypeForEach,
   BigIntPrototypeValueOf,
   BooleanPrototypeValueOf,
   DatePrototypeGetTime,
@@ -287,8 +288,7 @@ function inspect(value, opts) {
     if (typeof opts === 'boolean') {
       ctx.showHidden = opts;
     } else if (opts) {
-      const optKeys = ObjectKeys(opts);
-      for (const key of optKeys) {
+      ArrayPrototypeForEach(ObjectKeys(opts), (key) => {
         // TODO(BridgeAR): Find a solution what to do about stylize. Either make
         // this function public or add a new API with a similar or better
         // functionality.
@@ -300,7 +300,7 @@ function inspect(value, opts) {
           // This is required to pass through the actual user input.
           ctx.userOptions = opts;
         }
-      }
+      });
     }
   }
   if (ctx.colors) ctx.stylize = stylizeWithColor;
@@ -1189,7 +1189,7 @@ function formatError(err, constructor, tag, ctx, keys) {
     // Highlight userland code and node modules.
     let newStack = stack.slice(0, stackStart);
     const lines = stack.slice(stackStart + 1).split('\n');
-    for (const line of lines) {
+    ArrayPrototypeForEach(lines, (line) => {
       const core = line.match(coreModuleRegExp);
       if (core !== null && NativeModule.exists(core[1])) {
         newStack += `\n${ctx.stylize(line, 'undefined')}`;
@@ -1206,7 +1206,7 @@ function formatError(err, constructor, tag, ctx, keys) {
         }
         newStack += pos === 0 ? line : line.slice(pos);
       }
-    }
+    });
     stack = newStack;
   }
   // The message and the stack have to be indented as well!
diff --git a/lib/repl.js b/lib/repl.js
index b2ab7e1f5b..1b1816c949 100644
--- a/lib/repl.js
+++ b/lib/repl.js
@@ -1265,7 +1265,7 @@ function complete(line, callback) {
       ArrayPrototypePush(completionGroups, _builtinLibs);
     }
   } else if (RegExpPrototypeTest(fsAutoCompleteRE, line)) {
-    [completionGroups, completeOn] = completeFSFunctions(line);
+    ({ 0: completionGroups, 1: completeOn } = completeFSFunctions(line));
   // Handle variable member lookup.
   // We support simple chained expressions like the following (no function
   // calls, etc.). That is for simplicity and also because we *eval* that
@@ -1278,7 +1278,7 @@ function complete(line, callback) {
   //   foo.<|>        # completions for 'foo' with filter ''
   } else if (line.length === 0 ||
              RegExpPrototypeTest(/\w|\.|\$/, line[line.length - 1])) {
-    const [match] = RegExpPrototypeExec(simpleExpressionRE, line) || [''];
+    const { 0: match = '' } = RegExpPrototypeExec(simpleExpressionRE, line);
     if (line.length !== 0 && !match) {
       completionGroupsLoaded();
       return;
@@ -1385,7 +1385,8 @@ function complete(line, callback) {
 
     const completions = [];
     // Unique completions across all groups.
-    const uniqueSet = new SafeSet(['']);
+    const uniqueSet = new SafeSet();
+    uniqueSet.add('');
     // Completion group 0 is the "closest" (least far up the inheritance
     // chain) so we put its completions last: to be closest in the REPL.
     ArrayPrototypeForEach(completionGroups, (group) => {
diff --git a/lib/vm.js b/lib/vm.js
index 3389384508..684fa5cab9 100644
--- a/lib/vm.js
+++ b/lib/vm.js
@@ -26,6 +26,7 @@ const {
   Symbol,
   PromiseReject,
   ReflectApply,
+  SafeArrayIterator,
 } = primordials;
 
 const {
@@ -130,7 +131,7 @@ class Script extends ContextifyScript {
     if (breakOnSigint && process.listenerCount('SIGINT') > 0) {
       return sigintHandlersWrap(super.runInThisContext, this, args);
     }
-    return super.runInThisContext(...args);
+    return super.runInThisContext(...new SafeArrayIterator(args));
   }
 
   runInContext(contextifiedObject, options) {

[RaisinTen]

The current test now causes the child process to crash on the spread operator in lib/internal/errors.js:

node:internal/errors:316
      return fn(...args);
                   ^

TypeError: Found non-callable @@iterator
    at hidden (node:internal/errors:316:14)
    at createHandle (node:net:136:3)
    at new Socket (node:net:313:20)
    at createWritableStdioStream (node:internal/bootstrap/switches/is_main_thread:72:18)
    at process.getStderr [as stderr] (node:internal/bootstrap/switches/is_main_thread:134:12)
    at Socket._destroy (node:net:646:26)
    at _destroy (node:internal/streams/destroy:67:23)
    at Socket.destroy (node:internal/streams/destroy:59:5)
    at endReadableNT (node:internal/streams/readable:1310:16)
    at processTicksAndRejections (node:internal/process/task_queues:80:21)

node:internal/errors:316
      return fn(...args);
             ^

TypeError: fn is not a function
    at hidden (node:internal/errors:316:14)
    at createHandle (node:net:136:3)
    at new Socket (node:net:313:20)
    at createWritableStdioStream (node:internal/bootstrap/switches/is_main_thread:72:18)
    at process.getStderr [as stderr] (node:internal/bootstrap/switches/is_main_thread:134:12)
    at Socket._destroy (node:net:646:26)
    at _destroy (node:internal/streams/destroy:67:23)
    at Socket.destroy (node:internal/streams/destroy:59:5)
    at endReadableNT (node:internal/streams/readable:1310:16)
    at processTicksAndRejections (node:internal/process/task_queues:80:21)

Will add new SafeArrayIterator to it.

[aduh95]

@RaisinTen I'm able to have it working (failing on master, passing on this PR). To tested it, I ran the following commands:

git reset upstream/master --hard
curl -L https://github.com/nodejs/node/pull/36428.patch | git am
curl -L https://github.com/nodejs/node/pull/36532.patch | git am
curl -L https://github.com/nodejs/node/pull/36444.patch | git am
git apply patch.diff

patch.diff is the diff file I've shared in #36444 (comment).

[RaisinTen]

I don't understand why ccc860a failed. Currently the ... in internal/errors.js doesn't use a safe iterator so it should throw an error. The logs say that the process exited with 0.

[aduh95]

I don't understand why ccc860a failed. Currently the ... in internal/errors.js doesn't use a safe iterator so it should throw an error. The logs say that the process exited with 0.

Hum the CI actually shows a failure (exit code is 1), which is expected given SafeArrayIterator hasn't landed on master.

[aduh95]

@aduh95 isn't the exit code 0?

It expects a 0 and gets a 1, hence the AssertionError. And it gets a 1 because some internal function tries to iterate over an array without using SafeArrayIterator.

[RaisinTen]

The check in that commit was:

assert.strictEqual(code, 1);

and it fails, so doesn't that mean that code does not equal 1?

[RaisinTen]

@RaisinTen I'm able to have it working (failing on master, passing on this PR). To tested it, I ran the following commands:

git reset upstream/master --hard
curl -L https://github.com/nodejs/node/pull/36428.patch | git am
curl -L https://github.com/nodejs/node/pull/36532.patch | git am
curl -L https://github.com/nodejs/node/pull/36444.patch | git am
git apply patch.diff

patch.diff is the diff file I've shared in #36444 (comment).

Now that this is producing the expected error, should I run these commands and push the updated branch?

[aduh95]

should I run these commands and push the updated branch?

This PR is blocked on other PRs, I suggest waiting for those PRs to land first, then rebase on top of master and force-push to your branch. If you were to include the changes from the other PRs in this branch, you'd still have to rebase when they land so it wouldn't be very useful.

[aduh95]

@RaisinTen the other PRs have landed, you should be able to rebase and make the tests pass now. However, it might be worth opening a new PR and close this one considering that it has changed its focus quite a bit.

[RaisinTen]

@aduh95 cool, here's the new PR: #36663 :)