src: use NativeModuleLoader to compile per_context.js #24660

joyeecheung · 2018-11-26T14:37:49Z

This patch introduces a NativeModuleLoader::CompileAndCall that
can run a JS script under lib/ as a function called
with a null receiver and arguments specified from the C++ layer.
Since all our bootstrappers are wrapped in functions in the
source to avoid leaking variables into the global scope anyway,
this allows us to remove that extra indentation in the JS source code.

As a start we move the compilation and execution of per_context.js
to NativeModuleLoader::CompileAndCall(). This patch also changes the return
value of NativeModuleLoader::LookupAndCompile() to a MaybeLocal
since the caller has to take care of the result being empty
anyway.

This patch reverts the previous design of having the
NativeModuleLoader::Compile() method magically know about the
parameters of the function - until we have tooling
in-place to guess the parameter names in the source with some
annotation, it's more readable to allow the caller to specify
the parameters along with the arguments values.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

This patch introduces a NativeModuleLoader::CompileAndCall that can run a JS script under `lib/` as a function called with a null receiver and arguments specified from the C++ layer. Since all our bootstrappers are wrapped in functions in the source to avoid leaking variables into the global scope anyway, this allows us to remove that extra indentation in the JS source code. As a start we move the compilation and execution of per_context.js to NativeModuleLoader::CompileAndCall(). This patch also changes the return value of NativeModuleLoader::LookupAndCompile() to a MaybeLocal since the caller has to take care of the result being empty anyway. This patch reverts the previous design of having the NativeModuleLoader::Compile() method magically know about the parameters of the function - until we have tooling in-place to guess the parameter names in the source with some annotation, it's more readable to allow the caller to specify the parameters along with the arguments values.

nodejs-github-bot · 2018-11-26T14:37:51Z

@joyeecheung sadly an error occured when I tried to trigger a build :(

joyeecheung · 2018-11-26T14:40:45Z

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

cc @devsnek @addaleax

devsnek · 2018-11-26T16:01:26Z

lib/internal/per_context.js

@@ -1,44 +1,44 @@
+// arguments: global


the important part here wasn't the global, it was creating the closure to avoid variables leaking, since this is a script. can you make a note of that here or in node.cc so we don't accidentally break things in the future?

@devsnek I thought about that, but how does one break this from the JS side? This source is now wrapped inside a function provided by CompileFunctionInContext, so any declaration in this file goes into the scope of that function. Since we are in strict mode, one cannot accidentally declare a global, and as you can see here, the global object is...global. Also, additional properties should be caught by leakedGlobals in the test/common unless someone deliberately add them via Object.defineProperty with additional attributes..

@joyeecheung i'm not saying the current code doesn't work, just that there should be a note mentioning that this code expects a closure of some sort. if it were changed in the future, we could end up leaking the variables like AtomicsNotify to all evaluations in node.js

> vm.runInThisContext('const a = 5') undefined > vm.runInThisContext('a') 5 >

@devsnek: I see...but I think that can be said about all the built in JS, this just puts the per_context.js in the same situation as other native modules, maybe we should find a better place to document about them all...(I have a WIP that migrates the other two bootstrapper, once we are done they are all in the same situation)

Maybe in node_native_module.h? Or even a README in lib?

bnoordhuis · 2018-11-27T14:45:21Z

src/node_native_module.cc

    // In the case of early errors, v8 is already capable of
    // decorating the stack for us - note that we use CompileFunctionInContext
    // so there is no need to worry about wrappers.
-    return scope.Escape(Local<Value>());
+    return scope.EscapeMaybe(MaybeLocal<Value>());


Just return MaybeLocal<Value>(); should work, no need to go through EscapeMaybe() first.

bnoordhuis · 2018-11-27T14:45:53Z

src/node_native_module.cc

  }
+
+  return scope.EscapeMaybe(MaybeLocal<Value>(ret));


Just return scope.Escape(ret);?

bnoordhuis · 2018-11-27T14:46:58Z

src/node_native_module.h

+  // We need to decide whether to do that even when workers are not used.
+  static v8::MaybeLocal<v8::Value> CompileAsModule(Environment* env,
+                                                   const char* id,
+                                                   bool return_code_cache);


I realize you didn't introduce it but the bool would be clearer at call sites as an enum.

Good idea (In fact, it was me who introduced this bool, just not in this patch)

joyeecheung · 2018-11-27T15:25:52Z

@devsnek @bnoordhuis Thanks for the reviews, updated.

I added a few more comments in node_native_module.h, not sure if that's enough to warn people that the safety of closure is provided from the C++ layer, but I guess we could naturally adapt since that's what we already assume about requireable native modules anyway. This just makes the same guarantee for bootstrappers by using the same mechanism to compile them.

joyeecheung · 2018-11-28T13:25:09Z

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

