fix: don't run environment bootstrapper (#22342)

patches/node/.patches

@@ -22,7 +22,6 @@ refactor_allow_embedder_overriding_of_internal_fs_calls.patch
 chore_prevent_warn_non_context-aware_native_modules_being_loaded.patch
 inherit_electron_crashpad_pipe_name_in_child_process.patch
 chore_read_nobrowserglobals_from_global_not_process.patch
-chore_split_createenvironment_into_createenvironment_and.patch
 build_bring_back_node_with_ltcg_configuration.patch
 revert_crypto_add_oaeplabel_option.patch
 refactor_transferrablemodule_is_deprecated_use_compiledwasmmodule.patch
@@ -39,3 +38,4 @@ fix_remove_implicit_type_conversions.patch
 fix_include_io_h_in_uvwasi_for_win.patch
 fix_--perf-prof_only_works_on_linux.patch
 fix_we_need_to_eager-load_cjs_modules.patch
+fix_don_t_preparemainexecution_twice.patch

patches/node/build_add_gn_build_files.patch

@@ -916,10 +916,10 @@ index 0000000000000000000000000000000000000000..f13b471d17128468bed06e66bd03a2ea
 +}
 diff --git a/filenames.json b/filenames.json
 new file mode 100644
-index 0000000000000000000000000000000000000000..147561938788aac7021fe340f64fd8b4303603e6
+index 0000000000000000000000000000000000000000..aa9aa6d32f7d33a6f82ccb32a52f53ed3d5a77f0
 --- /dev/null
 +++ b/filenames.json
-@@ -0,0 +1,461 @@
+@@ -0,0 +1,460 @@
 +// This file is automatically generated by generate_gn_filenames_json.py
 +// DO NOT EDIT
 +{
@@ -988,7 +988,6 @@ index 0000000000000000000000000000000000000000..147561938788aac7021fe340f64fd8b4
 +    }
 +  ],
 +  "library_files": [
-+    "lib/internal/bootstrap/environment.js",
 +    "lib/internal/bootstrap/loaders.js",
 +    "lib/internal/bootstrap/node.js",
 +    "lib/internal/bootstrap/pre_execution.js",
@@ -1587,7 +1586,7 @@ index 0000000000000000000000000000000000000000..f3c5c798c0aefcb8cf9b1570a7b4817c
 +  args = rebase_path(inputs + outputs, root_build_dir)
 +}
 diff --git a/src/node_version.h b/src/node_version.h
-index 489dff631e51d1e93ed79acc48e52d33cd9c66d1..3f7349a4fa77aaae258ed32f378384165260c5b0 100644
+index e5205bb303fab8e53b56db68543a47c47280e3d4..53b97162319a0cc300b2d8950ffb414db6bf52e8 100644
 --- a/src/node_version.h
 +++ b/src/node_version.h
 @@ -89,7 +89,10 @@

patches/node/chore_prevent_warn_non_context-aware_native_modules_being_loaded.patch

@@ -8,7 +8,7 @@ once we stop warning and begin to unilaterally prevent non-context aware modules
 from being loaded.
 
 diff --git a/lib/internal/bootstrap/pre_execution.js b/lib/internal/bootstrap/pre_execution.js
