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

RunInThisContext is not aware of getters and setters #540

Closed
shimondoodkin opened this Issue Dec 31, 2010 · 13 comments

Comments

Projects
None yet
5 participants

in node 0.3.1 source at
node::Script::EvalMachine template
./src/node_script.cc:217
./src/node_script.cc:270

example of getter setters aware code:
var mixin = function(target) {
var i = 1, length = arguments.length, source;
for ( ; i < length; i++ ) {
// Only deal with defined values
if ( (source = arguments[i]) !== undefined ) {
Object.getOwnPropertyNames(source).forEach(function(k){
var d = Object.getOwnPropertyDescriptor(source, k) || {value:source[k]};
if (d.get) {
target.defineGetter(k, d.get);
if (d.set) target.defineSetter(k, d.set);
}
else if (target !== d.value) {
target[k] = d.value;
}
});
}
}
return target;
};

Member

bnoordhuis commented Jul 4, 2011

@shimondoodkin I'm not exactly sure what the issue is. Can you post a ready-to-run test case?

var assert=require('assert');

function node_script_cc_RunInThisContext_getters_setters_awareness()
{
//test1
var consolelogger={log:function(t){
//console.log(t);
this.mylog.push(t)
},mylog:[]};

function CtxObj(val){}
CtxObj.prototype = {
    nonsetter:"some value",
    get value(){
        consolelogger.log("getter used");
        return this._value;
    },
    set value(val){
        consolelogger.log("setter used");
        this._value = val;
    }
};

var cx=new CtxObj();
cx.value="123412"; // setter used
var x=cx.value;    // getter used
var vm= require('vm');
cx.consolelogger=consolelogger;
vm.runInNewContext("consolelogger.log('will use a setter'); value=1; /*setter used*/ consolelogger.log('just used a setter');", cx, __filename+"_vm.runInNewContext_2")

//console.log(consolelogger.mylog);

assert.deepEqual(consolelogger.mylog,
[ 'setter used',
  'getter used',
  'will use a setter',
  'setter used' ,
  'just used a setter',
], "test1 RunInThisContext is not aware of getters and setters. defined setters outside are not useable inside.")



//test2
function CtxObj2(val){}
CtxObj2.prototype = {
    xcounter:0,
    settercounter:0,
    get value(){
        this.xcounter++;
        return this.xcounter;
    },
    set value(val){
     this.settercounter++;
    }
};

var cx2=new CtxObj2();
var x=cx2.value;    // getter used // x=1
vm.runInNewContext("x=value;/*getter used x=2*/ x2=value;/*getter used x=3*/ value=0;/*setter used counter=1*/value=0;/*setter used counter=2*/ value=0;/*setter used counter=3*/", cx2, __filename+"_vm.runInNewContext_2")

assert.equal(cx2.x,            3, "test2a RunInThisContext is not aware of getters and setters. getters are not usable inside");
assert.equal(cx2.settercounter,3, "test2b RunInThisContext is not aware of getters and setters. setters are not usable inside");

// test3
var cx3={};
vm.runInNewContext("this.__defineGetter__('zee', function() { return this.z; });this.__defineSetter__('zee', function(zz) { this.z=zz; });", cx3, __filename+"_vm.runInNewContext")

var zee_setter=cx3.__lookupSetter__('zee');
assert.equal(typeof zee_setter,'function', "test3a RunInThisContext is not aware of getters and setters. defined setters inside are not useable outside. inside setter is not a function ");

var zee_getter=cx3.__lookupGetter__('zee');
assert.equal(typeof zee_getter,'function', "test3b RunInThisContext is not aware of getters and setters. defined getters inside are not useable outside. inside getter is not a function ");




// test4
var consolelogger4={log:function(t){
console.log(t);
this.mylog.push(t)
},mylog:[]};

var cx4={consolelogger4:consolelogger4,
    get value(){
        consolelogger4.log("getter used");
        return this._value;
    },
    set value(val){
        consolelogger4.log("setter used");
        this._value = val;
    }
};
vm.runInNewContext("this.__defineGetter__('zee', function() { consolelogger4.log('inside getter used'); return this.z; });this.__defineSetter__('zee', function(zz) { this.z=zz;consolelogger4.log('inside setter used'); });", cx4, __filename+"_vm.runInNewContext")
console.log(consolelogger4.mylog);
assert.equal(consolelogger4.mylog.length,0, "test4 RunInThisContext is not aware of getters and setters. getters and setters should not be called spontainiusly without user want ");

}

node_script_cc_RunInThisContext_getters_setters_awareness()

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

isaacs commented Oct 19, 2011

Closed by 200df86

@isaacs isaacs closed this Oct 19, 2011

