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

atom/browser/atom_browser_main_parts.cc

@@ -302,13 +302,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());
 

atom/common/node_bindings.cc

@@ -294,7 +294,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());
@@ -335,14 +336,19 @@ 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_ == BROWSER) {
+  }
+
+  if (browser_env_ == BrowserEnvironment::BROWSER) {
     // SetAutorunMicrotasks is no longer called in node::CreateEnvironment
     // so instead call it here to match expected node behavior
     context->GetIsolate()->SetMicrotasksPolicy(v8::MicrotasksPolicy::kExplicit);

atom/common/node_bindings.h

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

atom/renderer/atom_renderer_client.cc

@@ -106,7 +106,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

atom/renderer/web_worker_observer.cc

@@ -49,7 +49,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());

patches/node/.patches

@@ -53,3 +53,4 @@ fix_extern_the_nativemoduleenv_and_options_parser_for_debug_builds.patch
 fix_ensure_js2c_maps_internal-fs_streams.patch
 chore_read_nobrowserglobals_from_global_not_process.patch
 chore_use_v8_inspector_js_protocol_to_find_pdl_file.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 <[email protected]>
+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 e313460a13ced7e7a9982db6f4a988699ec56111..faeba6d4e687261df1a58ad3c1e14d53b0e6d832 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);
+@@ -286,9 +287,16 @@ Environment* CreateEnvironment(IsolateData* isolate_data,
+                                       Environment::kOwnsInspector));
+   env->InitializeLibuv(per_process::v8_is_profiling);
+   env->ProcessCliArgs(args, exec_args);
+-  if (RunBootstrapping(env).IsEmpty()) {
++  if (bootstrap && !BootstrapEnvironment(env)) {
+     return nullptr;
+   }
++  return env;
++}
++
++bool BootstrapEnvironment(Environment* env) {
++  if (RunBootstrapping(env).IsEmpty()) {
++    return false;
++  }
+ 
+   std::vector<Local<String>> parameters = {
+       env->require_string(),
+@@ -301,9 +309,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);