From b9a97c2ff76ed2f9bd5fa03fe92b1bf641d9ae3d Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Mon, 26 Nov 2018 21:50:19 +0800
Subject: [PATCH] src: use NativeModuleLoader to compile per_context.js

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: https://github.com/nodejs/node/pull/24660
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
---
 lib/internal/per_context.js |  64 ++++++++++-----------
 src/node.cc                 |  19 ++++---
 src/node_native_module.cc   | 107 ++++++++++++++++++++----------------
 src/node_native_module.h    |  54 ++++++++++++++----
 4 files changed, 144 insertions(+), 100 deletions(-)

diff --git a/lib/internal/per_context.js b/lib/internal/per_context.js
index ddf874c7eac656..87fc8d247eb933 100644
--- a/lib/internal/per_context.js
+++ b/lib/internal/per_context.js
@@ -1,44 +1,44 @@
+// arguments: global
+
 'use strict';
 
 // node::NewContext calls this script
 
-(function(global) {
-  // https://github.com/nodejs/node/issues/14909
-  if (global.Intl) delete global.Intl.v8BreakIterator;
+// https://github.com/nodejs/node/issues/14909
+if (global.Intl) delete global.Intl.v8BreakIterator;
 
-  // https://github.com/nodejs/node/issues/21219
-  // Adds Atomics.notify and warns on first usage of Atomics.wake
-  // https://github.com/v8/v8/commit/c79206b363 adds Atomics.notify so
-  // now we alias Atomics.wake to notify so that we can remove it
-  // semver major without worrying about V8.
+// https://github.com/nodejs/node/issues/21219
+// Adds Atomics.notify and warns on first usage of Atomics.wake
+// https://github.com/v8/v8/commit/c79206b363 adds Atomics.notify so
+// now we alias Atomics.wake to notify so that we can remove it
+// semver major without worrying about V8.
 
-  const AtomicsNotify = global.Atomics.notify;
-  const ReflectApply = global.Reflect.apply;
+const AtomicsNotify = global.Atomics.notify;
+const ReflectApply = global.Reflect.apply;
 
-  const warning = 'Atomics.wake will be removed in a future version, ' +
-    'use Atomics.notify instead.';
+const warning = 'Atomics.wake will be removed in a future version, ' +
+  'use Atomics.notify instead.';
 
-  let wakeWarned = false;
-  function wake(typedArray, index, count) {
-    if (!wakeWarned) {
-      wakeWarned = true;
+let wakeWarned = false;
+function wake(typedArray, index, count) {
+  if (!wakeWarned) {
+    wakeWarned = true;
 
-      if (global.process !== undefined) {
-        global.process.emitWarning(warning, 'Atomics');
-      } else {
-        global.console.error(`Atomics: ${warning}`);
-      }
+    if (global.process !== undefined) {
+      global.process.emitWarning(warning, 'Atomics');
+    } else {
+      global.console.error(`Atomics: ${warning}`);
     }
-
-    return ReflectApply(AtomicsNotify, this, arguments);
   }
 
-  global.Object.defineProperties(global.Atomics, {
-    wake: {
-      value: wake,
-      writable: true,
-      enumerable: false,
-      configurable: true,
-    },
-  });
-}(this));
+  return ReflectApply(AtomicsNotify, this, arguments);
+}
+
+global.Object.defineProperties(global.Atomics, {
+  wake: {
+    value: wake,
+    writable: true,
+    enumerable: false,
+    configurable: true,
+  },
+});
diff --git a/src/node.cc b/src/node.cc
index 425c1f875dd983..e01baf9726a45e 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -162,7 +162,6 @@ using v8::ObjectTemplate;
 using v8::PropertyAttribute;
 using v8::ReadOnly;
 using v8::Script;
-using v8::ScriptCompiler;
 using v8::ScriptOrigin;
 using v8::SealHandleScope;
 using v8::SideEffectType;
@@ -2500,14 +2499,16 @@ Local<Context> NewContext(Isolate* isolate,
     // Run lib/internal/per_context.js
     Context::Scope context_scope(context);
 
-    // TODO(joyeecheung): use NativeModuleLoader::Compile
-    Local<String> per_context =
-        per_process_loader.GetSource(isolate, "internal/per_context");
-    ScriptCompiler::Source per_context_src(per_context, nullptr);
-    Local<Script> s = ScriptCompiler::Compile(
-        context,
-        &per_context_src).ToLocalChecked();
-    s->Run(context).ToLocalChecked();
+    std::vector<Local<String>> parameters = {
+        FIXED_ONE_BYTE_STRING(isolate, "global")};
+    std::vector<Local<Value>> arguments = {context->Global()};
+    MaybeLocal<Value> result = per_process_loader.CompileAndCall(
+        context, "internal/per_context", &parameters, &arguments, nullptr);
+    if (result.IsEmpty()) {
+      // Execution failed during context creation.
+      // TODO(joyeecheung): deprecate this signature and return a MaybeLocal.
+      return Local<Context>();
+    }
   }
 
   return context;
diff --git a/src/node_native_module.cc b/src/node_native_module.cc
index 11136a244c685f..77763229ce44de 100644
--- a/src/node_native_module.cc
+++ b/src/node_native_module.cc
@@ -103,29 +103,54 @@ void NativeModuleLoader::CompileCodeCache(
 
   // TODO(joyeecheung): allow compiling cache for bootstrapper by
   // switching on id
-  Local<Value> result = CompileAsModule(env, *id, true);
+  MaybeLocal<Value> result =
+      CompileAsModule(env, *id, CompilationResultType::kCodeCache);
   if (!result.IsEmpty()) {
-    args.GetReturnValue().Set(result);
+    args.GetReturnValue().Set(result.ToLocalChecked());
   }
 }
 
 void NativeModuleLoader::CompileFunction(
     const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
-
   CHECK(args[0]->IsString());
   node::Utf8Value id(env->isolate(), args[0].As<String>());
-  Local<Value> result = CompileAsModule(env, *id, false);
+
+  MaybeLocal<Value> result =
+      CompileAsModule(env, *id, CompilationResultType::kFunction);
   if (!result.IsEmpty()) {
-    args.GetReturnValue().Set(result);
+    args.GetReturnValue().Set(result.ToLocalChecked());
   }
 }
 
-Local<Value> NativeModuleLoader::CompileAsModule(Environment* env,
-                                                 const char* id,
-                                                 bool produce_code_cache) {
+// TODO(joyeecheung): it should be possible to generate the argument names
+// from some special comments for the bootstrapper case.
+MaybeLocal<Value> NativeModuleLoader::CompileAndCall(
+    Local<Context> context,
+    const char* id,
+    std::vector<Local<String>>* parameters,
+    std::vector<Local<Value>>* arguments,
+    Environment* optional_env) {
+  Isolate* isolate = context->GetIsolate();
+  MaybeLocal<Value> compiled = per_process_loader.LookupAndCompile(
+      context, id, parameters, CompilationResultType::kFunction, nullptr);
+  if (compiled.IsEmpty()) {
+    return compiled;
+  }
+  Local<Function> fn = compiled.ToLocalChecked().As<Function>();
+  return fn->Call(
+      context, v8::Null(isolate), arguments->size(), arguments->data());
+}
+
+MaybeLocal<Value> NativeModuleLoader::CompileAsModule(
+    Environment* env, const char* id, CompilationResultType result) {
+  std::vector<Local<String>> parameters = {env->exports_string(),
+                                           env->require_string(),
+                                           env->module_string(),
+                                           env->process_string(),
+                                           env->internal_binding_string()};
   return per_process_loader.LookupAndCompile(
-      env->context(), id, produce_code_cache, env);
+      env->context(), id, &parameters, result, env);
 }
 
 // Currently V8 only checks that the length of the source code is the
@@ -183,15 +208,18 @@ ScriptCompiler::CachedData* NativeModuleLoader::GetCachedData(
   return new ScriptCompiler::CachedData(code_cache_value, code_cache_length);
 }
 
-// Returns Local<Function> of the compiled module if produce_code_cache
+// Returns Local<Function> of the compiled module if return_code_cache
 // is false (we are only compiling the function).
 // Otherwise return a Local<Object> containing the cache.
-Local<Value> NativeModuleLoader::LookupAndCompile(Local<Context> context,
-                                                  const char* id,
-                                                  bool produce_code_cache,
-                                                  Environment* optional_env) {
+MaybeLocal<Value> NativeModuleLoader::LookupAndCompile(
+    Local<Context> context,
+    const char* id,
+    std::vector<Local<String>>* parameters,
+    CompilationResultType result_type,
+    Environment* optional_env) {
   Isolate* isolate = context->GetIsolate();
   EscapableHandleScope scope(isolate);
+  Local<Value> ret;  // Used to convert to MaybeLocal before return
 
   Local<String> source = GetSource(isolate, id);
 
@@ -209,7 +237,7 @@ Local<Value> NativeModuleLoader::LookupAndCompile(Local<Context> context,
   //    built with them.
   // 2. If we are generating code cache for tools/general_code_cache.js, we
   //    are not going to use any cache ourselves.
-  if (has_code_cache_ && !produce_code_cache) {
+  if (has_code_cache_ && result_type == CompilationResultType::kFunction) {
     cached_data = GetCachedData(id);
     if (cached_data != nullptr) {
       use_cache = true;
@@ -219,7 +247,7 @@ Local<Value> NativeModuleLoader::LookupAndCompile(Local<Context> context,
   ScriptCompiler::Source script_source(source, origin, cached_data);
 
   ScriptCompiler::CompileOptions options;
-  if (produce_code_cache) {
+  if (result_type == CompilationResultType::kCodeCache) {
     options = ScriptCompiler::kEagerCompile;
   } else if (use_cache) {
     options = ScriptCompiler::kConsumeCodeCache;
@@ -227,42 +255,25 @@ Local<Value> NativeModuleLoader::LookupAndCompile(Local<Context> context,
     options = ScriptCompiler::kNoCompileOptions;
   }
 
-  MaybeLocal<Function> maybe_fun;
-  // Currently we assume if Environment is ready, then we must be compiling
-  // native modules instead of bootstrappers.
-  if (optional_env != nullptr) {
-    Local<String> parameters[] = {optional_env->exports_string(),
-                                  optional_env->require_string(),
-                                  optional_env->module_string(),
-                                  optional_env->process_string(),
-                                  optional_env->internal_binding_string()};
-    maybe_fun = ScriptCompiler::CompileFunctionInContext(context,
-                                                         &script_source,
-                                                         arraysize(parameters),
-                                                         parameters,
-                                                         0,
-                                                         nullptr,
-                                                         options);
-  } else {
-    // Until we migrate bootstrappers compilations here this is unreachable
-    // TODO(joyeecheung): it should be possible to generate the argument names
-    // from some special comments for the bootstrapper case.
-    // Note that for bootstrappers we may not be able to get the argument
-    // names as env->some_string() because we might be compiling before
-    // those strings are initialized.
-    UNREACHABLE();
-  }
+  MaybeLocal<Function> maybe_fun =
+      ScriptCompiler::CompileFunctionInContext(context,
+                                               &script_source,
+                                               parameters->size(),
+                                               parameters->data(),
+                                               0,
+                                               nullptr,
+                                               options);
 
-  Local<Function> fun;
   // This could fail when there are early errors in the native modules,
   // e.g. the syntax errors
-  if (maybe_fun.IsEmpty() || !maybe_fun.ToLocal(&fun)) {
+  if (maybe_fun.IsEmpty()) {
     // 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 MaybeLocal<Value>();
   }
 
+  Local<Function> fun = maybe_fun.ToLocalChecked();
   if (use_cache) {
     if (optional_env != nullptr) {
       // This could happen when Node is run with any v8 flag, but
@@ -279,7 +290,7 @@ Local<Value> NativeModuleLoader::LookupAndCompile(Local<Context> context,
     }
   }
 
-  if (produce_code_cache) {
+  if (result_type == CompilationResultType::kCodeCache) {
     std::unique_ptr<ScriptCompiler::CachedData> cached_data(
         ScriptCompiler::CreateCodeCacheForFunction(fun));
     CHECK_NE(cached_data, nullptr);
@@ -296,10 +307,12 @@ Local<Value> NativeModuleLoader::LookupAndCompile(Local<Context> context,
                          copied.release(),
                          cached_data_length,
                          ArrayBufferCreationMode::kInternalized);
-    return scope.Escape(Uint8Array::New(buf, 0, cached_data_length));
+    ret = Uint8Array::New(buf, 0, cached_data_length);
   } else {
-    return scope.Escape(fun);
+    ret = fun;
   }
+
+  return scope.Escape(ret);
 }
 
 void NativeModuleLoader::Initialize(Local<Object> target,
diff --git a/src/node_native_module.h b/src/node_native_module.h
index ad9343f1994630..76725ec0f2825b 100644
--- a/src/node_native_module.h
+++ b/src/node_native_module.h
@@ -16,10 +16,24 @@ namespace native_module {
 using NativeModuleRecordMap = std::map<std::string, UnionBytes>;
 using NativeModuleHashMap = std::map<std::string, std::string>;
 
-// The native (C++) side of the native module compilation.
-// This class should not depend on Environment
+// The native (C++) side of the NativeModule in JS land, which
+// handles compilation and caching of builtin modules (NativeModule)
+// and bootstrappers, whose source are bundled into the binary
+// as static data.
+// This class should not depend on a particular isolate, context, or
+// environment. Rather it should take them as arguments when necessary.
+// The instances of this class are per-process.
 class NativeModuleLoader {
  public:
+  // kCodeCache indicates that the compilation result should be returned
+  // as a Uint8Array, whereas kFunction indicates that the result should
+  // be returned as a Function.
+  // TODO(joyeecheung): it's possible to always produce code cache
+  // on the main thread and consume them in worker threads, or just
+  // share the cache among all the threads, although
+  // we need to decide whether to do that even when workers are not used.
+  enum class CompilationResultType { kCodeCache, kFunction };
+
   NativeModuleLoader();
   static void Initialize(v8::Local<v8::Object> target,
                          v8::Local<v8::Value> unused,
@@ -27,6 +41,18 @@ class NativeModuleLoader {
   v8::Local<v8::Object> GetSourceObject(v8::Local<v8::Context> context) const;
   v8::Local<v8::String> GetSource(v8::Isolate* isolate, const char* id) const;
 
+  // Run a script with JS source bundled inside the binary as if it's wrapped
+  // in a function called with a null receiver and arguments specified in C++.
+  // The returned value is empty if an exception is encountered.
+  // JS code run with this method can assume that their top-level
+  // declarations won't affect the global scope.
+  v8::MaybeLocal<v8::Value> CompileAndCall(
+      v8::Local<v8::Context> context,
+      const char* id,
+      std::vector<v8::Local<v8::String>>* parameters,
+      std::vector<v8::Local<v8::Value>>* arguments,
+      Environment* optional_env);
+
  private:
   static void GetCacheUsage(const v8::FunctionCallbackInfo<v8::Value>& args);
   // For legacy process.binding('natives') which is mutable, and for
@@ -48,17 +74,21 @@ class NativeModuleLoader {
   void LoadCodeCacheHash();  // Loads data into code_cache_hash_
 
   v8::ScriptCompiler::CachedData* GetCachedData(const char* id) const;
-  static v8::Local<v8::Value> CompileAsModule(Environment* env,
-                                              const char* id,
-                                              bool produce_code_cache);
-  // TODO(joyeecheung): make this public and reuse it to compile bootstrappers.
+
+  // Compile a script as a NativeModule that can be loaded via
+  // NativeModule.p.require in JS land.
+  static v8::MaybeLocal<v8::Value> CompileAsModule(
+      Environment* env, const char* id, CompilationResultType result_type);
+
   // For bootstrappers optional_env may be a nullptr.
-  // This method magically knows what parameter it should pass to
-  // the function to be compiled.
-  v8::Local<v8::Value> LookupAndCompile(v8::Local<v8::Context> context,
-                                        const char* id,
-                                        bool produce_code_cache,
-                                        Environment* optional_env);
+  // If an exception is encountered (e.g. source code contains
+  // syntax error), the returned value is empty.
+  v8::MaybeLocal<v8::Value> LookupAndCompile(
+      v8::Local<v8::Context> context,
+      const char* id,
+      std::vector<v8::Local<v8::String>>* parameters,
+      CompilationResultType result_type,
+      Environment* optional_env);
 
   bool has_code_cache_ = false;
   NativeModuleRecordMap source_;