fix: make child windows not crash when ipc messages are received (#19633)

* fix: make child windows not crash when ipc messages are received

This also adds a path forward for apps using child windows with
nodeIntegration to migrate into a non-leaky way of doing it.

1. Ensure that if ipcNative is missing we don't crash rather log that it
is missing
2. Add a hidden option `--enable-node-leakage-in-renderers` (temporary
measure) to allow app devs to opt in to leaking node in child windows
3. Bypasses the Opener() check if renderer process reuse is enabled
(which would prevent the leak anyway)

So the path forward is: it no longer crashes --> folks use the hidden
option --> folks opt in to renderer process reuse.

* Apply suggestions from code review

Co-Authored-By: Jeremy Apthorp <jeremya@chromium.org>

* Update shell/renderer/atom_renderer_client.cc

Co-Authored-By: Jeremy Apthorp <jeremya@chromium.org>

atom/common/options_switches.cc

@@ -234,6 +234,7 @@ const char kNativeWindowOpen[] = "native-window-open";
 const char kWebviewTag[] = "webview-tag";
 const char kDisableElectronSiteInstanceOverrides[] =
     "disable-electron-site-instance-overrides";
+const char kEnableNodeLeakageInRenderers[] = "enable-node-leakage-in-renderers";
 
 // Command switch passed to renderer process to control nodeIntegration.
 const char kNodeIntegrationInWorker[] = "node-integration-in-worker";

atom/common/options_switches.h

@@ -119,6 +119,7 @@ extern const char kWebviewTag[];
 extern const char kNodeIntegrationInSubFrames[];
 extern const char kDisableHtmlFullscreenWindowResize[];
 extern const char kDisableElectronSiteInstanceOverrides[];
+extern const char kEnableNodeLeakageInRenderers[];
 
 extern const char kWidevineCdmPath[];
 extern const char kWidevineCdmVersion[];

atom/renderer/atom_render_frame_observer.cc

@@ -69,16 +69,25 @@ void AtomRenderFrameObserver::DidCreateScriptContext(
   if (ShouldNotifyClient(world_id))
     renderer_client_->DidCreateScriptContext(context, render_frame_);
 
+  auto* command_line = base::CommandLine::ForCurrentProcess();
+
   bool use_context_isolation = renderer_client_->isolated_world();
+  // This logic matches the EXPLAINED logic in atom_renderer_client.cc
+  // to avoid explaining it twice go check that implementation in
+  // DidCreateScriptContext();
   bool is_main_world = IsMainWorld(world_id);
   bool is_main_frame = render_frame_->IsMainFrame();
-  bool is_not_opened = !render_frame_->GetWebFrame()->Opener();
+  bool reuse_renderer_processes_enabled =
+      command_line->HasSwitch(switches::kDisableElectronSiteInstanceOverrides);
+  bool is_not_opened =
+      !render_frame_->GetWebFrame()->Opener() ||
+      command_line->HasSwitch(switches::kEnableNodeLeakageInRenderers);
   bool allow_node_in_sub_frames =
-      base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kNodeIntegrationInSubFrames);
+      command_line->HasSwitch(switches::kNodeIntegrationInSubFrames);
   bool should_create_isolated_context =
       use_context_isolation && is_main_world &&
-      (is_main_frame || allow_node_in_sub_frames) && is_not_opened;
+      (is_main_frame || allow_node_in_sub_frames) &&
+      (is_not_opened || reuse_renderer_processes_enabled);
 
   if (should_create_isolated_context) {
     CreateIsolatedWorldContext();

atom/renderer/atom_renderer_client.cc

@@ -73,14 +73,26 @@ void AtomRendererClient::DidCreateScriptContext(
   // TODO(zcbenz): Do not create Node environment if node integration is not
   // enabled.
 
-  // Do not load node if we're aren't a main frame or a devtools extension
+  auto* command_line = base::CommandLine::ForCurrentProcess();
+
+  // Only load node if we are a main frame or a devtools extension
   // unless node support has been explicitly enabled for sub frames
-  bool is_main_frame =
-      render_frame->IsMainFrame() && !render_frame->GetWebFrame()->Opener();
+  bool reuse_renderer_processes_enabled =
+      command_line->HasSwitch(switches::kDisableElectronSiteInstanceOverrides);
+  // Consider the window not "opened" if it does not have an Opener, or if a
+  // user has manually opted in to leaking node in the renderer
+  bool is_not_opened =
+      !render_frame->GetWebFrame()->Opener() ||
+      command_line->HasSwitch(switches::kEnableNodeLeakageInRenderers);
+  // Consider this the main frame if it is both a Main Frame and it wasn't
+  // opened.  We allow an opened main frame to have node if renderer process
+  // reuse is enabled as that will correctly free node environments prevent a
+  // leak in child windows.
+  bool is_main_frame = render_frame->IsMainFrame() &&
+                       (is_not_opened || reuse_renderer_processes_enabled);
   bool is_devtools = IsDevToolsExtension(render_frame);
   bool allow_node_in_subframes =
-      base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kNodeIntegrationInSubFrames);
+      command_line->HasSwitch(switches::kNodeIntegrationInSubFrames);
   bool should_load_node =
       (is_main_frame || is_devtools || allow_node_in_subframes) &&
       !IsWebViewFrame(renderer_context, render_frame);
@@ -108,7 +120,6 @@ void AtomRendererClient::DidCreateScriptContext(
   DCHECK(!context.IsEmpty());
   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
   if (command_line->HasSwitch(switches::kDisableElectronSiteInstanceOverrides))

atom/renderer/electron_api_service_impl.cc

@@ -39,7 +39,10 @@ v8::Local<v8::Object> GetIpcObject(v8::Local<v8::Context> context) {
   auto global_object = context->Global();
   auto value =
       global_object->GetPrivate(context, private_binding_key).ToLocalChecked();
-  DCHECK(!value.IsEmpty() && value->IsObject());
+  if (value.IsEmpty() || !value->IsObject()) {
+    LOG(ERROR) << "Attempted to get the 'ipcNative' object but it was missing";
+    return v8::Local<v8::Object>();
+  }
   return value->ToObject(context).ToLocalChecked();
 }
 
@@ -50,6 +53,8 @@ void InvokeIpcCallback(v8::Local<v8::Context> context,
   auto* isolate = context->GetIsolate();
 
   auto ipcNative = GetIpcObject(context);
+  if (ipcNative.IsEmpty())
+    return;
 
   // Only set up the node::CallbackScope if there's a node environment.
   // Sandboxed renderers don't have a node environment.

lib/browser/guest-window-manager.js

@@ -33,7 +33,7 @@ const mergeOptions = function (child, parent, visited) {
     if (key in child && key !== 'webPreferences') continue
 
     const value = parent[key]
-    if (typeof value === 'object') {
+    if (typeof value === 'object' && !Array.isArray(value)) {
       child[key] = mergeOptions(child[key] || {}, value, visited)
     } else {
       child[key] = value