fix: bootstrap the node environment after we setup the InspectorAgent (#19317)

patches/node/.patches

@@ -38,3 +38,4 @@ src_add_missing_option_parser_template_for_the_debugoptionsparser.patch
 src_expose_maybeinitializecontext_to_allow_existing_contexts.patch
 fix_extern_the_nativemoduleenv_and_options_parser_for_debug_builds.patch
 chore_read_nobrowserglobals_from_global_not_process.patch
+chore_split_createenvironment_into_createenvironment_and.patch

patches/node/chore_split_createenvironment_into_createenvironment_and.patch

@@ -0,0 +1,69 @@
+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 5011774be2c5fee910079790feae747fa1785b87..d5cb74025959ad32a6d2e6a914bf99a2b3d7946f 100644
+--- a/src/api/environment.cc
++++ b/src/api/environment.cc
+@@ -269,7 +269,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);
+@@ -287,9 +288,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(),
+@@ -302,9 +310,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 de007ed97a52c17cff8b8917d25f2383d5bbae6a..cdf96c283dbfb4c763d0b0e21497fc289e28741a 100644
+--- a/src/node.h
++++ b/src/node.h
+@@ -330,7 +330,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);

shell/browser/atom_browser_main_parts.cc

@@ -285,13 +285,44 @@ void AtomBrowserMainParts::PostEarlyInitialization() {
   node_bindings_->Initialize();
   // Create the global environment.
   node::Environment* env = node_bindings_->CreateEnvironment(
-      js_env_->context(), js_env_->platform());
+      js_env_->context(), js_env_->platform(), false);
   node_env_.reset(new 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_.reset(new 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

@@ -288,7 +288,8 @@ 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 = AtomCommandLine::argv();
   std::vector<std::string> args(atom_args.size());
@@ -327,13 +328,17 @@ node::Environment* NodeBindings::CreateEnvironment(
   std::unique_ptr<const char*[]> c_argv = StringVectorToArgArray(args);
   node::Environment* env = node::CreateEnvironment(
       node::CreateIsolateData(context->GetIsolate(), uv_loop_, platform),
-      context, args.size(), c_argv.get(), 0, nullptr);
+      context, args.size(), c_argv.get(), 0, nullptr, bootstrap_env);
   DCHECK(env);
 
   // Clean up the global _noBrowserGlobals that we unironically injected into
   // the global scope
-  if (browser_env_ != BrowserEnvironment::BROWSER)
+  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");
+  }
 
   if (browser_env_ == BrowserEnvironment::BROWSER) {
     // SetAutorunMicrotasks is no longer called in node::CreateEnvironment

shell/common/node_bindings.h

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

shell/renderer/atom_renderer_client.cc

@@ -107,7 +107,8 @@ void AtomRendererClient::DidCreateScriptContext(
   v8::Local<v8::Context> context =
       node::MaybeInitializeContext(renderer_context);
   DCHECK(!context.IsEmpty());
-  node::Environment* env = node_bindings_->CreateEnvironment(context);
+  node::Environment* env =
+      node_bindings_->CreateEnvironment(context, nullptr, true);
   auto* command_line = base::CommandLine::ForCurrentProcess();
   // If we have disabled the site instance overrides we should prevent loading
   // any non-context aware native module

shell/renderer/web_worker_observer.cc

@@ -50,7 +50,8 @@ void WebWorkerObserver::ContextCreated(v8::Local<v8::Context> worker_context) {
   // Setup node environment for each window.
   v8::Local<v8::Context> context = node::MaybeInitializeContext(worker_context);
   DCHECK(!context.IsEmpty());
-  node::Environment* env = node_bindings_->CreateEnvironment(context);
+  node::Environment* env =
+      node_bindings_->CreateEnvironment(context, nullptr, true);
 
   // Add Electron extended APIs.
   electron_bindings_->BindTo(env->isolate(), env->process_object());