lib: replace `eval` with `vm.runInThisContext` #18623

MylesBorins · 2018-02-07T16:09:08Z

MylesBorins · 2018-02-07T16:10:51Z

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

TimothyGu · 2018-02-07T18:22:51Z

lib/internal/v8_prof_processor.js

@@ -1,4 +1,6 @@
 /* eslint-disable strict */


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

MylesBorins · 2018-02-07T22:47:07Z

updated @TimothyGu

bnoordhuis · 2018-02-08T09:09:49Z

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 · 2018-02-08T17:17:02Z

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

addaleax · 2018-02-08T17:23:02Z

@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 · 2018-02-08T17:26:50Z

@addaleax turtles! Will report back

MylesBorins · 2018-02-09T19:28:34Z

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 · 2018-02-09T20:30:12Z

@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 · 2018-02-10T22:06:24Z

@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 · 2018-02-10T22:29:07Z

@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 · 2018-02-12T05:43:31Z

@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 · 2018-02-12T08:56:47Z

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

MylesBorins · 2018-02-12T16:28:08Z

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/

Refs: nodejs#18212 (review)

MylesBorins · 2018-02-13T22:13:13Z

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

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

MylesBorins · 2018-02-15T03:19:36Z

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

jasnell · 2018-02-16T22:03:17Z

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 · 2018-02-17T13:35:24Z

It came out green.

Landed in 99d693d 🎉

PR-URL: nodejs#18623 Refs: nodejs#18212 (review) Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

PR-URL: #18623 Refs: #18212 (review) Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Make the script not error out immediately because of a missing pseudo-global. (Note that it seems like tests are still broken on `master`.) Fixes: nodejs#19044 Refs: nodejs#18623

Make the script not error out immediately because of a missing pseudo-global. (Note that it seems like tests are still broken on `master`.) PR-URL: #19059 Fixes: #19044 Refs: #18623 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Make the script not error out immediately because of a missing pseudo-global. (Note that it seems like tests are still broken on `master`.) PR-URL: nodejs#19059 Fixes: nodejs#19044 Refs: nodejs#18623 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

PR-URL: nodejs#18623 Refs: nodejs#18212 (review) Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Make the script not error out immediately because of a missing pseudo-global. (Note that it seems like tests are still broken on `master`.) PR-URL: nodejs#19059 Fixes: nodejs#19044 Refs: nodejs#18623 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

PR-URL: #18623 Refs: #18212 (review) Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Make the script not error out immediately because of a missing pseudo-global. (Note that it seems like tests are still broken on `master`.) PR-URL: #19059 Fixes: #19044 Refs: #18623 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

PR-URL: #18623 Refs: #18212 (review) Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Make the script not error out immediately because of a missing pseudo-global. (Note that it seems like tests are still broken on `master`.) PR-URL: #19059 Fixes: #19044 Refs: #18623 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: Minwoo Jung <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

MylesBorins requested a review from bnoordhuis February 7, 2018 16:09

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Feb 7, 2018

MylesBorins mentioned this pull request Feb 7, 2018

deps: cherry-pick 0bcb1d6f from upstream V8 #18212

Closed

bnoordhuis approved these changes Feb 7, 2018

View reviewed changes

MylesBorins added the lib / src Issues and PRs related to general changes in the lib or src directory. label Feb 7, 2018

cjihrig approved these changes Feb 7, 2018

View reviewed changes

TimothyGu approved these changes Feb 7, 2018

View reviewed changes

TimothyGu reviewed Feb 7, 2018

View reviewed changes

MylesBorins force-pushed the remove-eval branch from f8d48a6 to aca1ece Compare February 7, 2018 19:09

addaleax approved these changes Feb 7, 2018

View reviewed changes

targos approved these changes Feb 7, 2018

View reviewed changes

TimothyGu approved these changes Feb 7, 2018

View reviewed changes

jasnell approved these changes Feb 9, 2018

View reviewed changes

MylesBorins force-pushed the remove-eval branch 2 times, most recently from 3e96fb8 to aca1ece Compare February 10, 2018 10:14

MylesBorins force-pushed the remove-eval branch from aca1ece to 267bc78 Compare February 12, 2018 05:41

MylesBorins force-pushed the remove-eval branch from 267bc78 to 2c5fcb7 Compare February 12, 2018 22:16

lib: replace eval with vm.runInThisContext

4dd2749

Refs: nodejs#18212 (review)

MylesBorins force-pushed the remove-eval branch from 2c5fcb7 to 4dd2749 Compare February 13, 2018 22:12

TimothyGu approved these changes Feb 14, 2018

View reviewed changes

BridgeAR approved these changes Feb 16, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 16, 2018

BridgeAR closed this Feb 17, 2018

MylesBorins mentioned this pull request Feb 21, 2018

v9.6.0 proposal #18902

Merged

This was referenced Feb 28, 2018

lib: re-fix v8_prof_processor #19059

Closed

node --prof-process is broken #19044

Closed

addaleax added the land-on-v8.x label Jun 30, 2018

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: replace `eval` with `vm.runInThisContext` #18623

lib: replace `eval` with `vm.runInThisContext` #18623

MylesBorins commented Feb 7, 2018

MylesBorins commented Feb 7, 2018

TimothyGu Feb 7, 2018

MylesBorins commented Feb 7, 2018

bnoordhuis commented Feb 8, 2018 •

edited

Loading

MylesBorins commented Feb 8, 2018

addaleax commented Feb 8, 2018

MylesBorins commented Feb 8, 2018

MylesBorins commented Feb 9, 2018 •

edited

Loading

addaleax commented Feb 9, 2018

MylesBorins commented Feb 10, 2018

addaleax commented Feb 10, 2018 •

edited

Loading

MylesBorins commented Feb 12, 2018

MylesBorins commented Feb 12, 2018

MylesBorins commented Feb 12, 2018

MylesBorins commented Feb 13, 2018

MylesBorins commented Feb 15, 2018

jasnell commented Feb 16, 2018

BridgeAR commented Feb 17, 2018 •

edited

Loading

lib: replace eval with vm.runInThisContext #18623

lib: replace eval with vm.runInThisContext #18623

Conversation

MylesBorins commented Feb 7, 2018

MylesBorins commented Feb 7, 2018

TimothyGu Feb 7, 2018

Choose a reason for hiding this comment

MylesBorins commented Feb 7, 2018

bnoordhuis commented Feb 8, 2018 • edited Loading

MylesBorins commented Feb 8, 2018

addaleax commented Feb 8, 2018

MylesBorins commented Feb 8, 2018

MylesBorins commented Feb 9, 2018 • edited Loading

addaleax commented Feb 9, 2018

MylesBorins commented Feb 10, 2018

addaleax commented Feb 10, 2018 • edited Loading

MylesBorins commented Feb 12, 2018

MylesBorins commented Feb 12, 2018

MylesBorins commented Feb 12, 2018

MylesBorins commented Feb 13, 2018

MylesBorins commented Feb 15, 2018

jasnell commented Feb 16, 2018

BridgeAR commented Feb 17, 2018 • edited Loading

lib: replace `eval` with `vm.runInThisContext` #18623

lib: replace `eval` with `vm.runInThisContext` #18623

bnoordhuis commented Feb 8, 2018 •

edited

Loading

MylesBorins commented Feb 9, 2018 •

edited

Loading

addaleax commented Feb 10, 2018 •

edited

Loading

BridgeAR commented Feb 17, 2018 •

edited

Loading