From 3094f62f0bad0842c48cd3cd8ce7f04f10f04884 Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Tue, 24 Jul 2018 16:21:38 +0900 Subject: [PATCH] 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 | 19 ------------------- atom/common/context_counter.h | 15 --------------- atom/renderer/renderer_client_base.cc | 5 ++--- atom/renderer/renderer_client_base.h | 3 +++ filenames.gypi | 2 -- lib/browser/objects-registry.js | 27 +++++++++++++++++---------- lib/browser/rpc-server.js | 8 ++++---- 7 files changed, 26 insertions(+), 53 deletions(-) delete mode 100644 atom/common/context_counter.cc delete mode 100644 atom/common/context_counter.h diff --git a/atom/common/context_counter.cc b/atom/common/context_counter.cc deleted file mode 100644 index 881a968a4d97..000000000000 --- a/atom/common/context_counter.cc +++ /dev/null @@ -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 diff --git a/atom/common/context_counter.h b/atom/common/context_counter.h deleted file mode 100644 index 4c21478ae664..000000000000 --- a/atom/common/context_counter.h +++ /dev/null @@ -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_ diff --git a/atom/renderer/renderer_client_base.cc b/atom/renderer/renderer_client_base.cc index 6d444778bc1f..3af38a0c8ae1 100644 --- a/atom/renderer/renderer_client_base.cc +++ b/atom/renderer/renderer_client_base.cc @@ -8,7 +8,6 @@ #include #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 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 key = mate::StringToSymbol(isolate, "contextId"); v8::Local private_key = v8::Private::ForApi(isolate, key); diff --git a/atom/renderer/renderer_client_base.h b/atom/renderer/renderer_client_base.h index c6f766ff939b..2b13f8b038cb 100644 --- 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 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 index 80bb4b7cac84..2215cb4f6d89 100644 --- a/filenames.gypi +++ b/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', diff --git a/lib/browser/objects-registry.js b/lib/browser/objects-registry.js index b2667454152c..92806500dcce 100644 --- a/lib/browser/objects-registry.js +++ b/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) diff --git a/lib/browser/rpc-server.js b/lib/browser/rpc-server.js index 91e1484411d2..12ee9d4a1c98 100644 --- a/lib/browser/rpc-server.js +++ b/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) {