Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

vm misses const definitions after recursive loads (only in v0.11.15) #9084

Closed
paleolitico opened this issue Jan 23, 2015 · 11 comments
Closed

Comments

@paleolitico
Copy link

We have a system configured with scripts in a DSL that is loaded with vm module.
This DSL have a "include" action so that a script can load other scripts.
We use "const" to declare constants in the script.
With Node.js upto 0.11.14 everything behaves as expected.
But with 0.11.15, something strange happens with const declarations:
consts before the first include behave as usual,
but consts after the first include disappear and we got an undefined.

We have simplified the DSL to the following program:

//------------------------------------------------------------
// load-conf.js
var vm = require('vm');
var fs = require('fs');

function include(script) {
vm.runInContext(fs.readFileSync(script), ctxt, script);
}

var sandbox = {
include: include,
console: console
};

var ctxt = vm.createContext(sandbox);

for (var i = 2; i < process.argv.length; i++) include(process.argv[i]);

The following configuration scripts can be used to test the problem.

//------------------------------------------------------------
// const-root.conf
console.info("const-root.conf");

function f0(x) {return x;}
const x0 = 10;
var y0 = 20;
console.info("Root before include (0):", f0, x0, y0);

include("const-included.conf");

function f2(x) {return x;}
const x2 = 10;
var y2 = 20;
console.info("Root after include (0):", f0, x0, y0);
console.info("Root after include (2):", f2, x2, y2);
//------------------------------------------------------------
// const-included.conf
console.info("const-included.conf");

function f1(x) {return x;}
const x1 = 10;
var y1 = 20;
console.info("Included (1):", f1, x1, y1);

When executed with node-v0.10.35 or node-v0.11.14, we get the following output:

$ node-v0.11.14/node load-conf.js const-root.conf
const-root.conf
Root before include (0): function f0(x) {return x;} 10 20
const-included.conf
Included (1): function f1(x) {return x;} 10 20
Root after include (0): function f0(x) {return x;} 10 20
Root after include (2): function f2(x) {return x;} 10 20

But when executed with node-0.11.15, we get:

$ node-v0.11.15/node load-conf.js const-root.conf
const-root.conf
Root before include (0): function f0(x) {return x;} 10 20
const-included.conf
Included (1): function f1(x) {return x;} 10 20
Root after include (0): function f0(x) {return x;} 10 20
Root after include (2): function f2(x) {return x;} undefined 20

Note the undefined in the last line of the output!

@misterdjules
Copy link

@tjfontaine @cjihrig @chrisdickinson @trevnorris @jasnell Do you have some time to look into it to determine if this needs to be added to the 0.11.16 milestone?

@trevnorris trevnorris added the vm label Jan 26, 2015
@trevnorris
Copy link

@misterdjules Do we know the commit that caused this? If we can get a quick fix for it let's go ahead and throw it on the 0.11.16 milestone.

@misterdjules
Copy link

@trevnorris I've been busy with other issues/PRs so I haven't tried to investigate at all.

@trevnorris trevnorris self-assigned this Jan 26, 2015
@misterdjules
Copy link

@trevnorris The regression was introduced by the most recent upgrade to V8 (and subsequent commits that fix it). It also happens in io.js, so it's either due to the way node.js/io.js uses the V8's API, or it's still a bug in V8.

I'll try to think of ways to narrow down the issue.

@trevnorris
Copy link

@misterdjules excellent work. Thanks for tracking that down.

@trevnorris
Copy link

@domenic Pulling you in since this also affects io.js. Any thoughts on why this is happening?

@trevnorris
Copy link

This worked in V8 up to 3.28.22. After that this test fails.

@jasnell
Copy link
Member

jasnell commented Jun 29, 2015

@trevnorris ... any further thoughts on this one?

@trevnorris
Copy link

@jasnell Sorry. I only did some bisecting and briefly looked at it. Nothing deep enough to give a proper assessment.

@domenic
Copy link

domenic commented Jun 30, 2015

This is on my to-do list to look in to, but pretty far down there. The fact that it broke in V8 is encouraging, and hopefully will lead to a fix similar to that discovered in nodejs/node#1774.

@domenic
Copy link

domenic commented Jul 26, 2015

Candidate commit: https://code.google.com/p/v8/source/detail?r=22385 based on @trevnorris's 3.28.22 -> 3.28.23 range.

@trevnorris trevnorris removed their assignment Feb 18, 2016
@Trott Trott closed this as completed Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants