Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vm.runInContext: eval a strict function #2245

Closed
DmitrySoshnikov opened this issue Jul 25, 2015 · 15 comments
Closed

vm.runInContext: eval a strict function #2245

DmitrySoshnikov opened this issue Jul 25, 2015 · 15 comments
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.

Comments

@DmitrySoshnikov
Copy link

Evaluated strict functions, when ran inside vm.ranInContext do not capture environment.

This works (file test.js):

var code = `
var foo = {m: 1};

function bar() {
  "use strict";
  return foo;
}
`;

eval(code);

bar();

Node session:

> vm.runInNewContext(fs.readFileSync('/Users/dmitrys/test.js', 'utf-8'))
{ m: 1 }
>

This doesn't:

function main() {

  var code = `
    var foo = {m: 1};

    function bar() {
      "use strict";
      return foo;
    }
  `;

  eval(code);

  bar(); // foo is not defined

}

main();

Node session:

> vm.runInNewContext(fs.readFileSync('/Users/dmitrys/test.js', 'utf-8'))
undefined:6
      return foo;
             ^
ReferenceError: foo is not defined
    at bar (eval at main (evalmachine.<anonymous>:13:8), <anonymous>:6:14)
    at main (evalmachine.<anonymous>:15:3)
    at evalmachine.<anonymous>:19:1
    at ContextifyScript.Script.runInNewContext (vm.js:18:15)
    at Object.exports.runInNewContext (vm.js:49:17)
    at repl:1:4
    at REPLServer.defaultEval (repl.js:154:27)
    at bound (domain.js:254:14)
    at REPLServer.runBound [as eval] (domain.js:267:12)
    at REPLServer.<anonymous> (repl.js:308:12)

Also the code works if there is no "use strict" for foo.

@brendanashworth brendanashworth added the vm Issues and PRs related to the vm subsystem. label Jul 25, 2015
@domenic
Copy link
Contributor

domenic commented Jul 26, 2015

Possibly related: #548

This is almost certainly a V8 bug. @trevnorris tracked down #548 to V8s after 3.28.22 failing. I'll go see if there's any obvious culprit...

@vkurchatkin vkurchatkin added the v8 engine Issues and PRs related to the V8 dependency. label Jul 26, 2015
@amasad
Copy link

amasad commented Jul 27, 2015

could be related to: nodejs/node-v0.x-archive#8721?

@DmitrySoshnikov
Copy link
Author

How likely this can be fixed in soon time? Is there any ETA? We're blocked by this issue at the moment, and will have to restructure our tests to not to use eval(...) (we use it to evaluate a translated to JS code, and run it). Seems we don't have any alternative to eval in this case (since Function doesn't capture environment either).

@trevnorris
Copy link
Contributor

@domenic this related to the fact we wrap all our scripts to prevent global variable pollution?

@DmitrySoshnikov
Copy link
Author

@trevnorris, seems strict functions just don't capture the local scope at all (and see only global environment, similarly to the Function(...) functions). Notice that the first example works, since bar is global, and sees the global foo in first case. But once it's wrapped into the main function, the bar doesn't see foo anymore.

@targos
Copy link
Member

targos commented Jul 29, 2015

I does not seem to be confined to the vm module. I get the same error if I directly run node test.js

Edit: and with node --use-strict test.js it changes to ReferenceError: bar is not defined

@DmitrySoshnikov
Copy link
Author

@targos, the --use-strict adds a Program-level strict mode? Then the bar function correctly won't be created in the caller's environment (in the environment of main).

However, we run the non-strict mode, and only the function bar itself is strict. The strictness of a function should not affect (per ES5) the environment which should be captured regardless.

So I'd say it's still the issue of the vm module.

@DmitrySoshnikov
Copy link
Author

OK, seems it's really Node/V8 generic evaluation issue, not limited to the vm module. Updated example and the outputs in the Node 0.8 (works), and Node 0.10 (doesn't work):

The ~/test.js:

function main() {

  var code = [
    "var foo = {m: 1};",
    "",
    "function bar() {",
    "  'use strict';",
    "  console.log(foo);", // foo isn't captured in 0.10
    "}",
  ].join('\n');

  eval(code);

  bar(); // foo is not defined, since node 0.10, works in 0.8

}

main();

Tests:

dmitrys-mbp:~ dmitrys$ nvm use 0.8
Now using node v0.8.28 (npm v1.2.30)
dmitrys-mbp:~ dmitrys$ node ~/test.js
{ m: 1 }
dmitrys-mbp:~ dmitrys$ nvm use 0.10
Now using node v0.10.38 (npm v1.4.28)
dmitrys-mbp:~ dmitrys$ node ~/test.js

