vm: don't set host-defined options when it's not necessary

This makes it possile to hit the in-isolate compilation cache when
host-defined options are not necessary.

benchmark/vm/compile-script-in-isolate-cache.js

@@ -0,0 +1,35 @@
+'use strict';
+
+// This benchmarks compiling scripts that hit the in-isolate compilation
+// cache (by having the same source).
+const common = require('../common.js');
+const fs = require('fs');
+const vm = require('vm');
+const fixtures = require('../../test/common/fixtures.js');
+const scriptPath = fixtures.path('snapshot', 'typescript.js');
+
+const bench = common.createBenchmark(main, {
+  type: ['with-dynamic-import-callback', 'without-dynamic-import-callback'],
+  n: [100],
+});
+
+const scriptSource = fs.readFileSync(scriptPath, 'utf8');
+
+function main({ n, type }) {
+  let script;
+  bench.start();
+  let options = {};
+  switch (type) {
+    case 'with-dynamic-import-callback':
+      // Use a dummy callback for now until we really need to benchmark it.
+      options.importModuleDynamically = async() => {};
+      break;
+    case 'without-dynamic-import-callback':
+      break;
+  }
+  for (let i = 0; i < n; i++) {
+    script = new vm.Script(scriptSource, options);
+  }
+  bench.end(n);
+  script.runInThisContext();
+}

lib/vm.js

@@ -86,6 +86,12 @@ class Script extends ContextifyScript {
     }
     validateBoolean(produceCachedData, 'options.produceCachedData');
 
+    if (importModuleDynamically !== undefined) {
+      // Check that it's either undefined or a function before we pass
+      // it into the native constructor.
+      validateFunction(importModuleDynamically,
+                       'options.importModuleDynamically');
+    }
     // Calling `ReThrow()` on a native TryCatch does not generate a new
     // abort-on-uncaught-exception check. A dummy try/catch in JS land
     // protects against that.
