From 14ed71fa1b9c6c26c681d66e94415e739365351d Mon Sep 17 00:00:00 2001 From: Robo Date: Tue, 11 Sep 2018 23:48:10 +0530 Subject: [PATCH] fix: use render client id to track deleted render process hosts (#14520) * fix: use render client id to track deleted render process hosts Instead of relying on OS process id, which may not be unique when a process is reused, we rely on the renderer client id passed by the content layer when starting the renderer process which is guaranteed to be unique for the lifetime of the app. * fix: store context id as int64_t Ensuring that it doesn't wrap easily with a large number of context creation on some malformed web pages. --- atom/browser/api/atom_api_web_contents.cc | 4 +-- atom/renderer/renderer_client_base.cc | 36 +++++++++++----------- atom/renderer/renderer_client_base.h | 4 +-- lib/browser/objects-registry.js | 12 +++----- spec/fixtures/api/render-view-deleted.html | 2 +- 5 files changed, 27 insertions(+), 31 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 4233cf7f2738..7e341522ab72 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -56,7 +56,6 @@ #include "atom/common/native_mate_converters/value_converter.h" #include "atom/common/options_switches.h" #include "base/message_loop/message_loop.h" -#include "base/process/process_handle.h" #include "base/strings/utf_string_conversions.h" #include "base/threading/thread_task_runner_handle.h" #include "base/values.h" @@ -777,8 +776,7 @@ void WebContents::RenderViewCreated(content::RenderViewHost* render_view_host) { } void WebContents::RenderViewDeleted(content::RenderViewHost* render_view_host) { - Emit("render-view-deleted", render_view_host->GetProcess()->GetID(), - base::GetProcId(render_view_host->GetProcess()->GetHandle())); + Emit("render-view-deleted", render_view_host->GetProcess()->GetID()); } void WebContents::RenderProcessGone(base::TerminationStatus status) { diff --git a/atom/renderer/renderer_client_base.cc b/atom/renderer/renderer_client_base.cc index 1b434d598deb..9c68469c45d4 100644 --- a/atom/renderer/renderer_client_base.cc +++ b/atom/renderer/renderer_client_base.cc @@ -16,13 +16,13 @@ #include "atom/renderer/content_settings_observer.h" #include "atom/renderer/preferences_manager.h" #include "base/command_line.h" -#include "base/process/process.h" #include "base/strings/string_split.h" #include "base/strings/stringprintf.h" #include "chrome/renderer/media/chrome_key_systems.h" #include "chrome/renderer/printing/print_web_view_helper.h" #include "chrome/renderer/tts_dispatcher.h" #include "content/public/common/content_constants.h" +#include "content/public/common/content_switches.h" #include "content/public/renderer/render_frame.h" #include "content/public/renderer/render_view.h" #include "native_mate/dictionary.h" @@ -50,14 +50,6 @@ #include "chrome/renderer/pepper/pepper_helper.h" #endif // defined(ENABLE_PEPPER_FLASH) -// This is defined in later versions of Chromium, remove this if you see -// compiler complaining duplicate defines. -#if defined(OS_WIN) || defined(OS_FUCHSIA) -#define CrPRIdPid "ld" -#else -#define CrPRIdPid "d" -#endif - namespace atom { namespace { @@ -71,8 +63,8 @@ v8::Local GetRenderProcessPreferences( return v8::Null(isolate); } -std::vector ParseSchemesCLISwitch(const char* switch_name) { - base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); +std::vector ParseSchemesCLISwitch(base::CommandLine* command_line, + const char* switch_name) { std::string custom_schemes = command_line->GetSwitchValueASCII(switch_name); return base::SplitString(custom_schemes, ",", base::TRIM_WHITESPACE, base::SPLIT_WANT_NONEMPTY); @@ -81,13 +73,21 @@ std::vector ParseSchemesCLISwitch(const char* switch_name) { } // namespace RendererClientBase::RendererClientBase() { + auto* command_line = base::CommandLine::ForCurrentProcess(); // Parse --standard-schemes=scheme1,scheme2 std::vector standard_schemes_list = - ParseSchemesCLISwitch(switches::kStandardSchemes); + ParseSchemesCLISwitch(command_line, switches::kStandardSchemes); for (const std::string& scheme : standard_schemes_list) url::AddStandardScheme(scheme.c_str(), url::SCHEME_WITHOUT_PORT); isolated_world_ = base::CommandLine::ForCurrentProcess()->HasSwitch( switches::kContextIsolation); + // We rely on the unique process host id which is notified to the + // renderer process via command line switch from the content layer, + // if this switch is removed from the content layer for some reason, + // we should define our own. + DCHECK(command_line->HasSwitch(::switches::kRendererClientId)); + renderer_client_id_ = + command_line->GetSwitchValueASCII(::switches::kRendererClientId); } RendererClientBase::~RendererClientBase() {} @@ -95,10 +95,9 @@ RendererClientBase::~RendererClientBase() {} void RendererClientBase::DidCreateScriptContext( v8::Handle context, content::RenderFrame* render_frame) { - // global.setHidden("contextId", `${processId}-${++next_context_id_}`) - std::string context_id = base::StringPrintf( - "%" CrPRIdPid "-%d", base::GetProcId(base::Process::Current().Handle()), - ++next_context_id_); + // global.setHidden("contextId", `${processHostId}-${++next_context_id_}`) + auto context_id = base::StringPrintf( + "%s-%" PRId64, renderer_client_id_.c_str(), ++next_context_id_); v8::Isolate* isolate = context->GetIsolate(); v8::Local key = mate::StringToSymbol(isolate, "contextId"); v8::Local private_key = v8::Private::ForApi(isolate, key); @@ -116,6 +115,8 @@ void RendererClientBase::AddRenderBindings( } void RendererClientBase::RenderThreadStarted() { + auto* command_line = base::CommandLine::ForCurrentProcess(); + blink::WebCustomElement::AddEmbedderCustomElementName("webview"); blink::WebCustomElement::AddEmbedderCustomElementName("browserplugin"); @@ -137,7 +138,7 @@ void RendererClientBase::RenderThreadStarted() { // Parse --secure-schemes=scheme1,scheme2 std::vector secure_schemes_list = - ParseSchemesCLISwitch(switches::kSecureSchemes); + ParseSchemesCLISwitch(command_line, switches::kSecureSchemes); for (const std::string& scheme : secure_schemes_list) blink::SchemeRegistry::RegisterURLSchemeAsSecure( WTF::String::FromUTF8(scheme.data(), scheme.length())); @@ -151,7 +152,6 @@ void RendererClientBase::RenderThreadStarted() { #if defined(OS_WIN) // Set ApplicationUserModelID in renderer process. - base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); base::string16 app_id = command_line->GetSwitchValueNative(switches::kAppUserModelId); if (!app_id.empty()) { diff --git a/atom/renderer/renderer_client_base.h b/atom/renderer/renderer_client_base.h index ffad57a6f144..2b495d613a7f 100644 --- a/atom/renderer/renderer_client_base.h +++ b/atom/renderer/renderer_client_base.h @@ -56,9 +56,9 @@ class RendererClientBase : public content::ContentRendererClient { std::unique_ptr preferences_manager_; ChromeKeySystemsProvider key_systems_provider_; bool isolated_world_; - + std::string renderer_client_id_; // An increasing ID used for indentifying an V8 context in this process. - int next_context_id_ = 0; + int64_t next_context_id_ = 0; }; } // namespace atom diff --git a/lib/browser/objects-registry.js b/lib/browser/objects-registry.js index 148216c293f8..8209605fe572 100644 --- a/lib/browser/objects-registry.js +++ b/lib/browser/objects-registry.js @@ -87,9 +87,6 @@ class ObjectsRegistry { // Private: Dereference the object from store. dereference (id) { - // FIXME(MarshallOfSound): We should remove this once remote deref works well - if (process.env.ELECTRON_DISABLE_REMOTE_DEREFERENCING) return - let pointer = this.storage[id] if (pointer == null) { return @@ -103,10 +100,11 @@ class ObjectsRegistry { // Private: Clear the storage when renderer process is destroyed. registerDeleteListener (webContents, contextId) { - // contextId => ${OSProcessId}-${contextCount} - const OSProcessId = contextId.split('-')[0] - const listener = (event, deletedProcessId, deletedOSProcessId) => { - if (deletedOSProcessId && deletedOSProcessId.toString() === OSProcessId) { + // contextId => ${processHostId}-${contextCount} + const processHostId = contextId.split('-')[0] + const listener = (event, deletedProcessHostId) => { + if (deletedProcessHostId && + deletedProcessHostId.toString() === processHostId) { webContents.removeListener('render-view-deleted', listener) this.clear(webContents, contextId) } diff --git a/spec/fixtures/api/render-view-deleted.html b/spec/fixtures/api/render-view-deleted.html index 3e65344af57b..bfc281eb4298 100644 --- a/spec/fixtures/api/render-view-deleted.html +++ b/spec/fixtures/api/render-view-deleted.html @@ -17,7 +17,7 @@ } // This should trigger a dereference and a remote getURL call should fail - contents.emit('render-view-deleted', {}, '', contents.getOSProcessId()) + contents.emit('render-view-deleted', {}, contents.getProcessId()) try { contents.getURL() ipcRenderer.send('error-message', 'No error thrown')