Function redefinition in vm.runInContext

[kyriosli]

Possible bug of iojs or perhaps v8?

    var vm = require('vm'), ctx = vm.createContext({});

    vm.runInContext('function test(){return 0}', ctx);
    vm.runInContext('function test(){return 1}', ctx);

    var result = vm.runInContext('test()', ctx);
    // returns 0 in io.js, but 1 in node.js

Tested under win7 x64 and linux.

[Fishrock123]

Confirmed result difference, 0 in iojs and 1 in node (all current versions (0.10.33 & 0.11.15)

[kyriosli]

It seems that a Context is a proxy object in node.js, but a plain object in io.js

[chrisdickinson]

What version of node? Likely this is due to the switch to contextify internally.

[domenic]

This does seem bad, as the intention here is similar to

<script>
function test() { return 0; }
</script>

<script>
function test() { return 1; }
</script>

<script>
alert(test());
</script>

(which alerts 1).

I wonder if it is due to 3c5ea41

[Fishrock123]

@domenic not directly / not alone, that commit exists in 0.11.15, which also appears to have the bug.

[domenic]

@Fishrock123 right, I am wondering what the behavior was before that commit but after the contextify switch.

[piscisaureus]

Possibly related: nodejs/node-v0.x-archive#9084

[targos]

Still returning 0 on v2.1.0.
Could be interesting to test on next branch, maybe with #1773 ?

[Fishrock123]

@domenic is this fixed in next?

[domenic]

Haven't been able to test this with the new patches, but I didn't see any reason why the new patches would necessarily fix it. I still have a list of vm bugs to work on :)

[brendanashworth]

Behavior still occurs in 3.0, marking as a bug!

[Trott]

Still returns 0 in v4.2.0.

[Trott]

Not sure if this is helpful or stating the obvious, but as @domenic seems to have suspected, 3c5ea41 is the first commit that exhibits the bug. The immediately previous commit does not exhibit the bug.

[targos]

I think I understand the problem here (but I don't know how to fix it):

• 3c5ea41 added a CopyProperties method to make sure that properties added on global with Object.defineProperty calls are properly put in the context object. This includes function declarations.
• To check if property is missing in the context, it uses hasOwnProperty
• If a property is missing, it is added to the context. => This is the source of the issue. Now that the property is an own property of the context, next calls to CopyProperties won't change it!

You don't need to redefine a function to see the problem. Just put a property with the same name in the context before running the code:

> var vm = require('vm'), ctx = vm.createContext({});
> vm.runInContext('function a(){return 0}', ctx);
> ctx
{ a: [Function: a] }

> var vm = require('vm'), ctx = vm.createContext({a: 1});
> vm.runInContext('function a(){return 0}', ctx);
> ctx
{ a: 1 }

[targos]

Wait, I have an idea for a fix

[targos]

So I can't make it work, probably because I don't really understand V8's API.

Here is the patch I tried:

Index: src/node_contextify.cc
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- src/node_contextify.cc  (revision 6f14b3a7db93d3d2e2c20b3489dd7ddb7171ee97)
+++ src/node_contextify.cc  (revision )
@@ -138,8 +138,8 @@
     int length = names->Length();
     for (int i = 0; i < length; i++) {
       Local<String> key = names->Get(i)->ToString(env()->isolate());
-      bool has = sandbox->HasOwnProperty(key);
-      if (!has) {
+      bool isEqual = sandbox->Get(key)->SameValue(global->Get(key));
+      if (!isEqual) {
         // Could also do this like so:
         //
         // PropertyAttribute att = global->GetPropertyAttributes(key_v);

For some reason, as soon as the property exists on both sides, isEqual is always true.

cc @bnoordhuis ?

[domenic]

I have not confirmed but my guess as to what's wrong with your patch is that by using ->Get, you are triggering the interceptors which contextify installs. You might want to try replacing one or both of those with GetRealNamedPropertyInPrototypeChain or GetRealNamedProperty.

However, I think the ultimate fix here (for this and other issues) might be to try to copy the Blink architecture a bit better. See https://groups.google.com/forum/#!topic/v8-users/OwcOrXYOpUE which was originally about #2734 but might have broader correctness implications.

[idoby]

The issue seems to be that GlobalPropertySetterCallback isn't even called when defining a function with the form "function a() {}", but everything works as expected when running "a = function () {}".

Test case:

'use strict';
var common = require('../common');
var assert = require('assert');
var vm = require('vm');

var ctx = vm.createContext({a: 1});
// Uncomment the following line to make test pass
//vm.runInContext('a = function () {}', ctx);
vm.runInContext('function a() {}', ctx);

assert.equal(typeof ctx.a, 'function');

[amasad]

Running into this. Curious how does the node repl work around this issue?

[fhinkel]

@amasad We preprocess the input in repl and convert function declarations (function x() {...}) to function definitions (var x = function() {...}), see bb9eabe. That's why it works in repl.

[fhinkel]

https://bugs.chromium.org/p/v8/issues/detail?id=5375

[fhinkel]

Fixed upstream in V8. I guess we keep this issue open until current V8 version lands in Node. At that point we can also revert #7624.

[jasnell]

@fhinkel would it be possible to float the patch on older v8's?

[fhinkel]

There were some other changes right where the fix was. It's no problem to fix it in older versions - but merging might be painful. Let me check if the patch applies cleanly on current master.

[fhinkel]

The patch doesn't apply on cleanly on master. I'd prefer waiting until we update V8 rather than floating the patch.

[jasnell]

Sounds good to me! Thanks @fhinkel !