Repository navigation
vm: references to context inside objects are not === the original context #855
Description
Activity
Shouldn't
vm.runInContext('var result = document.defaultView === this');
be
vm.runInContext('var result = document.defaultView === this', sandbox);
?
@mscdex looks like it. The code throws without that fix. With the fix the assertion still fails.
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_syntaxand throw a%IsJSGlobalProxy()in there, it's true forthisand false fordocument.defaultView. Are you saying that it works with contextify?@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?
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?
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 therv == ctx->sandbox_clause, which auto-converts to the proxy-global (this). That's pretty black magic.- added 4 commits that reference this issue
on Feb 16, 2015 I think there are two possible things we could do here:
-
Embrace the global-vs.-globalproxy thing. That is what web browsers do: there is a
WindowProxythat is essentially a proxy to the actual global object, aWindow. (The trick is that if you have an<iframe>that navigates from page to page, each page gets a newWindow, but the sameWindowProxy(whose backingWindowchanges). That is whyiframe.contentWindowis the same across navigations.)In browsers, only the window proxy is ever observable---
thisin the global scope returns it, and all getters onWindowreturnWindowProxyinstances (e.g.window,self,top, etc. all return theWindowProxycorresponding to theWindowthey are defined on). The only way to determine the existence of the whole Window/WindowProxy duality is through indirect questions like "when I dox = 5and create a global variable, then navigate the page, willwindow.xstill 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.defaultViewreturn the global, and since the censorship only applies to properties ofsandboxand not to those ofsandbox.document, thensandbox.document.defaultViewis 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
vmusers to understand and manage the difference. We should remove the half-assed censorship, and probably expose avm.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 asvar 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
sandboxdirectly, jsdom and code like it manages to ensure that only the global proxy is ever visible. -
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
thiswould no longer pass%IsJSGlobalProxy().
-
1 looks fine to me. I don't think that
getGlobalProxyis 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.
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.
- added a commit that references this issue
on Feb 20, 2015 30 remaining items
This issue has been marked as stale due to 210 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.Still an issue
Failing test case:
Investigating this in my local build ... this is the only remaining test suite failure after switching jsdom to io.js vm.