fix: use webContentsId with contextId together

zcbenz · zcbenz · commit b60125f782bd · 2018-07-20T10:53:16.000+09:00
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.
diff --git a/atom/common/context_counter.cc b/atom/common/context_counter.cc
diff --git a/atom/common/context_counter.h b/atom/common/context_counter.h
diff --git a/atom/renderer/renderer_client_base.cc b/atom/renderer/renderer_client_base.cc
@@ -9,7 +9,6 @@
 
 #include "atom/common/atom_constants.h"
 #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"
@@ -92,9 +91,9 @@ RendererClientBase::~RendererClientBase() {
 void RendererClientBase::DidCreateScriptContext(
     v8::Handle<v8::Context> context,
     content::RenderFrame* render_frame) {
-  // global.setHidden("contextId", `${processId}-${GetNextContextId()}`)
+  // global.setHidden("contextId", `${processId}-${++nextContextId}`)
   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);
diff --git a/atom/renderer/renderer_client_base.h b/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
diff --git a/filenames.gypi b/filenames.gypi
@@ -437,8 +437,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',
diff --git a/lib/browser/objects-registry.js b/lib/browser/objects-registry.js
@@ -11,7 +11,7 @@ class ObjectsRegistry {
     this.storage = {}
 
     // Stores the IDs of objects referenced by WebContents.
-    // (webContentsId) => [id]
+    // (webContentsContextId) => [id]
     this.owners = {}
   }
 
@@ -22,9 +22,10 @@ class ObjectsRegistry {
     const id = this.saveToStorage(obj)
 
     // Add object to the set of referenced objects.
-    let owner = this.owners[contextId]
+    const webContentsContextId = `${webContents.id}-${contextId}`
+    let owner = this.owners[webContentsContextId]
     if (!owner) {
-      owner = this.owners[contextId] = new Set()
+      owner = this.owners[webContentsContextId] = new Set()
       this.registerDeleteListener(webContents, contextId)
     }
     if (!owner.has(id)) {
@@ -44,8 +45,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 webContentsContextId = `${webContents.id}-${contextId}`
+    let owner = this.owners[webContentsContextId]
     if (owner) {
       // Remove the reference in owner.
       owner.delete(id)
@@ -55,13 +57,14 @@ class ObjectsRegistry {
   }
 
   // Clear all references to objects refrenced by the WebContents.
-  clear (contextId) {
-    let owner = this.owners[contextId]
+  clear (webContents, contextId) {
+    const webContentsContextId = `${webContents.id}-${contextId}`
+    let owner = this.owners[webContentsContextId]
     if (!owner) return
 
     for (let id of owner) this.dereference(id)
 
-    delete this.owners[contextId]
+    delete this.owners[webContentsContextId]
   }
 
   // Private: Saves the object into storage and assigns an ID for it.
@@ -91,13 +94,13 @@ class ObjectsRegistry {
     }
   }
 
-  // Private: Clear the storage when webContents is reloaded/navigated.
+  // Private: Clear the storage when renderer process is destoryed.
   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)
diff --git a/lib/browser/rpc-server.js b/lib/browser/rpc-server.js
@@ -391,12 +391,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) {
