From 78fe545d1800ce77fe9886effe66bb19c372745e Mon Sep 17 00:00:00 2001 From: Jeremy Rose Date: Thu, 11 Jun 2020 10:22:28 -0700 Subject: [PATCH] refactor: remove renderer-side refcount in remote (#24054) --- lib/browser/remote/objects-registry.ts | 12 ++------- lib/browser/remote/server.ts | 4 +-- lib/renderer/api/remote.js | 2 -- .../browser/api/electron_api_web_contents.cc | 4 +-- shell/browser/api/electron_api_web_contents.h | 3 +-- shell/common/api/api.mojom | 3 +-- shell/common/api/electron_api_v8_util.cc | 1 - .../common/api/remote/remote_object_freer.cc | 25 +------------------ shell/common/api/remote/remote_object_freer.h | 4 --- 9 files changed, 8 insertions(+), 50 deletions(-) diff --git a/lib/browser/remote/objects-registry.ts b/lib/browser/remote/objects-registry.ts index 4b561845888..e01e6ebb4e8 100644 --- a/lib/browser/remote/objects-registry.ts +++ b/lib/browser/remote/objects-registry.ts @@ -51,19 +51,11 @@ 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). - // 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: WebContents, contextId: string, id: number, rendererSideRefCount: number) { + remove (webContents: WebContents, contextId: string, id: number) { const ownerKey = getOwnerKey(webContents, contextId); const owner = this.owners[ownerKey]; if (owner && owner.has(id)) { - const newRefCount = owner.get(id)! - rendererSideRefCount; + const newRefCount = owner.get(id)! - 1; // Only completely remove if the number of references GCed in the // renderer is the same as the number of references we sent them diff --git a/lib/browser/remote/server.ts b/lib/browser/remote/server.ts index 2ed49817a8c..4b130e33352 100644 --- a/lib/browser/remote/server.ts +++ b/lib/browser/remote/server.ts @@ -557,8 +557,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, rendererSideRefCount) { - objectsRegistry.remove(event.sender, contextId, id, rendererSideRefCount); +handleRemoteCommand('ELECTRON_BROWSER_DEREFERENCE', function (event, contextId, id) { + objectsRegistry.remove(event.sender, contextId, id); }); handleRemoteCommand('ELECTRON_BROWSER_CONTEXT_RELEASE', (event, contextId) => { diff --git a/lib/renderer/api/remote.js b/lib/renderer/api/remote.js index 9773a266ef1..757ca9b9257 100644 --- a/lib/renderer/api/remote.js +++ b/lib/renderer/api/remote.js @@ -231,7 +231,6 @@ function metaToValue (meta) { } else { let ret; if (remoteObjectCache.has(meta.id)) { - v8Util.addRemoteObjectRef(contextId, meta.id); return remoteObjectCache.get(meta.id); } @@ -261,7 +260,6 @@ 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, 'electronId', meta.id); - v8Util.addRemoteObjectRef(contextId, meta.id); remoteObjectCache.set(meta.id, ret); return ret; } diff --git a/shell/browser/api/electron_api_web_contents.cc b/shell/browser/api/electron_api_web_contents.cc index d8fcadd7635..b70b5033912 100644 --- a/shell/browser/api/electron_api_web_contents.cc +++ b/shell/browser/api/electron_api_web_contents.cc @@ -1228,12 +1228,10 @@ void WebContents::MessageHost(const std::string& channel, #if BUILDFLAG(ENABLE_REMOTE_MODULE) void WebContents::DereferenceRemoteJSObject(const std::string& context_id, - int object_id, - int ref_count) { + int object_id) { base::ListValue args; args.Append(context_id); args.Append(object_id); - args.Append(ref_count); EmitWithSender("-ipc-message", bindings_.dispatch_context(), InvokeCallback(), /* internal */ true, "ELECTRON_BROWSER_DEREFERENCE", std::move(args)); diff --git a/shell/browser/api/electron_api_web_contents.h b/shell/browser/api/electron_api_web_contents.h index c96e67862b7..5e194037843 100644 --- a/shell/browser/api/electron_api_web_contents.h +++ b/shell/browser/api/electron_api_web_contents.h @@ -599,8 +599,7 @@ class WebContents : public gin_helper::TrackableObject, blink::CloneableMessage arguments) override; #if BUILDFLAG(ENABLE_REMOTE_MODULE) void DereferenceRemoteJSObject(const std::string& context_id, - int object_id, - int ref_count) override; + int object_id) override; #endif void UpdateDraggableRegions( std::vector regions) override; diff --git a/shell/common/api/api.mojom b/shell/common/api/api.mojom index be8758f7098..3fc2c105928 100644 --- a/shell/common/api/api.mojom +++ b/shell/common/api/api.mojom @@ -84,8 +84,7 @@ interface ElectronBrowser { [EnableIf=enable_remote_module] DereferenceRemoteJSObject( string context_id, - int32 object_id, - int32 ref_count); + int32 object_id); UpdateDraggableRegions( array regions); diff --git a/shell/common/api/electron_api_v8_util.cc b/shell/common/api/electron_api_v8_util.cc index dbe7f0a0d9e..2cadb27770e 100644 --- a/shell/common/api/electron_api_v8_util.cc +++ b/shell/common/api/electron_api_v8_util.cc @@ -149,7 +149,6 @@ void Initialize(v8::Local exports, dict.SetMethod("setRemoteCallbackFreer", &electron::RemoteCallbackFreer::BindTo); dict.SetMethod("setRemoteObjectFreer", &electron::RemoteObjectFreer::BindTo); - dict.SetMethod("addRemoteObjectRef", &electron::RemoteObjectFreer::AddRef); dict.SetMethod( "createDoubleIDWeakMap", &electron::api::KeyWeakMap>::Create); diff --git a/shell/common/api/remote/remote_object_freer.cc b/shell/common/api/remote/remote_object_freer.cc index e82bbe5b717..9955732c8bd 100644 --- a/shell/common/api/remote/remote_object_freer.cc +++ b/shell/common/api/remote/remote_object_freer.cc @@ -35,14 +35,6 @@ 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, @@ -65,25 +57,10 @@ void RemoteObjectFreer::RunDestructor() { if (!render_frame) return; - // Reset our local ref count in case we are in a GC race condition - // and will get more references in an inbound IPC message - int ref_count = 0; - const auto objects_it = ref_mapper_.find(context_id_); - if (objects_it != std::end(ref_mapper_)) { - auto& objects = objects_it->second; - const auto ref_it = objects.find(object_id_); - if (ref_it != std::end(objects)) { - ref_count = ref_it->second; - objects.erase(ref_it); - } - if (objects.empty()) - ref_mapper_.erase(objects_it); - } - mojom::ElectronBrowserPtr electron_ptr; render_frame->GetRemoteInterfaces()->GetInterface( mojo::MakeRequest(&electron_ptr)); - electron_ptr->DereferenceRemoteJSObject(context_id_, object_id_, ref_count); + electron_ptr->DereferenceRemoteJSObject(context_id_, object_id_); } } // namespace electron diff --git a/shell/common/api/remote/remote_object_freer.h b/shell/common/api/remote/remote_object_freer.h index 037b19e77a2..631c77749f2 100644 --- a/shell/common/api/remote/remote_object_freer.h +++ b/shell/common/api/remote/remote_object_freer.h @@ -18,7 +18,6 @@ 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, @@ -29,9 +28,6 @@ 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_;