From edd5c4b9bbac3e612627407c057ca25170b7cf9d Mon Sep 17 00:00:00 2001 From: Robo Date: Wed, 29 Aug 2018 00:02:46 +0530 Subject: [PATCH] fix: use OS process handle to clear object registry (#14324) RenderProcessHost switch can happen between ipc calls when speculative process are invvolved, which will lead to deletion of entries on current context. Use OS process handles to uniquely associate a destruction handler for a render process. --- atom/browser/api/atom_api_web_contents.cc | 3 ++- atom/renderer/renderer_client_base.cc | 5 +++-- lib/browser/objects-registry.js | 7 ++++--- spec/fixtures/api/render-view-deleted.html | 2 +- 4 files changed, 10 insertions(+), 7 deletions(-) diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc index 21e21ce7756..4e50833f3bd 100644 --- a/atom/browser/api/atom_api_web_contents.cc +++ b/atom/browser/api/atom_api_web_contents.cc @@ -768,7 +768,8 @@ 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()); + Emit("render-view-deleted", render_view_host->GetProcess()->GetID(), + base::GetProcId(render_view_host->GetProcess()->GetHandle())); } void WebContents::RenderProcessGone(base::TerminationStatus status) { diff --git a/atom/renderer/renderer_client_base.cc b/atom/renderer/renderer_client_base.cc index 69490420ef5..19623f0c8d4 100644 --- a/atom/renderer/renderer_client_base.cc +++ b/atom/renderer/renderer_client_base.cc @@ -16,7 +16,7 @@ #include "atom/renderer/content_settings_observer.h" #include "atom/renderer/preferences_manager.h" #include "base/command_line.h" -#include "base/process/process_handle.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" @@ -97,7 +97,8 @@ void RendererClientBase::DidCreateScriptContext( content::RenderFrame* render_frame) { // global.setHidden("contextId", `${processId}-${++next_context_id_}`) std::string context_id = base::StringPrintf( - "%" CrPRIdPid "-%d", base::GetCurrentProcId(), ++next_context_id_); + "%" CrPRIdPid "-%d", base::GetProcId(base::Process::Current().Handle()), + ++next_context_id_); v8::Isolate* isolate = context->GetIsolate(); v8::Local key = mate::StringToSymbol(isolate, "contextId"); v8::Local private_key = v8::Private::ForApi(isolate, key); diff --git a/lib/browser/objects-registry.js b/lib/browser/objects-registry.js index f03a23d2f69..148216c293f 100644 --- a/lib/browser/objects-registry.js +++ b/lib/browser/objects-registry.js @@ -103,9 +103,10 @@ class ObjectsRegistry { // Private: Clear the storage when renderer process is destroyed. registerDeleteListener (webContents, contextId) { - const processId = webContents.getProcessId() - const listener = (event, deletedProcessId) => { - if (deletedProcessId === processId) { + // contextId => ${OSProcessId}-${contextCount} + const OSProcessId = contextId.split('-')[0] + const listener = (event, deletedProcessId, deletedOSProcessId) => { + if (deletedOSProcessId && deletedOSProcessId.toString() === OSProcessId) { 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 bfc281eb429..3e65344af57 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.getProcessId()) + contents.emit('render-view-deleted', {}, '', contents.getOSProcessId()) try { contents.getURL() ipcRenderer.send('error-message', 'No error thrown')