vm.runInThisContext is bound to the top context

[TimothyGu]

• Version: 8.3.0
• Platform: Linux 4.9.0-3-amd64 SMP Debian 4.9.30-2+deb9u3 (2017-08-06) x86_64 GNU/Linux
• Subsystem: vm

'use strict';

const vm = require('vm');
const context = vm.createContext({
  status: 'inside',
  runInThisContext: vm.runInThisContext
});
global.status = 'outside';
console.log('runInContext only:', vm.runInContext('status', context));
console.log('runInContext + runInThisContext:', vm.runInContext('runInThisContext("status")', context));

prints

runInContext only: inside
runInContext + runInThisContext: outside

suggesting that vm.runInThisContext always runs the code in the top context rather than the current context.

[bnoordhuis]

Lightly tested but this patch should fix it.

EvalMachine() calls v8::UnboundScript::BindToCurrentContext(), which is the context of vm.runInThisContext(), not the most recently entered context.

diff --git a/src/node_contextify.cc b/src/node_contextify.cc
index 792b1c4f80..931e3ee824 100644
--- a/src/node_contextify.cc
+++ b/src/node_contextify.cc
@@ -643,6 +643,10 @@ class ContextifyScript : public BaseObject {
     bool display_errors = maybe_display_errors.ToChecked();
     bool break_on_sigint = maybe_break_on_sigint.ToChecked();
 
+    auto context = args.GetIsolate()->GetEnteredContext();
+    if (context.IsEmpty()) context = env->context();
+    Context::Scope context_scope(context);
+
     // Do the eval within this context
     EvalMachine(env, timeout, display_errors, break_on_sigint, args,
                 &try_catch);

[TimothyGu]

@bnoordhuis That does fix the OP, but not this one:

'use strict';
const vm = require('vm');
const context = vm.createContext({
  status: 'inside',
  runInThisContext: vm.runInThisContext
});
global.status = 'outside';

console.log(vm.runInContext('eval', context)("runInThisContext('status')"));

Is it possible to actually fix this?

[TimothyGu]

I think #14757 (comment) is helpful, even if it does not completely fix the problem. @bnoordhuis are you interested in submitting it as a PR?

[bnoordhuis]

There is good news and bad news. The good news is that the patch fixes not just the original bug report but also the known_issues test for #7788 and a stdio redirection bug in the REPL.

The bad news is that #14757 (comment) is a special case of this, which makes it a regression of the two steps forward, one step back variety.

// Refs: https://github.com/nodejs/node/issues/14757
'use strict';
const common = require('../common');
const assert = require('assert');
const vm = require('vm');

{
  const s = '(function() { return [this, runInThisContext("this")] })';
  const f = vm.runInNewContext(s, vm);
  const [that0, that1] = f();
  assert.strictEqual(that0, that1);
  assert.notEqual(that0, global);
}

I think it could be solved if you knew what context the caller of vm.runInThisContext() belongs to but that's currently pretty much impossible to find out (except for v8::FunctionCallbackInfo<v8::Value>::Callee(), but that's deprecated.)

V8 had an API at one time to retrieve the v8::Function associated with a stack frame (v8::StackFrame::GetFunction(), IIRC.) Perhaps it's time to bring that back.

Alternatively, we could store the script ids of each context and map them back with v8::StackFrame::GetScriptId().

[github-actions]

Closing this because it has stalled. Feel free to reopen if this issue is still relevant, or to ping the collaborator who labelled it stalled if you have any questions.