This repository has been archived by the owner. It is now read-only.

createContext/runInXXContext : "this" does not refer to the correct object #1674

Closed
Ayms opened this Issue Sep 9, 2011 · 14 comments

Comments

Projects
None yet
3 participants

Ayms commented Sep 9, 2011

While turning an object into a context and then running code into that context, "this" in prototype functions defined for the initial object does not refer to the context (apparently it does refer to an {} object which we don't know what it is exactly)

Please see the tests below :

var vm = require('vm');

var f=function() {
this.coucou="coucou";
};

f.prototype={
dum : 1,
hello : function() {
console.log('hello');
console.log(this.coucou);
}
}

g=new f(); //global

g.hello(); //hello + coucou //OK

g=vm.createContext(g);

g.g=g; //assign in g context global var g referring to it

g.hello(); //hello + coucou //OK

g.coucou2='coucou2';

g.console=console;

vm.createScript('hello()', '').runInContext(g); //hello + undefined - "this" does not refer to g //NOK

vm.runInContext('hello()',g); //hello + undefined - "this" does not refer to g //NOK

vm.createScript('hello()', '').runInNewContext(g); //hello + undefined - "this" does not refer to g //NOK

vm.createScript('g.hello()', '').runInThisContext(); //hello + coucou //OK

vm.createScript('g.hello()', '').runInContext(g); //hello + undefined - "this" does not refer to g //NOK

vm.createScript('var ess=function() {console.log(this.coucou2)};ess()', '').runInContext(g); //coucou2 //OK

I’m not sure if it will solve all of your issues, but you should read my related issue #1801.

@ry’s code copies properties to (before) and from (after) the vm’s globals from the context object you pass; it is not “live” in any way. This might affect what you’re experiencing with the this value.

In other words, the only thing that matters to the context object is the properties defined on it at the time it is passed to the vm module’s functions.

(I’m planning to fix this bug myself, but it’s going to take me some time to get around to it, unfortunately.)

Ayms commented Oct 12, 2011

Thanks, not sure it is exactly the same issue as #1801 but this can look related (I believe in your tests cases that if you do o.o=o instead of o.oo=o the result should be correct but maybe I misunderstand the point).

There is an easy and not nice workaround for the tests above :

var f=function() {
this.coucou="coucou";
var self=this;
this.hello=function() {console.log(self.coucou)}
};

Then until the issue is fixed we can not use prototype...

I don't know the script code but apparently if the copied property is a function, we should bind it to the context itself (and not the sandbox which maybe happens here), same thing for getters and setters

And maybe, while turning 'f' into a context, we should do automatically f.f=f

Ayms commented Oct 12, 2011

correction : or bind it to the sandbox and do f.f=f, I don't know how this works exactly with the sandbox

As documented in #1801, I believe I’ve closed this with elliottcable@cf21650. I’d appreciate if you’d clone and compile against that, and see if you can still reproduce this issue. (Many of these issues seem related.)

Ayms commented Oct 17, 2011

Unless we did not clone correctly, unfortunately the result is worse now, please see below. I have seen the comments on #1801, maybe I can help if I get more clarifications about what's happening exactly while creating a context and what you did change (not sure to get everything in #1801 exchanges)

var vm = require('vm');

var f=function() {
this.coucou="coucou";
};

f.prototype={
dum : 1,
hello : function() {
console.log(this.coucou);
}
}

g=new f(); //global

console.log("before context");

g.hello(); //coucou //OK

g=vm.createContext(g);

g.g=g; //assign in g context global var g referring to it

console.log("after context");

g.hello(); //Crash : # has no method 'hello' //NOK

g.console=console;

console.log("try vm");

vm.createScript('hello()', '').runInContext(g); //hello is not defined //NOK

Okay, first off, the return value of createContext() is not the same object. You have to understand, a v8::Context is not the same thing as a global-object. It’s a special object whose only purpose, from your point of view, is to be passed to runInContext().

Secondly, the same is true of the “sandbox object” (what you are calling g, which is not actually the global.) When you pass g as the sandbox for this v8::Context, and then create a script, you’re setting your g object as the prototype for the global-object within that v8::Context.

Those said, here is your code, rewritten to work as I think you want it to:

var vm = require('vm');

var F=function() {
  this.coucou = "coucou";
}

F.prototype.hello = function() {
  console.log(this.coucou);
}

g = new F(); // sandbox

console.log("before context");
g.hello(); //coucou //OK

ctx=vm.createContext(g);
g.g=g; // assign in sandbox self-reference back to sandbox

console.log("after context");
g.hello(); //coucou //OK

g.console=console;

console.log("try vm");
vm.createScript('hello()').runInContext(ctx); //coucou //OK

Ayms commented Oct 18, 2011

I know it is not the same object, what I do : create g object, overwritting it with the context created from g object, assigning g global var to this context referring to the context, work in the initial context with g and work in vm with g.

Let's take a concrete example, we have set a node-dom project that we will put online today, this creates the DOM from a web page. What we do :

var window=new WINDOW(); //creates window object from WINDOW prototype

window=vm.createContext(window); //window becomes a context, let's call it 'ctx' not to get confused
//pb : "this" in properties functions/getters/setters of ctx from WINDOW prototype do not refer to ctx

window.window=window; //define window global var in 'ctx' referring to 'ctx'

vm.createScript('do some stuff in ctx context;var hello="hello";console.log(window.hello) //hello//OK',window)

//do some stuff in initial context

console.log(window.hello); //hello//OK

window.$=jQuery;

//do some stuff in ctx context

vm.createScript('do some stuff in ctx context;console.log($) //jQuery object//OK; console.log(window.$) //jQuery object//OK; load YUI framework',window)

//

//back in the initial context

console.log(window.Y); //YUI object//OK

So, we understand that we have to do js stuff both in the initial context and ctx context, and this stuff must be reflected in window in both contexts (DOM creation/manipulation occurs in both contexts, initial creation in initial context, modifications by scripts insertion from web pages in ctx context)

Why not to do in c++ something like (example given in js):

ctx=vm.createContext(window) --> while copying properties of window, if function/getter/setter ctx.property=function() {var self=this;return function() {return window.property.apply(self,arguments)}} or window.property.bind(ctx) if this works, this is an example for functions, it has to be adapted for setters and getters (getOwnPropertyDescriptor and then defineProperty with functions bound to ctx)

And then there are no reasons why window=vm.createContext(window) does not work, which is the case now.

Ayms commented Oct 18, 2011

So I looked in the node_script.cc code, especially the cloneObjectMethod, which is using js, let me see how I would modify this.

I am not sure what you mean by "special object" but if I take global for example, it behaves as my window example above (and does global.global=global)

To be honest, I’m having a really hard time understanding your commentary. Could you, at the very least, utilize code-formatting? If you don’t know how to do so, you should read Gruber’s documentation, and further GitHub’s modifications. Of especial interest being the syntax-hilighted triple-backtick codeblocks, which can be used as follows:

```javascript
// JavaScript code in here
```

(I know it sounds nitpicky, but I really am trying to understand you, here. I’m not being intentionally facetious.)

Ayms commented Oct 19, 2011

OK, please see below, this is how it is working before your modifications and how it should be working after (with correction of "this" issue) :

var window=new WINDOW(); //creates window object from WINDOW prototype

window=vm.createContext(window); 

//window becomes a context, let's call it 'ctx' not to get confused
//pb : "this" in properties functions/getters/setters of ctx from WINDOW prototype do not refer to ctx

window.window=window; //define window global var in 'ctx' referring to 'ctx'

vm.runInContext('
    do some stuff in ctx context; \
    var hello="hello"; \
    console.log(window.hello) //hello//OK'
   ,window)

//do some stuff in initial context

console.log(window.hello); //hello//OK

window.$=jQuery;

//do some stuff in ctx context

vm.runInContext('
    do some stuff in ctx context; \
    console.log($) //jQuery object//OK; \
    console.log(window.$) //jQuery object//OK; \
    load YUI framework'
    ,window)

//back in the initial context

console.log(window.Y); //YUI object//OK

@nais okay. This brings me back to my original point: this line of code is inherently broken:

window = vm.createContext(window)

Why, exactly, are you overwriting your object with a reference to the context? You shouldn’t be doing this. Put as simply as I can:

You should not be using the return value of createContext, hereafter called “the ctx,” for any other purpose than passing it to one of the following functions:

  • Script.runInContext()
  • vm.runInContext()

Instead of setting values on the context, set them on the sandbox itself (in your example, this window object, whatever that is; the original one, the result of new WINDOW(). Don’t pay attention to or in any way use/modify the ctx object.


All of that said, I’m curious what you expected the ctx object to be used for? What were your assumptions about its purpose/use/API? Maybe I can use you as a ‘guinea pig,’ to help me make the entire VM API less confusing. Is there some sort of task you need to complete that you can’t do with the sandbox object, that you were instead attempting to do with the ctx?

Ayms commented Oct 20, 2011

See #1801 answer, if sandbox and ctx are updated together and I can give them the same name, there is no pb. Then sandbox properties = ctx properties = window properties. But it is not the case after your changes.

So let's try to make another example, based on https://github.com/Nais/node-dom :

//I am in the initial context//

I do load a web page

node-dom is building the DOM from this page, it creates the window object and subsequent objects

window then should be the context of the DOM as it is usually in browsers

all DOM objects are then defined in initial context

then node-dom can encounter scripts, it loads it and execute it in VM in window context

then it creates/modifies DOM objects in VM context (window)

so the link between objects created in initial context and VM must be window, and to be more precise it is window.document in fact (where document is defined from window prototype in initial context and then in VM)

the script can load jQuery for example (so VM sets $ var, and in VM we have also window.$ defined)

since I am building the DOM from the initial context, I might want to do something with it once it is built

then in initial context I might want to do something like :

    document=window.document;
    document.onload=function() {use jQuery; use window.$}

it will execute in the initial context, then window.$ that was set in VM must exist in initial context

or more generally I might want to do :

    document.onload=function() {
        var script=document.createElement('script');
        script.src=xxx;
        script.onload=one function that will execute in initial context after script is evaluated in VM context
        document.body.appendChild(script)
        etc..
    }

Then you realize same kind of things are executed in initial context and in VM, if you have another solution to handle this with a ctx not equal to window, please let me know.

Another thing that can be disturbing is that functions will execute in the context where it has been defined, example :

I do load this web page :

<html>
  <head></head>
    <body onload="console.log(this.nodeName)">
     <script language="Javascript">
        var script=...
        script.onload=function() {console.log(this.parentNode.nodeName;}
      </script>
    </body>
</html>

node-dom will detect onload on body and might want to do : execute Function() {console.log(this.nodeName)}

then this will execute in initial context, to be correct node-dom must rebuild the function in VM and execute it there

same thing as #1770 in fact

while for the script insertion the onload function will be created in VM and executed there

I can send you a test file that checks all this, if all tests are ok then it means that the context implementation is correct, if not there is a problem

@nais I messaged, let’s talk about this in IRC and get everything clear, so we stop wasting space on Issues until we understand eachother.

Owner

indutny commented Mar 18, 2014

No feedback for 2 years

@indutny indutny closed this Mar 18, 2014

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.