fix: use render client id to track deleted render process hosts (backport: 3-0-x) (#14557)

* fix: use render client id to track deleted render process hosts

* fix: use webContentsId with contextId together (#13749)

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"
@@ -770,8 +769,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/browser/atom_browser_client.cc

@@ -345,10 +345,6 @@ void AtomBrowserClient::AppendExtraCommandLineSwitches(
       web_preferences->AppendCommandLineSwitches(command_line);
     SessionPreferences::AppendExtraCommandLineSwitches(
         web_contents->GetBrowserContext(), command_line);
-
-    auto context_id = atom::api::WebContents::GetIDForContents(web_contents);
-    command_line->AppendSwitchASCII(switches::kContextId,
-                                    base::IntToString(context_id));
   }
 }
 

atom/common/options_switches.cc

@@ -189,9 +189,6 @@ const char kAppUserModelId[] = "app-user-model-id";
 // The application path
 const char kAppPath[] = "app-path";
 
-// The context ID for this process
-const char kContextId[] = "context-id";
-
 // The command line switch versions of the options.
 const char kBackgroundColor[] = "background-color";
 const char kPreloadScript[] = "preload";

atom/common/options_switches.h

@@ -93,7 +93,6 @@ extern const char kRegisterServiceWorkerSchemes[];
 extern const char kSecureSchemes[];
 extern const char kAppUserModelId[];
 extern const char kAppPath[];
-extern const char kContextId[];
 
 extern const char kBackgroundColor[];
 extern const char kPreloadScript[];

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/pepper/pepper_helper.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"
@@ -46,14 +46,6 @@
 #include "atom/common/atom_constants.h"
 #endif  // defined(ENABLE_PDF_VIEWER)
 
-// 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 {
@@ -67,8 +59,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);
@@ -78,12 +70,20 @@ std::vector<std::string> ParseSchemesCLISwitch(const char* switch_name) {
 
 RendererClientBase::RendererClientBase() {
   // Parse --standard-schemes=scheme1,scheme2
+  auto* command_line = base::CommandLine::ForCurrentProcess();
   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() {}
@@ -91,10 +91,9 @@ RendererClientBase::~RendererClientBase() {}
 void RendererClientBase::DidCreateScriptContext(
     v8::Handle<v8::Context> context,
     content::RenderFrame* render_frame) {
-  // global.setHidden("contextId", `${processId}-${++nextContextId}`)
-  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);
@@ -112,6 +111,8 @@ void RendererClientBase::AddRenderBindings(
 }
 
 void RendererClientBase::RenderThreadStarted() {
+  auto* command_line = base::CommandLine::ForCurrentProcess();
+
   blink::WebCustomElement::AddEmbedderCustomElementName("webview");
   blink::WebCustomElement::AddEmbedderCustomElementName("browserplugin");
 
@@ -133,7 +134,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()));
@@ -147,7 +148,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

@@ -58,8 +58,9 @@ class RendererClientBase : public content::ContentRendererClient {
   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

@@ -2,6 +2,10 @@
 
 const v8Util = process.atomBinding('v8_util')
 
+const getOwnerKey = (webContents, contextId) => {
+  return `${webContents.id}-${contextId}`
+}
+
 class ObjectsRegistry {
   constructor () {
     this.nextId = 0
@@ -11,7 +15,7 @@ class ObjectsRegistry {
     this.storage = {}
 
     // Stores the IDs of objects referenced by WebContents.
-    // (webContentsContextId) => [id]
+    // (ownerKey) => [id]
     this.owners = {}
   }
 
@@ -22,10 +26,10 @@ class ObjectsRegistry {
     const id = this.saveToStorage(obj)
 
     // Add object to the set of referenced objects.
-    const webContentsContextId = `${webContents.id}-${contextId}`
-    let owner = this.owners[webContentsContextId]
+    const ownerKey = getOwnerKey(webContents, contextId)
+    let owner = this.owners[ownerKey]
     if (!owner) {
-      owner = this.owners[webContentsContextId] = new Set()
+      owner = this.owners[ownerKey] = new Set()
       this.registerDeleteListener(webContents, contextId)
     }
     if (!owner.has(id)) {
@@ -46,8 +50,8 @@ class ObjectsRegistry {
   // Note that an object may be double-freed (cleared when page is reloaded, and
   // then garbage collected in old page).
   remove (webContents, contextId, id) {
-    const webContentsContextId = `${webContents.id}-${contextId}`
-    let owner = this.owners[webContentsContextId]
+    const ownerKey = getOwnerKey(webContents, contextId)
+    let owner = this.owners[ownerKey]
     if (owner) {
       // Remove the reference in owner.
       owner.delete(id)
@@ -58,13 +62,13 @@ class ObjectsRegistry {
 
   // Clear all references to objects refrenced by the WebContents.
   clear (webContents, contextId) {
-    const webContentsContextId = `${webContents.id}-${contextId}`
-    let owner = this.owners[webContentsContextId]
+    const ownerKey = getOwnerKey(webContents, contextId)
+    let owner = this.owners[ownerKey]
     if (!owner) return
 
     for (let id of owner) this.dereference(id)
 
-    delete this.owners[webContentsContextId]
+    delete this.owners[ownerKey]
   }
 
   // Private: Saves the object into storage and assigns an ID for it.
@@ -83,8 +87,6 @@ class ObjectsRegistry {
 
   // Private: Dereference the object from store.
   dereference (id) {
-    if (process.env.ELECTRON_DISABLE_REMOTE_DEREFERENCING) return
-
     let pointer = this.storage[id]
     if (pointer == null) {
       return
@@ -96,12 +98,13 @@ class ObjectsRegistry {
     }
   }
 
-  // Private: Clear the storage when renderer process is destoryed.
+  // 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')