vm: references to context inside objects are not === the original context

[domenic]

Failing test case:

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

var sandbox = {};
sandbox.document = { defaultView: sandbox };

vm.createContext(sandbox);

vm.runInContext('var result = document.defaultView === this', sandbox);

assert.equal(sandbox.result, true);

Investigating this in my local build ... this is the only remaining test suite failure after switching jsdom to io.js vm.

[mscdex]

Shouldn't

vm.runInContext('var result = document.defaultView === this');

be

vm.runInContext('var result = document.defaultView === this', sandbox);

?

[Fishrock123]

@mscdex looks like it. The code throws without that fix. With the fix the assertion still fails.

[bnoordhuis]

Correct me if I'm wrong but isn't that because of the global proxy object? I bet that if you start iojs with --allow_natives_syntax and throw a %IsJSGlobalProxy() in there, it's true for this and false for document.defaultView. Are you saying that it works with contextify?

[domenic]

@mscdex yes, edited OP.

@bnoordhuis interesting. You are right that this doesn't work with contextify. Which at least implies there is a workaround with jsdom---namely, do whatever it was that we did with contextify, which apparently was at least slightly different from what we're doing with vm. And you are also right this is due to the global proxy.

So at least this is not a regression. However, in browser environments the global proxy is purposefully indistinguishable from the global. (That is, you can never get ahold of the global directly). I wonder if we want to enforce something similar for vm code. Or maybe we don't, since this alternative is more powerful?

[domenic]

I still don't fully understand what's going on here. Consider this modified test case:

var assert = require('assert');
var vm = require('vm');

var sandbox = {};
sandbox.document = { defaultView: sandbox };
sandbox.window = sandbox;

vm.createContext(sandbox);

vm.runInContext('var result1 = document.defaultView === this', sandbox);
vm.runInContext('var result2 = window === this', sandbox);

console.log(sandbox.result1); // false
console.log(sandbox.result2); // true

Why does it fail only when stored as a property of another object?

[domenic]

I think it must be because of these lines:

    Local<Object> sandbox = PersistentToLocal(isolate, ctx->sandbox_);
    Local<Value> rv = sandbox->GetRealNamedProperty(property);
    if (rv.IsEmpty()) {
      Local<Object> proxy_global = PersistentToLocal(isolate,
                                                     ctx->proxy_global_);
      rv = proxy_global->GetRealNamedProperty(property);
    }
    if (!rv.IsEmpty() && rv == ctx->sandbox_) {
      rv = PersistentToLocal(isolate, ctx->proxy_global_);
    }

It seems like in the case of window, we run into the rv == ctx->sandbox_ clause, which auto-converts to the proxy-global (this). That's pretty black magic.

[domenic]

I think there are two possible things we could do here:

