vm: pass correct context to CompileFunctionInContext in CompileFunction #23206

darahayes · 2018-10-01T19:10:01Z

The ContextifyContext::CompileFunction function was accidentally passing the wrong context variable into ScriptCompiler::CompileFunctionInContext this resulted in the incorrect context being passed to the compiled function.

You can quickly test this with the following code:

const vm = require('vm')

const name = 'world'
const parsingContext = vm.createContext({ name: 'world' })

const code = `return 'hello ' + name`

const fn = vm.compileFunction(code, [], { parsingContext })

console.log(fn())

With Node v10.10.0 I get the following output:

:1
return 'hello ' + name
                  ^

ReferenceError: name is not defined
    at <anonymous>:1:19
    at Object.<anonymous> (/Users/dara/mydev/vmtest/index.js:10:13)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
    at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
    at Function.Module._load (internal/modules/cjs/loader.js:530:3)
    at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
    at startup (internal/bootstrap/node.js:279:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:696:3)

With the proposed changes, the output is hello world.

I also edited the description of the parsingContext option in the docs to explicitly use the word contextified. The docs use this word everywhere else when referring to a context object.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

ryzokuken

Looks great! Could you please add a test?

darahayes · 2018-10-01T19:33:30Z

@ryzokuken I'm also wondering. In these lines:

https://github.com/nodejs/node/pull/23206/files#diff-0cf206672499c2f86db4ffb0cc5b668bR1041

and

https://github.com/nodejs/node/pull/23206/files#diff-0cf206672499c2f86db4ffb0cc5b668bR1052

should we also be checking against parsing_context instead of context? I'm not actually 100% sure what this check is doing but it looks like we're checking the wrong object.

Sorry for the slowness here, I've never actually written c++ before and I'm not exactly familiar with the v8 APIs. I am trying though 😄

darahayes · 2018-10-01T19:34:46Z

@ryzokuken I'd be happy to help write a test. Is it good enough to test this from the JS layer or should the test be written in C++? Sorry for all the questions!

devsnek · 2018-10-01T19:50:29Z

@darahayes

In these lines ... should we also be checking against parsing_context instead of context?

Nope. Those get calls should happen in whatever the current context is.

darahayes · 2018-10-01T19:54:18Z

@devsnek thanks. Now I think understand what's happening in both of those loops. We're basically checking (by name) to see if that particular value exists in the current context, and if so, then we can add it right?

Working on some small tests now!

darahayes · 2018-10-01T20:09:13Z

@ryzokuken just added some small test cases. Hope they're okay :)

ryzokuken · 2018-10-01T20:34:03Z

@darahayes

Is it good enough to test this from the JS layer or should the test be written in C++?

JS tests should be fine. Thanks.

ryzokuken

The tests look great, can you confirm that they all pass with the changed code?

darahayes · 2018-10-01T21:30:16Z

@ryzokuken using ./configure && make -j4 test shows everything passes.

Additionally, I was able to run the individual test file I modified with python tools/test.py -J --mode=release parallel/test-vm-basic.

Surely this would be running in the Travis build too right?

mscdex · 2018-10-01T21:48:32Z

CI: https://ci.nodejs.org/job/node-test-pull-request/17567/

ryzokuken · 2018-10-02T17:51:27Z

@darahayes It would, yes! The CI failures are probably not related to your change, and it will be fixed soon.

darahayes · 2018-10-08T11:53:59Z

pinging @ryzokuken, just wondering is there any more work to be done on my side to get this merged? Thanks!

lundibundi · 2018-10-08T12:05:43Z

@darahayes while we are at it could you rebase on master to make CI easier?

darahayes · 2018-10-08T12:08:24Z

@lundibundi just rebased!

lundibundi · 2018-10-08T12:09:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/17678/

ryzokuken · 2018-10-09T13:31:09Z

CI is green! If nobody lands with in the next few hours, I will.

ryzokuken

Final LG.

ryzokuken · 2018-10-09T13:50:15Z

@darahayes shucks, the other two commits aren't fixups. Would you mind squashing the three commits into one?

ContextifyContext::CompileFunction in src/node_contextify.cc was incorrectly passing the context variable to ScriptCompiler::CompileFunctionInContext This meant that the parsingContext option in vm.compileFunction was not being applied properly to the compiled function. fixes: nodejs#23194 doc: clarify parsingContext option for vm.compileScript test: usage of parsingContext in vm.compileFunction

darahayes · 2018-10-09T15:46:50Z

@ryzokuken looks like the build failed for some unrelated reason. Some timeout. Is it possible to rerun it and see what happens?

ryzokuken · 2018-10-09T18:09:13Z

@darahayes timeout in not ok 519 parallel/test-worker-memory indeed. Weird that it'd happen only on Windows. Could you check if it's a known flake?

ryzokuken · 2018-10-09T18:09:52Z

@darahayes here we go: #23277

ryzokuken · 2018-10-09T18:10:05Z

/cc @Trott