-index e7bd18855fb98a822833b4366bfb595dccfc1b6f..a96f891f675cc598e912a5e010c24938a9f6fc98 100644
+index c86b5851ca9332ca440443e5feab2e65c7ac30b9..9416cf377871c217413bd9bcc27a7b35c9de1ca2 100644
 --- a/lib/internal/bootstrap/pre_execution.js
 +++ b/lib/internal/bootstrap/pre_execution.js
 @@ -80,8 +80,10 @@ function patchProcessObject(expandArgv1) {

patches/node/chore_split_createenvironment_into_createenvironment_and.patch

@@ -1,69 +0,0 @@
-From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
-From: Samuel Attard <sattard@slack-corp.com>
-Date: Wed, 17 Jul 2019 14:45:59 -0700
-Subject: chore: split CreateEnvironment into CreateEnvironment and
- BootstrapEnvironment
-
-This allows us to run operations on a created but not yet bootstrapped
-environment such as setting up an InspectorAgent
-
-diff --git a/src/api/environment.cc b/src/api/environment.cc
-index 356131156b4a714eebf4e202cd105f0f184e3852..96192f0ea4174315d73e03849ce1bed996afc44c 100644
---- a/src/api/environment.cc
-+++ b/src/api/environment.cc
-@@ -263,7 +263,8 @@ Environment* CreateEnvironment(IsolateData* isolate_data,
-                                int argc,
-                                const char* const* argv,
-                                int exec_argc,
--                               const char* const* exec_argv) {
-+                               const char* const* exec_argv,
-+                               bool bootstrap) {
-   Isolate* isolate = context->GetIsolate();
-   HandleScope handle_scope(isolate);
-   Context::Scope context_scope(context);
-@@ -281,9 +282,16 @@ Environment* CreateEnvironment(IsolateData* isolate_data,
-                                       Environment::kOwnsProcessState |
-                                       Environment::kOwnsInspector));
-   env->InitializeLibuv(per_process::v8_is_profiling);
--  if (env->RunBootstrapping().IsEmpty()) {
-+  if (bootstrap && !BootstrapEnvironment(env)) {
-     return nullptr;
-   }
-+  return env;
-+}
-+
-+bool BootstrapEnvironment(Environment* env) {
-+  if (env->RunBootstrapping().IsEmpty()) {
-+    return false;
-+  }
- 
-   std::vector<Local<String>> parameters = {
-       env->require_string(),
-@@ -296,9 +304,10 @@ Environment* CreateEnvironment(IsolateData* isolate_data,
-   if (ExecuteBootstrapper(
-           env, "internal/bootstrap/environment", &parameters, &arguments)
-           .IsEmpty()) {
--    return nullptr;
-+    return false;
-   }
--  return env;
-+
-+  return true;
- }
- 
- void FreeEnvironment(Environment* env) {
-diff --git a/src/node.h b/src/node.h
-index e3258434eba34124c71562225e295cd1807fdf7c..2ac35208ae8b2cfd067b5b712d4447121ef6e47d 100644
---- a/src/node.h
-+++ b/src/node.h
-@@ -326,7 +326,9 @@ NODE_EXTERN Environment* CreateEnvironment(IsolateData* isolate_data,
-                                            int argc,
-                                            const char* const* argv,
-                                            int exec_argc,
--                                           const char* const* exec_argv);
-+                                           const char* const* exec_argv,
-+                                           bool bootstrap = true);
-+NODE_EXTERN bool BootstrapEnvironment(Environment* env);
- 
- NODE_EXTERN void LoadEnvironment(Environment* env);
- NODE_EXTERN void FreeEnvironment(Environment* env);

patches/node/fix_don_t_preparemainexecution_twice.patch

@@ -0,0 +1,58 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Shelley Vohr <shelley.vohr@gmail.com>
+Date: Fri, 21 Feb 2020 17:21:11 -0800
+Subject: fix: don't prepareMainExecution twice
+
+In https://github.com/nodejs/node/pull/26788 (v12.0.0), Node.js added a
+bootstrapper to `CreateEnvironment`, which would prepare the main thread
+for execution any time an embedder created a new environment. However, this
+caused an unfortunate doubling-up effect; Node.js also ran bootstrapping
+(called `prepareMainThreadExecution`) for all other execution paths
+(like the repl, the actual main module, eval, etc).
+
+To fix this, we can just remove bootstrapping code from `CreateEnvironment`. 
+
+diff --git a/lib/internal/bootstrap/environment.js b/lib/internal/bootstrap/environment.js
+deleted file mode 100644
+index 79a67dae378202ee377f2f138560b74f673af6e4..0000000000000000000000000000000000000000
+--- a/lib/internal/bootstrap/environment.js
++++ /dev/null
+@@ -1,13 +0,0 @@
+-'use strict';
+-
+-// This runs necessary preparations to prepare a complete Node.js context
+-// that depends on run time states.
+-// It is currently only intended for preparing contexts for embedders.
+-
+-/* global markBootstrapComplete */
+-const {
+-  prepareMainThreadExecution
+-} = require('internal/bootstrap/pre_execution');
+-
+-prepareMainThreadExecution();
+-markBootstrapComplete();
+diff --git a/src/api/environment.cc b/src/api/environment.cc
+index 356131156b4a714eebf4e202cd105f0f184e3852..6c2e0555f2ee554c8ac29465af01e9c47e1d81f9 100644
+--- a/src/api/environment.cc
++++ b/src/api/environment.cc
+@@ -284,20 +284,6 @@ Environment* CreateEnvironment(IsolateData* isolate_data,
+   if (env->RunBootstrapping().IsEmpty()) {
+     return nullptr;
+   }
+-
+-  std::vector<Local<String>> parameters = {
+-      env->require_string(),
+-      FIXED_ONE_BYTE_STRING(env->isolate(), "markBootstrapComplete")};
+-  std::vector<Local<Value>> arguments = {
+-      env->native_module_require(),
+-      env->NewFunctionTemplate(MarkBootstrapComplete)
+-          ->GetFunction(env->context())
+-          .ToLocalChecked()};
+-  if (ExecuteBootstrapper(
+-          env, "internal/bootstrap/environment", &parameters, &arguments)
+-          .IsEmpty()) {
+-    return nullptr;
+-  }
+   return env;
+ }
+ 

patches/node/fix_key_gen_apis_are_not_available_in_boringssl.patch

@@ -6,7 +6,7 @@ Subject: fix: key gen APIs are not available in BoringSSL
 This will make Node's key pair generation APIs fail.
 
 diff --git a/src/node_crypto.cc b/src/node_crypto.cc
-index ad06141752eae90a655d710932b479f9cd8482ff..861d321d6170b25e874080d4c70dda201cab3248 100644
+index ac2d969f60cf95e0ad4972d67d83642d7161a579..114b4d503f3c1c01aa9999cf60a044d80a39acc1 100644
 --- a/src/node_crypto.cc
 +++ b/src/node_crypto.cc
 @@ -290,24 +290,14 @@ Maybe<bool> Decorate(Environment* env, Local<Object> obj,
@@ -34,7 +34,7 @@ index ad06141752eae90a655d710932b479f9cd8482ff..861d321d6170b25e874080d4c70dda20
      V(USER)                                                                   \
  
  #define V(name) case ERR_LIB_##name: lib = #name "_"; break;
-@@ -2557,8 +2547,11 @@ void SSLWrap<Base>::GetEphemeralKeyInfo(
+@@ -2560,8 +2550,11 @@ void SSLWrap<Base>::GetEphemeralKeyInfo(
              .Check();
          break;
        case EVP_PKEY_EC:
@@ -46,7 +46,7 @@ index ad06141752eae90a655d710932b479f9cd8482ff..861d321d6170b25e874080d4c70dda20
          {
            const char* curve_name;
            if (kid == EVP_PKEY_EC) {
-@@ -3894,12 +3887,15 @@ Local<Value> KeyObject::GetAsymmetricKeyType() const {
+@@ -3897,12 +3890,15 @@ Local<Value> KeyObject::GetAsymmetricKeyType() const {
      return env()->crypto_ec_string();
    case EVP_PKEY_ED25519:
      return env()->crypto_ed25519_string();
@@ -62,7 +62,7 @@ index ad06141752eae90a655d710932b479f9cd8482ff..861d321d6170b25e874080d4c70dda20
    default:
      return Undefined(env()->isolate());
    }
-@@ -6402,6 +6398,8 @@ class DSAKeyPairGenerationConfig : public KeyPairGenerationConfig {
+@@ -6405,6 +6401,8 @@ class DSAKeyPairGenerationConfig : public KeyPairGenerationConfig {
      if (EVP_PKEY_paramgen_init(param_ctx.get()) <= 0)
        return nullptr;
  
@@ -71,7 +71,7 @@ index ad06141752eae90a655d710932b479f9cd8482ff..861d321d6170b25e874080d4c70dda20
      if (EVP_PKEY_CTX_set_dsa_paramgen_bits(param_ctx.get(), modulus_bits_) <= 0)
        return nullptr;
  
-@@ -6421,6 +6419,8 @@ class DSAKeyPairGenerationConfig : public KeyPairGenerationConfig {
+@@ -6424,6 +6422,8 @@ class DSAKeyPairGenerationConfig : public KeyPairGenerationConfig {
  
      EVPKeyCtxPointer key_ctx(EVP_PKEY_CTX_new(params.get(), nullptr));
      return key_ctx;
@@ -80,7 +80,7 @@ index ad06141752eae90a655d710932b479f9cd8482ff..861d321d6170b25e874080d4c70dda20
    }
  
   private:
-@@ -7101,9 +7101,12 @@ void Initialize(Local<Object> target,
+@@ -7104,9 +7104,12 @@ void Initialize(Local<Object> target,
    env->SetMethod(target, "generateKeyPairEC", GenerateKeyPairEC);
    env->SetMethod(target, "generateKeyPairNid", GenerateKeyPairNid);
    NODE_DEFINE_CONSTANT(target, EVP_PKEY_ED25519);

patches/node/revert_crypto_add_oaeplabel_option.patch

@@ -8,7 +8,7 @@ The BoringSSL incompatibilities (OPENSSL_memdup) will be shimmed in and this sho
 be removed when the associated update is rolled into Chromium.
 
 diff --git a/src/node_crypto.cc b/src/node_crypto.cc
-index 7950c68f52050b7c9a6798b653a9c831256b21b5..41b72d010824234c567586524d22cae6ac849edb 100644
+index 114b4d503f3c1c01aa9999cf60a044d80a39acc1..de6828fc44752789bef02d29d3320dbb6f41448a 100644
 --- a/src/node_crypto.cc
 +++ b/src/node_crypto.cc
 @@ -5514,18 +5514,6 @@ bool PublicKeyCipher::Cipher(Environment* env,

script/node-disabled-tests.json

@@ -20,24 +20,8 @@
   "async-hooks/test-signalwrap",
   "async-hooks/test-statwatcher",
   "async-hooks/test-timers.setInterval",
-  "es-module/test-cjs-esm-warn",
-  "es-module/test-esm-cjs-load-error-note",
-  "es-module/test-esm-cjs-main",
-  "es-module/test-esm-default-type",
-  "es-module/test-esm-json-cache",
-  "es-module/test-esm-no-extension",
-  "es-module/test-esm-scope-node-modules",
-  "es-module/test-esm-snapshot",
-  "es-module/test-esm-specifiers",
-  "es-module/test-esm-type-flag",
   "message/async_error_eval_esm",
-  "message/async_error_sync_esm",
-  "message/esm_display_syntax_error",
-  "message/esm_display_syntax_error_import",
-  "message/esm_display_syntax_error_import_module",
-  "message/esm_display_syntax_error_module",
   "message/esm_loader_not_found",
-  "message/esm_loader_syntax_error",
   "message/source_map_throw_catch",
   "message/source_map_throw_first_tick",
   "message/source_map_throw_set_immediate",
@@ -207,7 +191,6 @@
   "parallel/test-vm-sigint-existing-handler",
   "parallel/test-vm-syntax-error-message",
   "parallel/test-vm-timeout",
-  "parallel/test-warn-sigprof",
   "parallel/test-whatwg-encoding-custom-textdecoder",
   "parallel/test-worker-debug",
   "parallel/test-worker-invalid-workerdata",
@@ -220,17 +203,8 @@
   "parallel/test-worker-stdio",
   "parallel/test-wrap-js-stream-exceptions",
   "parallel/test-zlib-unused-weak",
-  "pseudo-tty/console_colors",
-  "pseudo-tty/ref_keeps_node_running",
-  "pseudo-tty/test-async-wrap-getasyncid-tty",
-  "pseudo-tty/test-fatal-error",
-  "pseudo-tty/test-handle-wrap-isrefed-tty",
-  "pseudo-tty/test-set-raw-mode-reset",
   "pseudo-tty/test-set-raw-mode-reset-process-exit",
   "pseudo-tty/test-set-raw-mode-reset-signal",
-  "pseudo-tty/test-tty-color-support",
-  "pseudo-tty/test-tty-window-size",
-  "pseudo-tty/test-tty-wrap",
   "report/test-report-config",
   "report/test-report-getreport",
   "report/test-report-signal",
@@ -251,9 +225,6 @@
   "sequential/test-heap-prof",
   "sequential/test-heapdump",
   "sequential/test-heapdump-flag",
-  "sequential/test-inspector",
-  "sequential/test-inspector-async-call-stack-abort",
-  "sequential/test-inspector-console",
   "sequential/test-inspector-contexts",
   "sequential/test-inspector-port-zero",
   "sequential/test-inspector-resource-name-to-url",

shell/app/node_main.cc

@@ -142,17 +142,14 @@ int NodeMain(int argc, char* argv[]) {
         node::CreateIsolateData(gin_env.isolate(), loop, gin_env.platform());
     CHECK_NE(nullptr, isolate_data);
 
-    node::Environment* env =
+    node::Environment* env = node::CreateEnvironment(
-        node::CreateEnvironment(isolate_data, gin_env.context(), argc, argv,
+        isolate_data, gin_env.context(), argc, argv, exec_argc, exec_argv);
-                                exec_argc, exec_argv, false);
     CHECK_NE(nullptr, env);
 
     // Enable support for v8 inspector.
     NodeDebugger node_debugger(env);
     node_debugger.Start();
 
-    node::BootstrapEnvironment(env);
-
     // TODO(codebytere): we shouldn't have to call this - upstream?
     env->InitializeDiagnostics();
 

shell/browser/electron_browser_main_parts.cc

@@ -296,44 +296,13 @@ void ElectronBrowserMainParts::PostEarlyInitialization() {
   node_bindings_->Initialize();
   // Create the global environment.
   node::Environment* env = node_bindings_->CreateEnvironment(
-      js_env_->context(), js_env_->platform(), false);
+      js_env_->context(), js_env_->platform());
   node_env_ = std::make_unique<NodeEnvironment>(env);
 
-  /**
-   * 🚨  🚨  🚨  🚨  🚨  🚨  🚨  🚨  🚨
-   * UNSAFE ENVIRONMENT BLOCK BEGINS
-   *
-   * DO NOT USE node::Environment inside this block, bad things will happen
-   * and you won't be able to figure out why.  Just don't touch it, the only
-   * thing that can use it is NodeDebugger and that is ONLY allowed to access
-   * the inspector agent.
-   *
-   * This is unsafe because the environment is not yet bootstrapped, it's a race
-   * condition where we can't bootstrap before intializing the inspector agent.
-   *
-   * Long term we should figure out how to get node to initialize the inspector
-   * agent in the correct place without us splitting the bootstrap up, but for
-   * now this works.
-   * 🚨  🚨  🚨  🚨  🚨  🚨  🚨  🚨  🚨
-   */
-
   // Enable support for v8 inspector
   node_debugger_ = std::make_unique<NodeDebugger>(env);
   node_debugger_->Start();
 
-  // Only run the node bootstrapper after we have initialized the inspector
-  // TODO(MarshallOfSound): Figured out a better way to init the inspector
-  // before bootstrapping
-  node::BootstrapEnvironment(env);
-
-  /**
-   * ✅  ✅  ✅  ✅  ✅  ✅  ✅
-   * UNSAFE ENVIRONMENT BLOCK ENDS
-   *
-   * Do whatever you want now with that env, it's safe again
-   * ✅  ✅  ✅  ✅  ✅  ✅  ✅
-   */
-
   // Add Electron extended APIs.
   electron_bindings_->BindTo(js_env_->isolate(), env->process_object());
 

shell/common/node_bindings.cc

@@ -349,8 +349,7 @@ void NodeBindings::Initialize() {
 
 node::Environment* NodeBindings::CreateEnvironment(
     v8::Handle<v8::Context> context,
-    node::MultiIsolatePlatform* platform,
+    node::MultiIsolatePlatform* platform) {
-    bool bootstrap_env) {
 #if defined(OS_WIN)
   auto& atom_args = ElectronCommandLine::argv();
   std::vector<std::string> args(atom_args.size());
@@ -389,9 +388,8 @@ node::Environment* NodeBindings::CreateEnvironment(
   std::unique_ptr<const char*[]> c_argv = StringVectorToArgArray(args);
   isolate_data_ =
       node::CreateIsolateData(context->GetIsolate(), uv_loop_, platform);
-  node::Environment* env =
+  node::Environment* env = node::CreateEnvironment(
-      node::CreateEnvironment(isolate_data_, context, args.size(), c_argv.get(),
+      isolate_data_, context, args.size(), c_argv.get(), 0, nullptr);
-                              0, nullptr, bootstrap_env);
   DCHECK(env);
 
   // Clean up the global _noBrowserGlobals that we unironically injected into
@@ -399,7 +397,6 @@ node::Environment* NodeBindings::CreateEnvironment(
   if (browser_env_ != BrowserEnvironment::BROWSER) {
     // We need to bootstrap the env in non-browser processes so that
     // _noBrowserGlobals is read correctly before we remove it
-    DCHECK(bootstrap_env);
     global.Delete("_noBrowserGlobals");
   }
 

shell/common/node_bindings.h

@@ -43,8 +43,7 @@ class NodeBindings {
 
   // Create the environment and load node.js.
   node::Environment* CreateEnvironment(v8::Handle<v8::Context> context,
-                                       node::MultiIsolatePlatform* platform,
+                                       node::MultiIsolatePlatform* platform);
-                                       bool bootstrap_env);
 
   // Load node.js in the environment.
   void LoadEnvironment(node::Environment* env);

shell/renderer/electron_renderer_client.cc

@@ -125,7 +125,7 @@ void ElectronRendererClient::DidCreateScriptContext(
   CHECK(initialized);
 
   node::Environment* env =
-      node_bindings_->CreateEnvironment(renderer_context, nullptr, true);
+      node_bindings_->CreateEnvironment(renderer_context, nullptr);
 
   // If we have disabled the site instance overrides we should prevent loading
   // any non-context aware native module

shell/renderer/web_worker_observer.cc

@@ -52,7 +52,7 @@ void WebWorkerObserver::ContextCreated(v8::Local<v8::Context> worker_context) {
   bool initialized = node::InitializeContext(worker_context);
   CHECK(initialized);
   node::Environment* env =
-      node_bindings_->CreateEnvironment(worker_context, nullptr, true);
+      node_bindings_->CreateEnvironment(worker_context, nullptr);
 
   // Add Electron extended APIs.
   electron_bindings_->BindTo(env->isolate(), env->process_object());