This patch introduces a NativeModuleLoader::CompileAndCall that can run a JS script under `lib/` as a function called with a null receiver and arguments specified from the C++ layer. Since all our bootstrappers are wrapped in functions in the source to avoid leaking variables into the global scope anyway, this allows us to remove that extra indentation in the JS source code. As a start we move the compilation and execution of per_context.js to NativeModuleLoader::CompileAndCall(). This patch also changes the return value of NativeModuleLoader::LookupAndCompile() to a MaybeLocal since the caller has to take care of the result being empty anyway. This patch reverts the previous design of having the NativeModuleLoader::Compile() method magically know about the parameters of the function - until we have tooling in-place to guess the parameter names in the source with some annotation, it's more readable to allow the caller to specify the parameters along with the arguments values. PR-URL: nodejs#24660 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>

Trott · 2018-11-28T19:06:37Z

Landed in 8041380.

Thank you for using --fixup. 😄

This patch introduces a NativeModuleLoader::CompileAndCall that can run a JS script under `lib/` as a function called with a null receiver and arguments specified from the C++ layer. Since all our bootstrappers are wrapped in functions in the source to avoid leaking variables into the global scope anyway, this allows us to remove that extra indentation in the JS source code. As a start we move the compilation and execution of per_context.js to NativeModuleLoader::CompileAndCall(). This patch also changes the return value of NativeModuleLoader::LookupAndCompile() to a MaybeLocal since the caller has to take care of the result being empty anyway. This patch reverts the previous design of having the NativeModuleLoader::Compile() method magically know about the parameters of the function - until we have tooling in-place to guess the parameter names in the source with some annotation, it's more readable to allow the caller to specify the parameters along with the arguments values. PR-URL: #24660 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>

This patch introduces a NativeModuleLoader::CompileAndCall that can run a JS script under `lib/` as a function called with a null receiver and arguments specified from the C++ layer. Since all our bootstrappers are wrapped in functions in the source to avoid leaking variables into the global scope anyway, this allows us to remove that extra indentation in the JS source code. As a start we move the compilation and execution of per_context.js to NativeModuleLoader::CompileAndCall(). This patch also changes the return value of NativeModuleLoader::LookupAndCompile() to a MaybeLocal since the caller has to take care of the result being empty anyway. This patch reverts the previous design of having the NativeModuleLoader::Compile() method magically know about the parameters of the function - until we have tooling in-place to guess the parameter names in the source with some annotation, it's more readable to allow the caller to specify the parameters along with the arguments values. PR-URL: nodejs#24660 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>

BethGriggs · 2019-02-12T10:48:29Z

@joyeecheung this change does not land cleanly on v10.x-staging. I've added the backport-requested-v10.x label, but feel free to swap to dont-land-on- if this change shouldn't land on v10.x.

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 26, 2018

refack approved these changes Nov 26, 2018

View reviewed changes

refack added the module Issues and PRs related to the module subsystem. label Nov 26, 2018

devsnek approved these changes Nov 26, 2018

View reviewed changes

bnoordhuis reviewed Nov 27, 2018

View reviewed changes

fixup! src: use NativeModuleLoader to compile per_context.js

9151e7a

joyeecheung force-pushed the per-context-native branch from 6cc42f8 to 9151e7a Compare November 27, 2018 15:23

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

Trott closed this Nov 28, 2018

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

BethGriggs added the backport-requested-v10.x label Feb 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: use NativeModuleLoader to compile per_context.js #24660

src: use NativeModuleLoader to compile per_context.js #24660

joyeecheung commented Nov 26, 2018

nodejs-github-bot commented Nov 26, 2018

joyeecheung commented Nov 26, 2018

devsnek Nov 26, 2018

joyeecheung Nov 26, 2018 •

edited

Loading

devsnek Nov 26, 2018

joyeecheung Nov 27, 2018

joyeecheung Nov 27, 2018

bnoordhuis Nov 27, 2018

bnoordhuis Nov 27, 2018

bnoordhuis Nov 27, 2018

joyeecheung Nov 27, 2018

joyeecheung commented Nov 27, 2018

joyeecheung commented Nov 28, 2018

Trott commented Nov 28, 2018

BethGriggs commented Feb 12, 2019

src: use NativeModuleLoader to compile per_context.js #24660

src: use NativeModuleLoader to compile per_context.js #24660

Conversation

joyeecheung commented Nov 26, 2018

Checklist

nodejs-github-bot commented Nov 26, 2018

joyeecheung commented Nov 26, 2018

Choose a reason for hiding this comment

joyeecheung Nov 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyeecheung commented Nov 27, 2018

joyeecheung commented Nov 28, 2018

Trott commented Nov 28, 2018

BethGriggs commented Feb 12, 2019

joyeecheung Nov 26, 2018 •

edited

Loading