TimothyGu

Now that parsingContext is also the context/realm in which the function is created, the name becomes a misnomer. For v8.Script, the parsingContext option only controls behavior seen through the Inspector API, as the created script is not bound to a V8 context. I suggest we change the name to context or something similar, if we can still do so.

Trott · 2018-10-10T01:14:38Z

/cc @Trott

Hi! Are you looking for a review from me? Or is there something else you'd like/need?

lundibundi · 2018-10-10T19:14:22Z

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/17728/

Though, @darahayes perhaps you can come up with a better name for the context (refs #23206 (review))

ryzokuken · 2018-10-11T03:59:05Z

@Trott pinged you because of the flaky test, thought it'd bring this particular failure to your notice plus you'll have a better idea about the flake in general 😅.

Trott · 2018-10-11T04:29:36Z

@Trott pinged you because of the flaky test, thought it'd bring this particular failure to your notice plus you'll have a better idea about the flake in general 😅.

Ah! Well, yes, that test has been observed timing out elsewhere and is reported in #23277. The Resume Build passed, so I think you can proceed.

danbev · 2018-10-11T06:51:18Z

Landed in 8da674d.

ContextifyContext::CompileFunction in src/node_contextify.cc was incorrectly passing the context variable to ScriptCompiler::CompileFunctionInContext This meant that the parsingContext option in vm.compileFunction was not being applied properly to the compiled function. fixes: #23194 doc: clarify parsingContext option for vm.compileScript test: usage of parsingContext in vm.compileFunction PR-URL: #23206 Fixes: #23194 Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

darahayes · 2018-10-11T08:38:53Z

Though, @darahayes perhaps you can come up with a better name for the context (refs #23206 (review))

Apologies for the delay in getting back to this. I would have preferred to get my changes right over getting them merged right now.

For v8.Script, the parsingContext option only controls behavior seen through the Inspector API, as the created script is not bound to a V8 context.

You'll have to forgive me, I'm super new to this.

I'm not exactly sure what the Inspector API is.
It might be a silly question, but is the created script not bound to the context that we pass into ScriptCompiler::CompileFunctionInContext? Or is it just compiled in that context, but at runtime, it's not bound to anything?

In any case if you folks think it's more appropriate, I'd be happy to submit a small PR that renames parsingContext to context from the JS side and the native side (and docs of course).

This would rename an option passed in from userland code, so these would be breaking changes right? Is there anything from a process or technical perspective that I need to be aware of when submitting a PR with breaking changes?

danbev · 2018-10-11T15:15:15Z

I would have preferred to get my changes right over getting them merged right now.

Sorry about that. I saw the author ready label and thought is was good to go.

darahayes · 2018-10-11T15:41:31Z

Sorry about that.

It's all good. It's really my fault because I didn't respond quick enough 😄

lundibundi · 2018-10-11T18:51:00Z

IMO it's not like this PR is not 'ready', that part was an addition that could happen here or in a follow-up PR as this one was opened for 'pass correct context to CompileFunctionInContext in CompileFunction'.
Also, I think that change deserves it's own PR as this one was almost ready at that point and it was unlikely that any more people would look through it lessening the possibility of coming up with a better name/other improvements connected.

Edit: also, @darahayes no worries, there is no such thing as be quick enough, there are obviously some sane limits of how much time a change might take but usually a PR is either 'needs changes' and it will wait or 'ready to go'. If the change was critical people would just use 'Request changes' to make sure this doesn't get landed before they are fixed.

darahayes · 2018-10-11T19:06:17Z

Thanks @lundibundi I'm happy to create a new issue around the naming and work on a follow up PR if you think that's the way to go 😊

ContextifyContext::CompileFunction in src/node_contextify.cc was incorrectly passing the context variable to ScriptCompiler::CompileFunctionInContext This meant that the parsingContext option in vm.compileFunction was not being applied properly to the compiled function. fixes: #23194 doc: clarify parsingContext option for vm.compileScript test: usage of parsingContext in vm.compileFunction PR-URL: #23206 Fixes: #23194 Reviewed-By: Ujjwal Sharma <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. vm Issues and PRs related to the vm subsystem. labels Oct 1, 2018

ryzokuken approved these changes Oct 1, 2018

View reviewed changes

darahayes force-pushed the master branch from c084b50 to df7fe04 Compare October 1, 2018 19:37

darahayes force-pushed the master branch from c763e7b to 08d58c8 Compare October 1, 2018 20:11

darahayes changed the title ~~Pass correct context to CompileFunctionInContext in vm.compileFunction~~ vm: pass correct context to CompileFunctionInContext in CompileFunction Oct 1, 2018

ryzokuken approved these changes Oct 1, 2018

View reviewed changes

jasnell approved these changes Oct 2, 2018

View reviewed changes

joyeecheung mentioned this pull request Oct 2, 2018

CI failures: 20180927-20181002 nodejs/reliability#18

Closed

darahayes force-pushed the master branch from 08d58c8 to 9e4569c Compare October 2, 2018 18:58

darahayes force-pushed the master branch from 9e4569c to 07f1d28 Compare October 8, 2018 12:07

thefourtheye approved these changes Oct 8, 2018

View reviewed changes

ryzokuken approved these changes Oct 9, 2018

View reviewed changes

darahayes force-pushed the master branch from 07f1d28 to 3d9d83c Compare October 9, 2018 14:07

TimothyGu reviewed Oct 9, 2018

View reviewed changes

lundibundi added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 10, 2018

danbev closed this Oct 11, 2018

codebytere mentioned this pull request Nov 27, 2018

v10.13.1 proposal #24675

Closed

codebytere mentioned this pull request Nov 29, 2018

v10.14.2 proposal #24727

Merged

SimenB mentioned this pull request Jan 23, 2020

Jest 25 with node env fails on node 10.14.1 jestjs/jest#9453

Closed

purumpum mentioned this pull request Jan 29, 2020

Globals do not resolved when run on Node v. 10 SimenB/jest-environment-jsdom-sixteen#2

Closed

SimenB mentioned this pull request Feb 8, 2020

setting testEnvironment to node causes jest functions to be undefined jestjs/jest#9538

Closed

SimenB mentioned this pull request Jun 8, 2020

About min node version support webpack-contrib/terser-webpack-plugin#248

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vm: pass correct context to CompileFunctionInContext in CompileFunction #23206

vm: pass correct context to CompileFunctionInContext in CompileFunction #23206

darahayes commented Oct 1, 2018 •

edited

Loading

ryzokuken left a comment

darahayes commented Oct 1, 2018 •

edited

Loading

darahayes commented Oct 1, 2018

devsnek commented Oct 1, 2018

darahayes commented Oct 1, 2018

darahayes commented Oct 1, 2018

ryzokuken commented Oct 1, 2018

ryzokuken left a comment

darahayes commented Oct 1, 2018

mscdex commented Oct 1, 2018

ryzokuken commented Oct 2, 2018

darahayes commented Oct 8, 2018

lundibundi commented Oct 8, 2018

darahayes commented Oct 8, 2018

lundibundi commented Oct 8, 2018

ryzokuken commented Oct 9, 2018

ryzokuken left a comment

ryzokuken commented Oct 9, 2018

darahayes commented Oct 9, 2018

ryzokuken commented Oct 9, 2018

ryzokuken commented Oct 9, 2018

ryzokuken commented Oct 9, 2018

TimothyGu left a comment

Trott commented Oct 10, 2018

lundibundi commented Oct 10, 2018 •

edited

Loading

ryzokuken commented Oct 11, 2018

Trott commented Oct 11, 2018

danbev commented Oct 11, 2018

darahayes commented Oct 11, 2018

danbev commented Oct 11, 2018

darahayes commented Oct 11, 2018

lundibundi commented Oct 11, 2018 •

edited

Loading

darahayes commented Oct 11, 2018

vm: pass correct context to CompileFunctionInContext in CompileFunction #23206

vm: pass correct context to CompileFunctionInContext in CompileFunction #23206

Conversation

darahayes commented Oct 1, 2018 • edited Loading

Checklist

ryzokuken left a comment

Choose a reason for hiding this comment

darahayes commented Oct 1, 2018 • edited Loading

darahayes commented Oct 1, 2018

devsnek commented Oct 1, 2018

darahayes commented Oct 1, 2018

darahayes commented Oct 1, 2018

ryzokuken commented Oct 1, 2018

ryzokuken left a comment

Choose a reason for hiding this comment

darahayes commented Oct 1, 2018

mscdex commented Oct 1, 2018

ryzokuken commented Oct 2, 2018

darahayes commented Oct 8, 2018

lundibundi commented Oct 8, 2018

darahayes commented Oct 8, 2018

lundibundi commented Oct 8, 2018

ryzokuken commented Oct 9, 2018

ryzokuken left a comment

Choose a reason for hiding this comment

ryzokuken commented Oct 9, 2018

darahayes commented Oct 9, 2018

ryzokuken commented Oct 9, 2018

ryzokuken commented Oct 9, 2018

ryzokuken commented Oct 9, 2018

TimothyGu left a comment

Choose a reason for hiding this comment

Trott commented Oct 10, 2018

lundibundi commented Oct 10, 2018 • edited Loading

ryzokuken commented Oct 11, 2018

Trott commented Oct 11, 2018

danbev commented Oct 11, 2018

darahayes commented Oct 11, 2018

danbev commented Oct 11, 2018

darahayes commented Oct 11, 2018

lundibundi commented Oct 11, 2018 • edited Loading

darahayes commented Oct 11, 2018

darahayes commented Oct 1, 2018 •

edited

Loading

darahayes commented Oct 1, 2018 •

edited

Loading

lundibundi commented Oct 10, 2018 •

edited

Loading

lundibundi commented Oct 11, 2018 •

edited

Loading