Skip to content

Commit 4ce3242

Browse files
committed
vm: run runInThisContext() in last entered context
Change `vm.runInThisContext()` to use the last entered context, not the top-level context that `vm.runInThisContext()` belongs to. Note the call chain: 1. `node::ContextifyScript::RunInThisContext()` 2. `node::ContextifyScript::EvalMachine()` 3. `v8::Script::BindToCurrentContext()` The current context in `RunInThisContext()` is that of the top-level context which was then bound to the script to execute. It works when `vm.runInThisContext()` is called from the top-level context (the common case and hence why this bug went unnoticed for so long) but not when called from an inferior context. As a pleasant side effect, this commit fixes a bug in the REPL where writes to `process.stdout` and `process.stderr` still went to the real stdout and stderr instead of being captured redirected. Fixes previously failing test `known_issues/test-repl-require-context` which has been moved to `test/parallel` and renamed to avoid a conflict with an existing test of the same name. Note that this commit changes the behavior of the following snippet: const s = '(function() { return vm.runInThisContext(this) })()'; const f = vm.runInNewContext(s, vm); f(); Before this commit, it returned the |this| of the new context, now it returns the |this| of the enclosing context. Two steps forward, one step back. Fixes: nodejs#7788 Refs: nodejs#14757
1 parent 4f339b5 commit 4ce3242

6 files changed

+34
-3
lines changed

src/node_contextify.cc

+4-1
Original file line numberDiff line numberDiff line change
@@ -652,7 +652,10 @@ class ContextifyScript : public BaseObject {
652652
bool display_errors = maybe_display_errors.ToChecked();
653653
bool break_on_sigint = maybe_break_on_sigint.ToChecked();
654654

655-
// Do the eval within this context
655+
auto context = args.GetIsolate()->GetEnteredContext();
656+
if (context.IsEmpty()) context = env->context();
657+
Context::Scope context_scope(context);
658+
656659
EvalMachine(env, timeout, display_errors, break_on_sigint, args,
657660
&try_catch);
658661
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// Refs: https://github.com/nodejs/node/issues/14757
2+
'use strict';
3+
const common = require('../common');
4+
const assert = require('assert');
5+
const vm = require('vm');
6+
7+
{
8+
const s = '(function() { return [this, runInThisContext("this")] })';
9+
const f = vm.runInNewContext(s, vm);
10+
const [that0, that1] = f();
11+
assert.strictEqual(that0, that1);
12+
assert.notEqual(that0, global);
13+
}

test/parallel/test-repl-require.js

+2-1
Original file line numberDiff line numberDiff line change
@@ -30,5 +30,6 @@ server.listen(options, function() {
3030
process.on('exit', function() {
3131
assert.strictEqual(false, /Cannot find module/.test(answer));
3232
assert.strictEqual(false, /Error/.test(answer));
33-
assert.strictEqual(answer, '\'eye catcher\'\n\'perhaps I work\'\n');
33+
assert(answer.includes('\'eye catcher\'\n\'perhaps I work\'\n'));
34+
assert(answer.includes('node_modules')); // Printed by the loaded modules.
3435
});

test/parallel/test-repl.js

+5-1
Original file line numberDiff line numberDiff line change
@@ -456,7 +456,11 @@ function tcp_test() {
456456
expect: (`12346\n${prompt_tcp}`) },
457457
{ client: client_tcp,
458458
send: `require(${JSON.stringify(moduleFilename)}).number`,
459-
expect: (`42\n${prompt_tcp}`) }
459+
expect: (`load fixtures/b/d.js\n` +
460+
`load package/index.js\n` +
461+
`load fixtures/b/c.js\n` +
462+
`load fixtures/a.js\n` +
463+
`42\n${prompt_tcp}`) }
460464
]);
461465
});
462466

test/parallel/test-vm-run-in-new-context.js

+10
Original file line numberDiff line numberDiff line change
@@ -73,3 +73,13 @@ const fn = vm.runInNewContext('(function() { obj.p = {}; })', { obj: {} });
7373
global.gc();
7474
fn();
7575
// Should not crash
76+
77+
// https://github.com/nodejs/node/issues/14757 - vm.runInNewContext() should
78+
// use the last entered context, not the context that vm.runInNewContext()
79+
// belongs to (which is the top level context.)
80+
{
81+
const [that0, that1] =
82+
vm.runInNewContext('[this, runInThisContext("this")]', vm);
83+
assert.strictEqual(that0, that1);
84+
assert.notEqual(that0, global);
85+
}

0 commit comments

Comments
 (0)