Does this affect the result Environment::GetCurrent(v8::Local<v8::Context> context) for the new context? Does that matter?

Yes, it does, and no, it shouldn't. Everything in the binding layer should be using Environment::GetCurrent(const v8::FunctionCallbackInfo<v8::Value>&) now.

Everything in the binding layer should be using Environment::GetCurrent(const v8::FunctionCallbackInfo<v8::Value>&) now.

$ fgrep 'Environment::GetCurrent(context)' src/* 2>&-|wc -l
30

Maybe I’m being naïve, but to me all of these look like exceptions to that rule?

Those are from module initialization functions; those operate on the context they're passed when loaded (which is always a node context.)

Those are from module initialization functions; those operate on the context they're passed when loaded (which is always a node context.)

Oh, right. The MakeCallback occurrences in node.cc are not a problem? Those are at least part of the public API, so it sounds like they could be called while another context is the current context?

(Sorry for the question spam)

Oh, that's a good point. Those functions use object->CreationContext() to look up the environment. I added a regression test to test/addons/make-callback (see diff) and sure enough, it crashes now.

On the flip side, using object->CreationContext() seems wrong because it's not necessarily the same context that the callback belongs to (and indeed the regression test hits the env->context() == env->isolate()->GetCurrentContext() assertion.)

diff --git a/test/addons/make-callback/test.js b/test/addons/make-callback/test.js
index 43ad014..b67d114 100644
--- a/test/addons/make-callback/test.js
+++ b/test/addons/make-callback/test.js
@@ -36,6 +36,10 @@ const recv = {
 assert.strictEqual(42, makeCallback(recv, 'one'));
 assert.strictEqual(42, makeCallback(recv, 'two', 1337));

+// Check that callbacks on a receiver from a different context works.
+const foreignObject = vm.runInNewContext('({ fortytwo() { return 42; } })');
+assert.strictEqual(42, makeCallback(foreignObject, 'fortytwo'));
+
 // Check that the callback is made in the context of the receiver.
 const target = vm.runInNewContext(`
     (function($Object) {

I've been prodding at it and I observe that the AssignToContext() calls cause the weird issue where:

auto context_1 = object->CreationContext();
auto context_2 = Environment::GetCurrent(context_1)->context();
CHECK_EQ(context_1, context_2);  // Boom!

It's not difficult to work around (I'll file a pull request) but it seems broken to me.

#9221 landed. I'm going to land the first commit and drop the second one.

@bnoordhuis should this be backported?

@bnoordhuis should this be backported?

ping @bnoordhuis

@gibfahn This (63c47e7) is safe to land.