tested this issue with joyent-node-b0f78af v0.5.10-pre,

good work @ELLIOTTCABLE it fixed several important issues

it still not passes few tests

the getters and setteres are not passed to the vm executed code
also when the execution ends they are exactly as they ware just before the execution.

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

function node_script_cc_RunInThisContext_getters_setters_awareness2()
{

//test2
function CtxObj2(val){}
CtxObj2.prototype = {
    console:console,
    xcounter:0,
    settercounter:0,
    get value(){
        this.xcounter++;
        return this.xcounter;
    },
    set value(val){
     this.settercounter++;
    }
};

var cx2=new CtxObj2();
var xx=cx2.value;    // getter used // xx=1
vm.runInNewContext("x=value;console.log('inside x is ',x, 'but should be 2');/*getter used x=2*/ x2=value;/*getter used x=3*/ value=0;/*setter used counter=1*/value=0;/*setter used counter=2*/ value=0;/*setter used counter=3*/", cx2, __filename+"_vm.runInNewContext_2")
assert.equal(cx2.x,            3, "test2a getters are not usable inside");
assert.equal(cx2.settercounter,3, "test2b setters are not usable inside");

// test3
var cx3={};
vm.runInNewContext("this.__defineGetter__('zee', function() { return this.z; });this.__defineSetter__('zee', function(zz) { this.z=zz; });", cx3, __filename+"_vm.runInNewContext")

var zee_setter=cx3.__lookupSetter__('zee');
assert.equal(typeof zee_setter,'function', "test3a defined setters inside are not useable outside. inside setter is not a function ");

var zee_getter=cx3.__lookupGetter__('zee');
assert.equal(typeof zee_getter,'function', "test3b defined getters inside are not useable outside. inside getter is not a function ");

}

node_script_cc_RunInThisContext_getters_setters_awareness2()

@isaacs isaacs reopened this Oct 19, 2011

Hm, what on earth is going on there? This seems very odd.

@shimondoodkin, could you reduce the testcase into several independant testcases, preferably that sequentially increase in levels of complexity (with the first one being the absolute simplest test that doesn’t pass)?

P.S. (Basic code-clarity stuff: Try and utilise full, descriptive names for the variables, and encapsulate unrelated functionality into separate tests; that would make it quite a bit easier to figure out what you’re trying to demonstrate.)

@shimondoodkin, I’ve refactored your testcases to my ability to understand them, and committed them (failing) to a branch in my own fork. Could you take a look at the regression file I committed, and verify that it exposes each of the functionalities that you experience as ‘broken?’

This will allow me to work on either A) “fixing” them as you expect them to operate, or B) clearing up the confusions being caused for you in the API as of ~0.7, with the rest of the VM API changes.

@ELLIOTTCABLE seems like your test is testing exactly what mine does (test 2, test3).

the first test was testing the situation when the getteres ware executed while passing them from outside to inside and while passed to inside they ware passed as values instead of as functions.
the 4th test was testing the other way around if the getteres and setteres ware defined inside , do they passed as functions to the outside and not as values.

p.s.
@ELLIOTTCABLE :D you right, about the coding style it was a dirty quick solution. next time i'll will do better

@shimondoodkin shimondoodkin reopened this Oct 20, 2011

@shimondoodkin the latter is definitely a known-issue. It’s something we “can’t fix” without changing the API, so we’re looking at solutions to implement and expose as of ~0.7-unstable. The former … is odd. I’m going to look into it when I have time.

@ELLIOTTCABLE i do not understand what you mean, maybe i haven't explained myself well. here:

i wrote you the the previous message because of that the test i gave you in the second time,the test your refactored - it was a cut down version of my first test script containing only test 2 and test 3 - only what was not working, without test 1 and test 4.

i just described the missing tests from your new test file. in case you want to write more covering tests, because you wrote a very good test and i thought may be you had wanted to enhance your new test file even more to prevent accidentally broken things later.

p.s.
i dont know the inner workings but i don't see any requirement to change the external api, if not in 0.6 maybe will be fixed in the later versions...

@shimondoodkin Yes; I know, I was speaking of the tests themselves.

Regarding the tests, your “test three” (what I called ‘the latter’ in my above reply) is a known issue. We won’t be able to “fix” your test 3 (make you able to do what you want to do in that test) without changing the API; that is something we’ll probably aim to fix in ~0.7.

Your “test two” demonstrates a breakage I was unaware of, and will investigate some time in the coming week.

@ELLIOTTCABLE , @shimondoodkin , could you confirm me if this is still an issue in the current versions of stable Node ?

Seems that there is no more interest in this issue, closing it :)

@diasdavid diasdavid closed this Jan 1, 2014

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