fix: use OS process handle to clear object registry (electron#14364)

trop[bot] · codebytere · commit ccf8a797dc77 · 2018-08-28T22:55:42.000-07:00
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.
diff --git a/atom/browser/api/atom_api_web_contents.cc b/atom/browser/api/atom_api_web_contents.cc
@@ -766,7 +766,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
@@ -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"
@@ -94,7 +94,8 @@ void RendererClientBase::DidCreateScriptContext(
     content::RenderFrame* render_frame) {
   // global.setHidden("contextId", `${processId}-${++nextContextId}`)
   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<v8::String> key = mate::StringToSymbol(isolate, "contextId");
   v8::Local<v8::Private> private_key = v8::Private::ForApi(isolate, key);
diff --git a/lib/browser/objects-registry.js b/lib/browser/objects-registry.js
@@ -98,9 +98,10 @@ class ObjectsRegistry {
 
   // Private: Clear the storage when renderer process is destoryed.
   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
@@ -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')

Original file line number	Diff line number	Diff line change
`@@ -766,7 +766,8 @@ void WebContents::RenderViewCreated(content::RenderViewHost* render_view_host) {`
`766`	`766`	`}`
`767`	`767`
`768`	`768`	`void WebContents::RenderViewDeleted(content::RenderViewHost* render_view_host) {`
`769`		`- Emit("render-view-deleted", render_view_host->GetProcess()->GetID());`
	`769`	`+ Emit("render-view-deleted", render_view_host->GetProcess()->GetID(),`
	`770`	`+ base::GetProcId(render_view_host->GetProcess()->GetHandle()));`
`770`	`771`	`}`
`771`	`772`
`772`	`773`	`void WebContents::RenderProcessGone(base::TerminationStatus status) {`
Original file line number	Diff line number	Diff line change
`@@ -17,7 +17,7 @@`
`17`	`17`	`}`
`18`	`18`
`19`	`19`	`// This should trigger a dereference and a remote getURL call should fail`
`20`		`- contents.emit('render-view-deleted', {}, contents.getProcessId())`
	`20`	`+ contents.emit('render-view-deleted', {}, '', contents.getOSProcessId())`
`21`	`21`	`try {`
`22`	`22`	`contents.getURL()`
`23`	`23`	`ipcRenderer.send('error-message', 'No error thrown')`