lib: replace eval with vm.runInThisContext

[MylesBorins]

Refs: #18212 (review)

[MylesBorins]

CI: https://ci.nodejs.org/job/node-test-pull-request/13014/

[TimothyGu]

Can we enable strict mode, since we are no longer using eval()?

[MylesBorins]

updated @TimothyGu

[bnoordhuis]

V8's profiler scripts weren't strict mode compatible at one point. You probably want to double-check that turning it on doesn't break anything.

edit: but they're executed as a separate script so it's probably fine.

[MylesBorins]

@bnoordhuis or @nodejs/benchmarking how would I go about testing this?

[addaleax]

@MylesBorins In theory python tools/test.py tick-processor should do the trick, but on master the tests seem to time out for me? You might want to try this on a release branch instead (or track down what makes things time out, if you experience that too)…

[MylesBorins]

@addaleax turtles! Will report back

[MylesBorins]

Floated this on the 8.10.0 proposal branch...

So this breaks the world 🎉

ReferenceError: require is not defined
    at evalmachine.<anonymous>:32:12
    at evalmachine.<anonymous>:3859:3
    at ContextifyScript.Script.runInThisContext (vm.js:50:33)
    at Object.runInThisContext (vm.js:139:38)
    at internal/v8_prof_processor.js:41:4
    at NativeModule.compile (bootstrap_node.js:597:7)
    at Function.NativeModule.require (bootstrap_node.js:542:18)
    at startup (bootstrap_node.js:134:20)
    at bootstrap_node.js:609:3
evalmachine.<anonymous>:32
const fs = require('fs');

/cc @nodejs/v8

edit: python tools/test.py tick-processor works before this patch is applied on v8.10.0-proposal, is broken on master though

[addaleax]

@MylesBorins I guess that’s because eval would capture the require from outside and make it accessible to the evaluated script

You can probably get around that by making the evaluated script a normal function instead of an IIFE, then compile that using vm.runInThisContext and then pass the pseudo-globals it needs (in particular require) to that function, similar to what we do with the normal module wrappers.

That being said: It’s not really obvious to me why we don’t just use require() to load the scripts we want in the first place?

[MylesBorins]

@addaleax removing the IIFE doesn't solve anything. Using require instead also didn't work. I tried banging my head against the wall to figure out why the behavior is different, but it doesn't seem like I can get the tests to pass if eval is switched out.

@bnoordhuis is this change totally neccessary? Could we instead check the V8 flag and bail with a clear warning if eval is disabled?

[addaleax]

@MylesBorins

diff --git a/lib/internal/v8_prof_processor.js b/lib/internal/v8_prof_processor.js
index deb8d77d0acb..bb71213a8f4d 100644
--- a/lib/internal/v8_prof_processor.js
+++ b/lib/internal/v8_prof_processor.js
@@ -33,9 +33,9 @@ if (process.platform === 'darwin') {
   tickArguments.push('--windows');
 }
 tickArguments.push.apply(tickArguments, process.argv.slice(1));
-script = `(function() {
+script = `(function(require) {
   arguments = ${JSON.stringify(tickArguments)};
   function write (s) { process.stdout.write(s) }
   ${script}
-})()`;
-vm.runInThisContext(script);
+})`;
+vm.runInThisContext(script)(require);

works fine for me? (edit: on v8.x-staging)

[MylesBorins]

@addaleax OHHHHHH. Got it working on my machine. I hadn't considered using runInThisContext to produce a function that I injected require into. Should we potentially include all lexically scoped variables?

specifically exports, require, module, __filename, __dirname

CI: https://ci.nodejs.org/job/node-test-pull-request/13112/

[MylesBorins]

CI is being weird, one more time https://ci.nodejs.org/job/node-test-pull-request/13117/

[MylesBorins]

Here's a build off master to compare to see if these failures are from this PR https://ci.nodejs.org/job/node-test-commit/16182/

[MylesBorins]

@TimothyGu it looks like I regressed moving between machines 🤦🏽‍♂️

one more ci: https://ci.nodejs.org/job/node-test-pull-request/13152/

[MylesBorins]

YACI (yet another CI): https://ci.nodejs.org/job/node-test-pull-request/13180/

[jasnell]

CI failures appear to be unrelated, but trying again on arm due to a total build bot failure: https://ci.nodejs.org/job/node-test-commit-arm-fanned/14534/

[BridgeAR]

It came out green.

Landed in 99d693d 🎉