@@ -96,14 +102,13 @@ class Script extends ContextifyScript {
             columnOffset,
             cachedData,
             produceCachedData,
-            parsingContext);
+            parsingContext,
+            importModuleDynamically);
     } catch (e) {
       throw e; /* node-do-not-add-exception-line */
     }
 
     if (importModuleDynamically !== undefined) {
-      validateFunction(importModuleDynamically,
-                       'options.importModuleDynamically');
       const { importModuleDynamicallyWrap } = require('internal/vm/module');
       const { registerModule } = require('internal/modules/esm/utils');
       registerModule(this, {

src/crypto/crypto_context.cc

@@ -1052,34 +1052,60 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
   EVP_PKEY* pkey_ptr = nullptr;
   X509* cert_ptr = nullptr;
   STACK_OF(X509)* extra_certs_ptr = nullptr;
-  if (d2i_PKCS12_bio(in.get(), &p12_ptr) &&
-      (p12.reset(p12_ptr), true) &&  // Move ownership to the smart pointer.
-      PKCS12_parse(p12.get(), pass.data(),
-                   &pkey_ptr,
-                   &cert_ptr,
-                   &extra_certs_ptr) &&
-      (pkey.reset(pkey_ptr), cert.reset(cert_ptr),
-       extra_certs.reset(extra_certs_ptr), true) &&  // Move ownership.
-      SSL_CTX_use_certificate_chain(sc->ctx_.get(),
-                                    std::move(cert),
-                                    extra_certs.get(),
-                                    &sc->cert_,
-                                    &sc->issuer_) &&
-      SSL_CTX_use_PrivateKey(sc->ctx_.get(), pkey.get())) {
-    // Add CA certs too
-    for (int i = 0; i < sk_X509_num(extra_certs.get()); i++) {
-      X509* ca = sk_X509_value(extra_certs.get(), i);
-
-      if (cert_store == GetOrCreateRootCertStore()) {
-        cert_store = NewRootCertStore();
-        SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store);
-      }
-      X509_STORE_add_cert(cert_store, ca);
-      SSL_CTX_add_client_CA(sc->ctx_.get(), ca);
+
+  if (!d2i_PKCS12_bio(in.get(), &p12_ptr)) {
+    goto done;
+  }
+
+  // Move ownership to the smart pointer:
+  p12.reset(p12_ptr);
+
+  if (!PKCS12_parse(
+          p12.get(), pass.data(), &pkey_ptr, &cert_ptr, &extra_certs_ptr)) {
+    goto done;
+  }
+
+  // Move ownership of the parsed data:
+  pkey.reset(pkey_ptr);
+  cert.reset(cert_ptr);
+  extra_certs.reset(extra_certs_ptr);
+
+  if (!pkey) {
+    return THROW_ERR_CRYPTO_OPERATION_FAILED(
+        env, "Unable to load private key from PFX data");
+  }
+
+  if (!cert) {
+    return THROW_ERR_CRYPTO_OPERATION_FAILED(
+        env, "Unable to load certificate from PFX data");
+  }
+
+  if (!SSL_CTX_use_certificate_chain(sc->ctx_.get(),
+                                     std::move(cert),
+                                     extra_certs.get(),
+                                     &sc->cert_,
+                                     &sc->issuer_)) {
+    goto done;
+  }
+
+  if (!SSL_CTX_use_PrivateKey(sc->ctx_.get(), pkey.get())) {
+    goto done;
+  }
+
+  // Add CA certs too
+  for (int i = 0; i < sk_X509_num(extra_certs.get()); i++) {
+    X509* ca = sk_X509_value(extra_certs.get(), i);
+
+    if (cert_store == GetOrCreateRootCertStore()) {
+      cert_store = NewRootCertStore();
+      SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store);
     }
-    ret = true;
+    X509_STORE_add_cert(cert_store, ca);
+    SSL_CTX_add_client_CA(sc->ctx_.get(), ca);
   }
+  ret = true;
 
+done:
   if (!ret) {
     // TODO(@jasnell): Should this use ThrowCryptoError?
     unsigned long err = ERR_get_error();  // NOLINT(runtime/int)

src/node_contextify.cc

@@ -771,10 +771,13 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
   bool produce_cached_data = false;
   Local<Context> parsing_context = context;
 
+  Local<Symbol> host_defined_options_id;
+  Local<PrimitiveArray> host_defined_options;
   if (argc > 2) {
     // new ContextifyScript(code, filename, lineOffset, columnOffset,
-    //                      cachedData, produceCachedData, parsingContext)
-    CHECK_EQ(argc, 7);
+    //                      cachedData, produceCachedData, parsingContext,
+    //                      needsHostDefinedOptions)
+    CHECK_EQ(argc, 8);
     CHECK(args[2]->IsNumber());
     line_offset = args[2].As<Int32>()->Value();
     CHECK(args[3]->IsNumber());
@@ -793,6 +796,13 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
       CHECK_NOT_NULL(sandbox);
       parsing_context = sandbox->context();
     }
+    if (!args[7]->IsUndefined()) {
+      host_defined_options_id = Symbol::New(isolate, filename);
+      host_defined_options =
+          PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength);
+      host_defined_options->Set(
+          isolate, loader::HostDefinedOptions::kID, host_defined_options_id);
+    }
   }
 
   ContextifyScript* contextify_script =
@@ -814,12 +824,6 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
         data + cached_data_buf->ByteOffset(), cached_data_buf->ByteLength());
   }
 
-  Local<PrimitiveArray> host_defined_options =
-      PrimitiveArray::New(isolate, loader::HostDefinedOptions::kLength);
-  Local<Symbol> id_symbol = Symbol::New(isolate, filename);
-  host_defined_options->Set(
-      isolate, loader::HostDefinedOptions::kID, id_symbol);
-
   ScriptOrigin origin(isolate,
                       filename,
                       line_offset,                          // line offset
@@ -865,8 +869,11 @@ void ContextifyScript::New(const FunctionCallbackInfo<Value>& args) {
     new_cached_data.reset(ScriptCompiler::CreateCodeCache(v8_script));
   }
 
-  if (contextify_script->object()
-          ->SetPrivate(context, env->host_defined_option_symbol(), id_symbol)
+  if (!host_defined_options_id.IsEmpty() &&
+      contextify_script->object()
+          ->SetPrivate(context,
+                       env->host_defined_option_symbol(),
+                       host_defined_options_id)
           .IsNothing()) {
     return;
   }