undefined:5
  console.log(foo);
              ^
ReferenceError: foo is not defined
    at bar (eval at main (/Users/dmitrys/test.js:12:8), <anonymous>:5:15)
    at main (/Users/dmitrys/test.js:14:3)
    at Object.<anonymous> (/Users/dmitrys/test.js:18:1)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
    at Function.Module._load (module.js:312:12)
    at Function.Module.runMain (module.js:497:10)
    at startup (node.js:119:16)
    at node.js:935:3
dmitrys-mbp:~ dmitrys$

@domenic
Copy link
Contributor

domenic commented Jul 29, 2015

Yeah, if this is related to nodejs/node-v0.x-archive#8721, it is more a Node issue than a V8 issue, as we were unable to reproduce the problem in D8 :-/. (I still believe the problem stems from a V8 bug though, given that it regressed in a V8 upgrade.)

@DmitrySoshnikov
Copy link
Author

Yeah, seems it's the same issue. Would still be great to get some ETA of fixing it.

@brendanashworth brendanashworth added the confirmed-bug Issues with confirmed bugs. label Jul 31, 2015
@ChALkeR ChALkeR removed the vm Issues and PRs related to the vm subsystem. label Sep 17, 2015
@ChALkeR
Copy link
Member

ChALkeR commented Sep 17, 2015

#2245 (comment) works for me on 4.0.0.
#2245 (comment) works for me on 4.0.0 (both parts).

Could we close this?

@targos
Copy link
Member

targos commented Sep 17, 2015

The issue in nodejs/node-v0.x-archive#8721 is also fixed in 4.0.0

@ide
Copy link
Contributor

ide commented Nov 16, 2015

Is there a regression test for this (in Node or V8)? Just seems like a subtle enough bug that it's worth checking for in the test suite.

@ChALkeR
Copy link
Member

ChALkeR commented Nov 17, 2015

@ide Ah, adding a test and closing this would make sense.

@jasnell
Copy link
Member

jasnell commented Apr 2, 2016

A test was added in #5250 and this now appears to be working in master. closing.

@jasnell jasnell closed this as completed Apr 2, 2016
jeresig added a commit to Khan/react-render-server that referenced this issue Mar 1, 2017
See: nodejs/node#2245

Test Plan:
I switched my Node version back to v4 and confirmed that I was able to run `render_component.js` without error.

Auditors: emily, csilvers
ryzokuken added a commit to ryzokuken/node that referenced this issue Mar 7, 2018
Rename the test appropriately alongside mentioning the subsystem
Also, make a few basic changes to make sure the test conforms
to the standard test structure

Refs: nodejs#19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Mar 11, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the test conforms
to the standard test structure.

This renames:
- test-regress-nodejsGH-1531
- test-regress-nodejsGH-2245
- test-regress-nodejsGH-3238
- test-regress-nodejsGH-3542
- test-regress-nodejsGH-3739
- test-regress-nodejsGH-4256

PR-URL: nodejs#19212
Refs: nodejs#19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Mar 17, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the test conforms
to the standard test structure.

This renames:
- test-regress-GH-1531
- test-regress-GH-2245
- test-regress-GH-3238
- test-regress-GH-3542
- test-regress-GH-3739
- test-regress-GH-4256

PR-URL: #19212
Refs: #19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 20, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the test conforms
to the standard test structure.

This renames:
- test-regress-GH-1531
- test-regress-GH-2245
- test-regress-GH-3238
- test-regress-GH-3542
- test-regress-GH-3739
- test-regress-GH-4256

PR-URL: #19212
Refs: #19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the test conforms
to the standard test structure.

This renames:
- test-regress-nodejsGH-1531
- test-regress-nodejsGH-2245
- test-regress-nodejsGH-3238
- test-regress-nodejsGH-3542
- test-regress-nodejsGH-3739
- test-regress-nodejsGH-4256

PR-URL: nodejs#19212
Refs: nodejs#19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
BethGriggs pushed a commit that referenced this issue Oct 2, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the test conforms
to the standard test structure.

This renames:
- test-regress-GH-1531
- test-regress-GH-2245
- test-regress-GH-3238
- test-regress-GH-3542
- test-regress-GH-3739
- test-regress-GH-4256

PR-URL: #19212
Refs: #19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

10 participants