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

@@ -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) {

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<v8::Value> GetRenderProcessPreferences(
     return v8::Null(isolate);
 }
 
-std::vector<std::string> ParseSchemesCLISwitch(const char* switch_name) {
-  base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+std::vector<std::string> 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<std::string> ParseSchemesCLISwitch(const char* switch_name) {
 }  // namespace
 
 RendererClientBase::RendererClientBase() {
+  auto* command_line = base::CommandLine::ForCurrentProcess();
   // Parse --standard-schemes=scheme1,scheme2
   std::vector<std::string> 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<v8::Context> 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<v8::String> key = mate::StringToSymbol(isolate, "contextId");
   v8::Local<v8::Private> 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<std::string> 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()) {

atom/renderer/renderer_client_base.h

@@ -56,9 +56,9 @@ class RendererClientBase : public content::ContentRendererClient {
   std::unique_ptr<PreferencesManager> 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

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)
       }

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')