fix: use webContentsId with contextId together (#13749)

After after using `processId-contextCounter` as contextId, it may happen
that contexts in different WebContents sharing the same renderer process
get the same contextId. Using webContentsId as part of key in
ObjectsRegistry can fix this.

atom/common/context_counter.cc

@@ -1,19 +0,0 @@
-// Copyright (c) 2018 GitHub, Inc.
-// Use of this source code is governed by the MIT license that can be
-// found in the LICENSE file.
-
-#include "atom/common/context_counter.h"
-
-namespace atom {
-
-namespace {
-
-int g_context_id = 0;
-
-}  // namespace
-
-int GetNextContextId() {
-  return ++g_context_id;
-}
-
-}  // namespace atom

atom/common/context_counter.h

@@ -1,15 +0,0 @@
-// Copyright (c) 2018 GitHub, Inc.
-// Use of this source code is governed by the MIT license that can be
-// found in the LICENSE file.
-
-#ifndef ATOM_COMMON_CONTEXT_COUNTER_H_
-#define ATOM_COMMON_CONTEXT_COUNTER_H_
-
-namespace atom {
-
-// Increase the context counter, and return current count.
-int GetNextContextId();
-
-}  // namespace atom
-
-#endif  // ATOM_COMMON_CONTEXT_COUNTER_H_

atom/renderer/renderer_client_base.cc

@@ -8,7 +8,6 @@
 #include <vector>
 
 #include "atom/common/color_util.h"
-#include "atom/common/context_counter.h"
 #include "atom/common/native_mate_converters/value_converter.h"
 #include "atom/common/options_switches.h"
 #include "atom/renderer/atom_autofill_agent.h"
@@ -97,9 +96,9 @@ RendererClientBase::~RendererClientBase() {}
 void RendererClientBase::DidCreateScriptContext(
     v8::Handle<v8::Context> context,
     content::RenderFrame* render_frame) {
-  // global.setHidden("contextId", `${processId}-${GetNextContextId()}`)
+  // global.setHidden("contextId", `${processId}-${++next_context_id_}`)
   std::string context_id = base::StringPrintf(
-      "%" CrPRIdPid "-%d", base::GetCurrentProcId(), GetNextContextId());
+      "%" CrPRIdPid "-%d", base::GetCurrentProcId(), ++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);

atom/renderer/renderer_client_base.h

@@ -57,6 +57,9 @@ class RendererClientBase : public content::ContentRendererClient {
  private:
   std::unique_ptr<PreferencesManager> preferences_manager_;
   bool isolated_world_;
+
+  // An increasing ID used for indentifying an V8 context in this process.
+  int next_context_id_ = 0;
 };
 
 }  // namespace atom

filenames.gypi

@@ -467,8 +467,6 @@
       'atom/common/color_util.h',
       'atom/common/common_message_generator.cc',
       'atom/common/common_message_generator.h',
-      'atom/common/context_counter.cc',
-      'atom/common/context_counter.h',
       'atom/common/crash_reporter/crash_reporter.cc',
       'atom/common/crash_reporter/crash_reporter.h',
       'atom/common/crash_reporter/crash_reporter_linux.cc',

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.
-    // (webContentsId) => [id]
+    // (ownerKey) => [id]
     this.owners = {}
   }
 
@@ -22,9 +26,10 @@ class ObjectsRegistry {
     const id = this.saveToStorage(obj)
 
     // Add object to the set of referenced objects.
-    let owner = this.owners[contextId]
+    const ownerKey = getOwnerKey(webContents, contextId)
+    let owner = this.owners[ownerKey]
     if (!owner) {
-      owner = this.owners[contextId] = new Set()
+      owner = this.owners[ownerKey] = new Set()
       this.registerDeleteListener(webContents, contextId)
     }
     if (!owner.has(id)) {
@@ -44,8 +49,9 @@ class ObjectsRegistry {
   // Dereference an object according to its ID.
   // Note that an object may be double-freed (cleared when page is reloaded, and
   // then garbage collected in old page).
-  remove (contextId, id) {
-    let owner = this.owners[contextId]
+  remove (webContents, contextId, id) {
+    const ownerKey = getOwnerKey(webContents, contextId)
+    let owner = this.owners[ownerKey]
     if (owner) {
       // Remove the reference in owner.
       owner.delete(id)
@@ -55,13 +61,14 @@ class ObjectsRegistry {
   }
 
   // Clear all references to objects refrenced by the WebContents.
-  clear (contextId) {
-    let owner = this.owners[contextId]
+  clear (webContents, contextId) {
+    const ownerKey = getOwnerKey(webContents, contextId)
+    let owner = this.owners[ownerKey]
     if (!owner) return
 
     for (let id of owner) this.dereference(id)
 
-    delete this.owners[contextId]
+    delete this.owners[ownerKey]
   }
 
   // Private: Saves the object into storage and assigns an ID for it.
@@ -91,13 +98,13 @@ class ObjectsRegistry {
     }
   }
 
-  // Private: Clear the storage when webContents is reloaded/navigated.
+  // Private: Clear the storage when renderer process is destroyed.
   registerDeleteListener (webContents, contextId) {
     const processId = webContents.getProcessId()
     const listener = (event, deletedProcessId) => {
       if (deletedProcessId === processId) {
         webContents.removeListener('render-view-deleted', listener)
-        this.clear(contextId)
+        this.clear(webContents, contextId)
       }
     }
     webContents.on('render-view-deleted', listener)

lib/browser/rpc-server.js

@@ -384,12 +384,12 @@ ipcMain.on('ELECTRON_BROWSER_MEMBER_GET', function (event, contextId, id, name)
 })
 
 ipcMain.on('ELECTRON_BROWSER_DEREFERENCE', function (event, contextId, id) {
-  objectsRegistry.remove(contextId, id)
+  objectsRegistry.remove(event.sender, contextId, id)
 })
 
-ipcMain.on('ELECTRON_BROWSER_CONTEXT_RELEASE', (e, contextId) => {
-  objectsRegistry.clear(contextId)
-  e.returnValue = null
+ipcMain.on('ELECTRON_BROWSER_CONTEXT_RELEASE', (event, contextId) => {
+  objectsRegistry.clear(event.sender, contextId)
+  event.returnValue = null
 })
 
 ipcMain.on('ELECTRON_BROWSER_GUEST_WEB_CONTENTS', function (event, contextId, guestInstanceId) {