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

* chore: remove node "split CreateEnvironment" patch

* chore: remove `environment.js` from electron-node

* chore: don't call the electron-node API we removed

* fix: don't prepareMainExecution twice

* test: re-enable some node tests

Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

patches/node/.patches

@@ -23,7 +23,6 @@ build_modify_js2c_py_to_allow_injection_of_original-fs_and_custom_embedder_js.pa
 refactor_allow_embedder_overriding_of_internal_fs_calls.patch
 chore_prevent_warn_non_context-aware_native_modules_being_loaded.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
@@ -37,3 +36,4 @@ test_use_tmpdir_refresh_in_test-esm-windows_js.patch
 override_existing_v8_reallocate.patch
 avoid_calling_deprecated_method.patch
 remove_deprecated_wasm_module_type_check.patch
+fix_don_t_preparemainexecution_twice.patch

patches/node/build_add_gn_build_files.patch

@@ -885,10 +885,10 @@ index 0000000000000000000000000000000000000000..f13b471d17128468bed06e66bd03a2ea
 +}
 diff --git a/filenames.json b/filenames.json
 new file mode 100644
-index 0000000000000000000000000000000000000000..db73a7699cdb1925c723fd1708d6ce1fad1cc946
+index 0000000000000000000000000000000000000000..aa9aa6d32f7d33a6f82ccb32a52f53ed3d5a77f0
 --- /dev/null
 +++ b/filenames.json
-@@ -0,0 +1,448 @@
+@@ -0,0 +1,447 @@
 +// This file is automatically generated by generate_gn_filenames_json.py
 +// DO NOT EDIT
 +{
@@ -957,7 +957,6 @@ index 0000000000000000000000000000000000000000..db73a7699cdb1925c723fd1708d6ce1f
 +    }
 +  ],
 +  "library_files": [
-+    "lib/internal/bootstrap/environment.js",
 +    "lib/internal/bootstrap/loaders.js",
 +    "lib/internal/bootstrap/node.js",
 +    "lib/internal/bootstrap/pre_execution.js",

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: Wed, 20 May 2020 17:37:39 -0500
+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;
+ }
+ 

script/node-disabled-tests.json

@@ -20,22 +20,7 @@
   "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/source_map_throw_catch",
   "message/source_map_throw_first_tick",
   "message/source_map_throw_set_immediate",
@@ -211,7 +196,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",
@@ -224,17 +208,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",
@@ -260,9 +235,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",
@@ -274,4 +246,4 @@
   "sequential/test-tls-connect",
   "sequential/test-vm-timeout-rethrow",
   "wpt/test-encoding"
-]
+]

shell/app/node_main.cc

@@ -150,17 +150,14 @@ int NodeMain(int argc, char* argv[]) {
     v8::Isolate::Scope isolate_scope(isolate);
     v8::HandleScope handle_scope(isolate);
 
-    node::Environment* env =
-        node::CreateEnvironment(isolate_data, gin_env.context(), argc, argv,
-                                exec_argc, exec_argv, false);
+    node::Environment* env = node::CreateEnvironment(
+        isolate_data, gin_env.context(), argc, argv, exec_argc, exec_argv);
     CHECK_NE(nullptr, env);
 
     // Enable support for v8 inspector.
     NodeDebugger node_debugger(env);
     node_debugger.Start();
 
-    node::BootstrapEnvironment(env);
-
     gin_helper::Dictionary process(isolate, env->process_object());
 
     isolate->SetAllowWasmCodeGenerationCallback(

shell/browser/electron_browser_main_parts.cc

@@ -297,44 +297,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

@@ -300,8 +300,7 @@ void NodeBindings::Initialize() {
 
 node::Environment* NodeBindings::CreateEnvironment(
     v8::Handle<v8::Context> context,
-    node::MultiIsolatePlatform* platform,
-    bool bootstrap_env) {
+    node::MultiIsolatePlatform* platform) {
 #if defined(OS_WIN)
   auto& atom_args = ElectronCommandLine::argv();
   std::vector<std::string> args(atom_args.size());
@@ -340,9 +339,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::CreateEnvironment(isolate_data_, context, args.size(), c_argv.get(),
-                              0, nullptr, bootstrap_env);
+  node::Environment* env = node::CreateEnvironment(
+      isolate_data_, context, args.size(), c_argv.get(), 0, nullptr);
   DCHECK(env);
 
   // Clean up the global _noBrowserGlobals that we unironically injected into
@@ -350,7 +348,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,
-                                       bool bootstrap_env);
+                                       node::MultiIsolatePlatform* platform);
 
   // 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());