From 33a9d898a6a3ddfa110d43a9b07205e07383e853 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Mon, 5 Aug 2019 12:50:51 -0700 Subject: [PATCH] fix: make child windows not crash when ipc messages are received (#19553) --- lib/browser/guest-window-manager.js | 2 +- shell/common/options_switches.cc | 1 + shell/common/options_switches.h | 1 + shell/renderer/atom_render_frame_observer.cc | 17 +++++++++++---- shell/renderer/atom_renderer_client.cc | 23 +++++++++++++++----- shell/renderer/electron_api_service_impl.cc | 7 +++++- 6 files changed, 39 insertions(+), 12 deletions(-) diff --git a/lib/browser/guest-window-manager.js b/lib/browser/guest-window-manager.js index 35693e837d29..c223b55f8ba0 100644 --- a/lib/browser/guest-window-manager.js +++ b/lib/browser/guest-window-manager.js @@ -34,7 +34,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 diff --git a/shell/common/options_switches.cc b/shell/common/options_switches.cc index 912e73e41cd2..5fe2f4aec174 100644 --- a/shell/common/options_switches.cc +++ b/shell/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"; diff --git a/shell/common/options_switches.h b/shell/common/options_switches.h index 30881e95a2e2..beb47746f62f 100644 --- a/shell/common/options_switches.h +++ b/shell/common/options_switches.h @@ -118,6 +118,7 @@ extern const char kNodeIntegrationInWorker[]; extern const char kWebviewTag[]; extern const char kNodeIntegrationInSubFrames[]; extern const char kDisableElectronSiteInstanceOverrides[]; +extern const char kEnableNodeLeakageInRenderers[]; extern const char kWidevineCdmPath[]; extern const char kWidevineCdmVersion[]; diff --git a/shell/renderer/atom_render_frame_observer.cc b/shell/renderer/atom_render_frame_observer.cc index b0e06d17c7d9..2f32b9fda111 100644 --- a/shell/renderer/atom_render_frame_observer.cc +++ b/shell/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(); diff --git a/shell/renderer/atom_renderer_client.cc b/shell/renderer/atom_renderer_client.cc index ffe535795289..4a3b5069bdeb 100644 --- a/shell/renderer/atom_renderer_client.cc +++ b/shell/renderer/atom_renderer_client.cc @@ -77,14 +77,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); @@ -112,7 +124,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)) diff --git a/shell/renderer/electron_api_service_impl.cc b/shell/renderer/electron_api_service_impl.cc index 599b6ae041a7..14a38f445bbd 100644 --- a/shell/renderer/electron_api_service_impl.cc +++ b/shell/renderer/electron_api_service_impl.cc @@ -39,7 +39,10 @@ v8::Local GetIpcObject(v8::Local 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(); + } return value->ToObject(context).ToLocalChecked(); } @@ -50,6 +53,8 @@ void InvokeIpcCallback(v8::Local 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.