1. Embrace the global-vs.-globalproxy thing. That is what web browsers do: there is a WindowProxy that is essentially a proxy to the actual global object, a Window. (The trick is that if you have an <iframe> that navigates from page to page, each page gets a new Window, but the same WindowProxy (whose backing Window changes). That is why iframe.contentWindow is the same across navigations.)

   In browsers, only the window proxy is ever observable---this in the global scope returns it, and all getters on Window return WindowProxy instances (e.g. window, self, top, etc. all return the WindowProxy corresponding to the Window they are defined on). The only way to determine the existence of the whole Window/WindowProxy duality is through indirect questions like "when I do x = 5 and create a global variable, then navigate the page, will window.x still be 5? (no)", or alternately by patching Blink to expose the actual Window.

   In V8, WindowProxy is not actually a proper ECMAScript Proxy. Instead it uses a hack called "hidden prototypes", which is kind of like a proxy but probably works a lot worse in the edge case behaviors. This is V8-specific; Firefox, e.g., has a proper proxy.

   With this in mind, we can see that the line above does something kind of strange, but well-intentioned. Namely, it makes it so that any time a property of the global object returns the global object, instead that property returns the global proxy. (Note that, since the global proxy is a proxy, this statement also holds: "any time a property of the global proxy returns the global object, instead that property returns the global proxy.) It is kind of a half-assed attempt at enforcing the same censorship Window does. As such, my test case demonstrates its half-assedness: you can bypass the censorship by just having sandbox.document.defaultView return the global, and since the censorship only applies to properties of sandbox and not to those of sandbox.document, then sandbox.document.defaultView is not censored and returns the actual global instead of the global proxy.

   If we were to go down this route, it would be the responsibility of vm users to understand and manage the difference. We should remove the half-assed censorship, and probably expose a vm.getGlobalProxy(sandbox) for people who want to use it. Then, environments like jsdom that want to emulate the browser's setup would do exactly that---they'd make sure that they only expose to contextified code the global proxy, instead of exposing the global itself. For example, the original post might be expanded and rewritten as

   var sandbox = {};
   vm.createContext(sandbox);
   var globalProxy = vm.getGlobalProxy(sandbox); // polyfill: vm.runInContext('this', sandbox)
   
   sandbox.window = globalProxy;
   sandbox.document = { defaultView: globalProxy };
   
   vm.runInContext('var result = document.defaultView === this', sandbox); // should now be true

   That is, by exercising caution to never expose sandbox directly, jsdom and code like it manages to ensure that only the global proxy is ever visible.

2. Try and extricate ourselves from this crazy browser legacy. Maybe there is a way to bypass all this browser globalproxy stuff. If we can somehow tell V8 to create a context without a global proxy, and to instead just use the sandbox object directly, that'd be sweet. The main impact here, I think, is that this would no longer pass %IsJSGlobalProxy().

[vkurchatkin]

1 looks fine to me. I don't think that getGlobalProxy is actually necessary: polyfill seems pretty obvious. Also people can just do something like:

var sandbox = {};
sandbox.document = { };
vm.createContext(sandbox);
vm.runInContext('document.defaultView === this', sandbox);

to create references to global proxy.

[domenic]

I would really like to investigate removing the global proxy concept if we can, since we shouldn't need it since we're not a browser of multiple interacting iframes. I'll see what I can do.

An alternative, if we stick with 1, is we should probably expose a method to re-target the global proxy at another backing global.

[Fishrock123]

@domenic ... and also this one?

[domenic]

This is almost certainly not fixed in next. It is going to be a big change, but I am excited to work on it.

[Trott]

@domenic Is this something you're still excited to work on? Or would adding a help wanted label be appropriate at this point?

[domenic]

@Trott yes, but also yes :). No time...

[fhinkel]

Since the workaround fixes the === problem, do we want close this? Or should we keep it open for the globalproxy discussion?

/cc @domenic

[domenic]

@fhinkel I think the issue identified here is still real, and the alternatives I gave in https://github.com/nodejs/node/issues/855#issuecomment-74768454 are the still the best paths forward, in my opinion. The === problem (which is a result of the global/global proxy distinction) is still likely to bite people, especially given the half-assed censorship we do in some cases (illustrated by https://github.com/nodejs/node/issues/855#issuecomment-74583712).

[fhinkel]

OK, thanks for clarifying!

[jasnell]

@fhinkel ... any updates on this one?

[Trott]

/ping @nodejs/vm in case anyone on that team has the time and inclination to work on this issue.

[devsnek]

i can noodle around with this and my current pr count drops a little bit i'll see if i can do something actionable

[devsnek]

so far my idea basically revolves around separating sandbox and global object

> const g = process.binding('contextify').makeContext({ a: 1 }, { name: 'owo' })
undefined
> g.global = g
{ a: 1, global: [Circular] }
> vm.runInContext('this.global === this', g)
true

> const g = vm.createContext({})
undefined
> g.global = g
{ global: [Circular] }
> vm.runInContext('this.global === this', g)
true
>

the proxy still exists and interacts with the sandbox but the return value (g) would be the thing you actually want to play with. in any case except where equality matters it wouldn't matter whether you changed properties on the sandbox or the actual global object.

the change is basically

const globalObj = vm.createContext(sandbox); // hey users there is a difference between sandbox and globalObj now

we can also remove the proxy around the sandbox since it becomes rather useless at this point, only being used for constructing the context global

[BridgeAR]

@devsnek did you ever find the time to further play around with this?