src: remove unneeded AssignToContext() call

[bnoordhuis]

It was added in commit 681fe59 ("vm: assign Environment to created
context") from April 2014 to work around a segmentation fault when
accessing process.env from another context but that is not necessary
anymore after commit 7e88a93 ("src: make accessors immune to context
confusion") from March 2015.

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

[addaleax]

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

[bnoordhuis]

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

[addaleax]

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?

[bnoordhuis]

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

[addaleax]

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)

[bnoordhuis]

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) {

[bnoordhuis]

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.

[bnoordhuis]

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

[MylesBorins]

@bnoordhuis should this be backported?

[gibfahn]

@bnoordhuis should this be backported?

ping @bnoordhuis

[addaleax]

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