From 78411db4b5c796570ee0e5eec1ef00f586e0a01d Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Tue, 16 Apr 2019 16:08:11 -0400 Subject: [PATCH] fix: maintain a ref count for objects sent over remote (#17464) * spec: clean up after a failed window count assertion Previously when this assertion failed all tests that ran after the failed assertion also failed. This ensure that the assertion fails for the test that actually caused the issue but cleans up the left-over windows so that future tests do not fail. * fix: maintain a ref count for objects sent over remote Previously there was a race condition where a GC could occur in the renderer process between the main process sending a meta.id and the renderer pulling the proxy out its weakmap to stop it being GC'ed. This fixes that race condition by maintaining a "sent" ref count in the object registry and a "received" ref count in the object cache on the renderer side. The deref request now sends the number of refs the renderer thinks it owns, if the number does not match the value in the object registry it is assumed that there is an IPC message containing a new reference in flight and this race condition was hit. The browser side ref count is then reduced and we wait for the new deref message. This guaruntees that an object will only be removed from the registry if every reference we sent has been guarunteed to be unreffed. --- atom/common/api/atom_api_v8_util.cc | 1 + atom/common/api/remote_object_freer.cc | 14 +++++++++ atom/common/api/remote_object_freer.h | 5 ++++ lib/browser/objects-registry.js | 40 +++++++++++++++++++------- lib/browser/rpc-server.js | 4 +-- lib/renderer/api/remote.js | 2 ++ spec-main/window-helpers.ts | 8 +++++- spec/window-helpers.js | 17 +++++++++-- 8 files changed, 75 insertions(+), 16 deletions(-) diff --git a/atom/common/api/atom_api_v8_util.cc b/atom/common/api/atom_api_v8_util.cc index e705187c6581..d41106170231 100644 --- a/atom/common/api/atom_api_v8_util.cc +++ b/atom/common/api/atom_api_v8_util.cc @@ -115,6 +115,7 @@ void Initialize(v8::Local exports, dict.SetMethod("takeHeapSnapshot", &TakeHeapSnapshot); dict.SetMethod("setRemoteCallbackFreer", &atom::RemoteCallbackFreer::BindTo); dict.SetMethod("setRemoteObjectFreer", &atom::RemoteObjectFreer::BindTo); + dict.SetMethod("addRemoteObjectRef", &atom::RemoteObjectFreer::AddRef); dict.SetMethod("createIDWeakMap", &atom::api::KeyWeakMap::Create); dict.SetMethod( "createDoubleIDWeakMap", diff --git a/atom/common/api/remote_object_freer.cc b/atom/common/api/remote_object_freer.cc index 39e14f0a2a5e..81e84a4d887f 100644 --- a/atom/common/api/remote_object_freer.cc +++ b/atom/common/api/remote_object_freer.cc @@ -36,6 +36,14 @@ void RemoteObjectFreer::BindTo(v8::Isolate* isolate, new RemoteObjectFreer(isolate, target, context_id, object_id); } +// static +void RemoteObjectFreer::AddRef(const std::string& context_id, int object_id) { + ref_mapper_[context_id][object_id]++; +} + +// static +std::map> RemoteObjectFreer::ref_mapper_; + RemoteObjectFreer::RemoteObjectFreer(v8::Isolate* isolate, v8::Local target, const std::string& context_id, @@ -62,6 +70,12 @@ void RemoteObjectFreer::RunDestructor() { base::ListValue args; args.AppendString(context_id_); args.AppendInteger(object_id_); + args.AppendInteger(ref_mapper_[context_id_][object_id_]); + // Reset our local ref count in case we are in a GC race condition and will + // get more references in an inbound IPC message + ref_mapper_[context_id_].erase(object_id_); + if (ref_mapper_[context_id_].empty()) + ref_mapper_.erase(context_id_); mojom::ElectronBrowserAssociatedPtr electron_ptr; render_frame->GetRemoteAssociatedInterfaces()->GetInterface( diff --git a/atom/common/api/remote_object_freer.h b/atom/common/api/remote_object_freer.h index f86de92d8fa0..b89ff388d35e 100644 --- a/atom/common/api/remote_object_freer.h +++ b/atom/common/api/remote_object_freer.h @@ -5,6 +5,7 @@ #ifndef ATOM_COMMON_API_REMOTE_OBJECT_FREER_H_ #define ATOM_COMMON_API_REMOTE_OBJECT_FREER_H_ +#include #include #include "atom/common/api/object_life_monitor.h" @@ -17,6 +18,7 @@ class RemoteObjectFreer : public ObjectLifeMonitor { v8::Local target, const std::string& context_id, int object_id); + static void AddRef(const std::string& context_id, int object_id); protected: RemoteObjectFreer(v8::Isolate* isolate, @@ -27,6 +29,9 @@ class RemoteObjectFreer : public ObjectLifeMonitor { void RunDestructor() override; + // { context_id => { object_id => ref_count }} + static std::map> ref_mapper_; + private: std::string context_id_; int object_id_; diff --git a/lib/browser/objects-registry.js b/lib/browser/objects-registry.js index a2d9b3ec9679..b9b85241ef96 100644 --- a/lib/browser/objects-registry.js +++ b/lib/browser/objects-registry.js @@ -14,8 +14,8 @@ class ObjectsRegistry { // (id) => {object, count} this.storage = {} - // Stores the IDs of objects referenced by WebContents. - // (ownerKey) => [id] + // Stores the IDs + refCounts of objects referenced by WebContents. + // (ownerKey) => { id: refCount } this.owners = {} } @@ -29,14 +29,16 @@ class ObjectsRegistry { const ownerKey = getOwnerKey(webContents, contextId) let owner = this.owners[ownerKey] if (!owner) { - owner = this.owners[ownerKey] = new Set() + owner = this.owners[ownerKey] = new Map() this.registerDeleteListener(webContents, contextId) } if (!owner.has(id)) { - owner.add(id) + owner.set(id, 0) // Increase reference count if not referenced before. this.storage[id].count++ } + + owner.set(id, owner.get(id) + 1) return id } @@ -49,14 +51,30 @@ 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 (webContents, contextId, id) { + // rendererSideRefCount is the ref count that the renderer process reported + // at time of GC if this is different to the number of references we sent to + // the given owner then a GC occurred between a ref being sent and the value + // being pulled out of the weak map. + // In this case we decrement out ref count and do not delete the stored + // object + // For more details on why we do renderer side ref counting see + // https://github.com/electron/electron/pull/17464 + remove (webContents, contextId, id, rendererSideRefCount) { const ownerKey = getOwnerKey(webContents, contextId) const owner = this.owners[ownerKey] - if (owner) { - // Remove the reference in owner. - owner.delete(id) - // Dereference from the storage. - this.dereference(id) + if (owner && owner.has(id)) { + const newRefCount = owner.get(id) - rendererSideRefCount + + // Only completely remove if the number of references GCed in the + // renderer is the same as the number of references we sent them + if (newRefCount <= 0) { + // Remove the reference in owner. + owner.delete(id) + // Dereference from the storage. + this.dereference(id) + } else { + owner.set(id, newRefCount) + } } } @@ -66,7 +84,7 @@ class ObjectsRegistry { const owner = this.owners[ownerKey] if (!owner) return - for (const id of owner) this.dereference(id) + for (const id of owner.keys()) this.dereference(id) delete this.owners[ownerKey] } diff --git a/lib/browser/rpc-server.js b/lib/browser/rpc-server.js index ac8180f2b492..837180f3f7eb 100644 --- a/lib/browser/rpc-server.js +++ b/lib/browser/rpc-server.js @@ -436,8 +436,8 @@ handleRemoteCommand('ELECTRON_BROWSER_MEMBER_GET', function (event, contextId, i return valueToMeta(event.sender, contextId, obj[name]) }) -handleRemoteCommand('ELECTRON_BROWSER_DEREFERENCE', function (event, contextId, id) { - objectsRegistry.remove(event.sender, contextId, id) +handleRemoteCommand('ELECTRON_BROWSER_DEREFERENCE', function (event, contextId, id, rendererSideRefCount) { + objectsRegistry.remove(event.sender, contextId, id, rendererSideRefCount) }) handleRemoteCommand('ELECTRON_BROWSER_CONTEXT_RELEASE', (event, contextId) => { diff --git a/lib/renderer/api/remote.js b/lib/renderer/api/remote.js index c7974a740ac6..d51ca3739090 100644 --- a/lib/renderer/api/remote.js +++ b/lib/renderer/api/remote.js @@ -227,6 +227,7 @@ function metaToValue (meta) { } else { let ret if (remoteObjectCache.has(meta.id)) { + v8Util.addRemoteObjectRef(contextId, meta.id) return remoteObjectCache.get(meta.id) } @@ -254,6 +255,7 @@ function metaToValue (meta) { // Track delegate obj's lifetime & tell browser to clean up when object is GCed. v8Util.setRemoteObjectFreer(ret, contextId, meta.id) v8Util.setHiddenValue(ret, 'atomId', meta.id) + v8Util.addRemoteObjectRef(contextId, meta.id) remoteObjectCache.set(meta.id, ret) return ret } diff --git a/spec-main/window-helpers.ts b/spec-main/window-helpers.ts index 56345a1bbb36..c2c7fabdcfd3 100644 --- a/spec-main/window-helpers.ts +++ b/spec-main/window-helpers.ts @@ -14,7 +14,13 @@ export const closeWindow = async ( } if (assertNotWindows) { - expect(BrowserWindow.getAllWindows()).to.have.lengthOf(0) + const windows = BrowserWindow.getAllWindows() + for (const win of windows) { + const closePromise = emittedOnce(win, 'closed') + win.close() + await closePromise + } + expect(windows).to.have.lengthOf(0) } } diff --git a/spec/window-helpers.js b/spec/window-helpers.js index 2efba2a55c16..ad1ea57f2790 100644 --- a/spec/window-helpers.js +++ b/spec/window-helpers.js @@ -1,5 +1,6 @@ const { expect } = require('chai') -const { BrowserWindow } = require('electron').remote +const { remote } = require('electron') +const { BrowserWindow } = remote const { emittedOnce } = require('./events-helpers') @@ -14,7 +15,19 @@ exports.closeWindow = async (window = null, } if (assertSingleWindow) { - expect(BrowserWindow.getAllWindows()).to.have.lengthOf(1) + // Although we want to assert that no windows were left handing around + // we also want to clean up the left over windows so that no future + // tests fail as a side effect + const currentId = remote.getCurrentWindow().id + const windows = BrowserWindow.getAllWindows() + for (const win of windows) { + if (win.id !== currentId) { + const closePromise = emittedOnce(win, 'closed') + win.close() + await closePromise + } + } + expect(windows).to.have.lengthOf(1) } }