From e1e73fa5f5e96af7c352053f34879e44ea8ab0bd Mon Sep 17 00:00:00 2001 From: Jeremy Rose Date: Tue, 16 Jun 2020 14:34:08 -0700 Subject: [PATCH] refactor: use WeakRef on main process side of remote (#24115) --- BUILD.gn | 7 --- lib/browser/remote/server.ts | 55 +++++++++++++---- shell/common/api/api.mojom | 7 --- shell/common/api/electron_api_v8_util.cc | 11 ---- .../api/remote/remote_callback_freer.cc | 61 ------------------- .../common/api/remote/remote_callback_freer.h | 49 --------------- shell/renderer/electron_api_service_impl.cc | 27 -------- shell/renderer/electron_api_service_impl.h | 4 -- typings/internal-ambient.d.ts | 3 - 9 files changed, 43 insertions(+), 181 deletions(-) delete mode 100644 shell/common/api/remote/remote_callback_freer.cc delete mode 100644 shell/common/api/remote/remote_callback_freer.h diff --git a/BUILD.gn b/BUILD.gn index 841a7f0d86..5df3b1b38d 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -602,13 +602,6 @@ source_set("electron_lib") { ] } - if (enable_remote_module) { - sources += [ - "shell/common/api/remote/remote_callback_freer.cc", - "shell/common/api/remote/remote_callback_freer.h", - ] - } - if (enable_desktop_capturer) { if (is_component_build && !is_linux) { # On windows the implementation relies on unexported diff --git a/lib/browser/remote/server.ts b/lib/browser/remote/server.ts index ae24d88893..714efd008f 100644 --- a/lib/browser/remote/server.ts +++ b/lib/browser/remote/server.ts @@ -14,16 +14,48 @@ if (!features.isRemoteModuleEnabled()) { throw new Error('remote module is disabled'); } -const hasProp = {}.hasOwnProperty; - // The internal properties of Function. const FUNCTION_PROPERTIES = [ 'length', 'name', 'arguments', 'caller', 'prototype' ]; +type RendererFunctionId = [string, number] // [contextId, funcId] +type FinalizerInfo = { id: RendererFunctionId, webContents: electron.WebContents, frameId: number }; +type WeakRef = { deref(): T | undefined } +type CallIntoRenderer = (...args: any[]) => void + // The remote functions in renderer processes. -// id => Function -const rendererFunctions = v8Util.createDoubleIDWeakMap<(...args: any[]) => void>(); +const rendererFunctionCache = new Map>(); +// eslint-disable-next-line no-undef +const finalizationRegistry = new (globalThis as any).FinalizationRegistry((fi: FinalizerInfo) => { + const mapKey = fi.id[0] + '~' + fi.id[1]; + const ref = rendererFunctionCache.get(mapKey); + if (ref !== undefined && ref.deref() === undefined) { + rendererFunctionCache.delete(mapKey); + if (!fi.webContents.isDestroyed()) { fi.webContents.sendToFrame(fi.frameId, 'ELECTRON_RENDERER_RELEASE_CALLBACK', fi.id[0], fi.id[1]); } + } +}); + +function getCachedRendererFunction (id: RendererFunctionId): CallIntoRenderer | undefined { + const mapKey = id[0] + '~' + id[1]; + const ref = rendererFunctionCache.get(mapKey); + if (ref !== undefined) { + const deref = ref.deref(); + if (deref !== undefined) return deref; + } +} +function setCachedRendererFunction (id: RendererFunctionId, wc: electron.WebContents, frameId: number, value: CallIntoRenderer) { + // eslint-disable-next-line no-undef + const wr = new (globalThis as any).WeakRef(value) as WeakRef; + const mapKey = id[0] + '~' + id[1]; + rendererFunctionCache.set(mapKey, wr); + finalizationRegistry.register(value, { + id, + webContents: wc, + frameId + } as FinalizerInfo); + return value; +} // Return the description of object's members: const getObjectMembers = function (object: any): ObjectMember[] { @@ -80,7 +112,7 @@ const valueToMeta = function (sender: electron.WebContents, contextId: string, v type = 'value'; } else if (isPromise(value)) { type = 'promise'; - } else if (hasProp.call(value, 'callee') && value.length != null) { + } else if (Object.prototype.hasOwnProperty.call(value, 'callee') && value.length != null) { // Treat the arguments object as array. type = 'array'; } else if (optimizeSimpleObject && v8Util.getHiddenValue(value, 'simple')) { @@ -226,9 +258,8 @@ const unwrapArgs = function (sender: electron.WebContents, frameId: number, cont const objectId: [string, number] = [contextId, meta.id]; // Cache the callbacks in renderer. - if (rendererFunctions.has(objectId)) { - return rendererFunctions.get(objectId); - } + const cachedFunction = getCachedRendererFunction(objectId); + if (cachedFunction !== undefined) { return cachedFunction; } const callIntoRenderer = function (this: any, ...args: any[]) { let succeed = false; @@ -242,8 +273,7 @@ const unwrapArgs = function (sender: electron.WebContents, frameId: number, cont v8Util.setHiddenValue(callIntoRenderer, 'location', meta.location); Object.defineProperty(callIntoRenderer, 'length', { value: meta.length }); - v8Util.setRemoteCallbackFreer(callIntoRenderer, frameId, contextId, meta.id, sender); - rendererFunctions.set(objectId, callIntoRenderer); + setCachedRendererFunction(objectId, sender, frameId, callIntoRenderer); return callIntoRenderer; } default: @@ -308,11 +338,12 @@ const logStack = function (contents: electron.WebContents, code: string, stack: handleRemoteCommand('ELECTRON_BROWSER_WRONG_CONTEXT_ERROR', function (event, contextId, passedContextId, id) { const objectId: [string, number] = [passedContextId, id]; - if (!rendererFunctions.has(objectId)) { + const cachedFunction = getCachedRendererFunction(objectId); + if (cachedFunction === undefined) { // Do nothing if the error has already been reported before. return; } - removeRemoteListenersAndLogWarning(event.sender, rendererFunctions.get(objectId)!); + removeRemoteListenersAndLogWarning(event.sender, cachedFunction); }); handleRemoteCommand('ELECTRON_BROWSER_REQUIRE', function (event, contextId, moduleName, stack) { diff --git a/shell/common/api/api.mojom b/shell/common/api/api.mojom index 3fc2c10592..320eedbc0a 100644 --- a/shell/common/api/api.mojom +++ b/shell/common/api/api.mojom @@ -15,13 +15,6 @@ interface ElectronRenderer { ReceivePostMessage(string channel, blink.mojom.TransferableMessage message); - // This is an API specific to the "remote" module, and will ultimately be - // replaced by generic IPC once WeakRef is generally available. - [EnableIf=enable_remote_module] - DereferenceRemoteJSCallback( - string context_id, - int32 object_id); - NotifyUserActivation(); TakeHeapSnapshot(handle file) => (bool success); diff --git a/shell/common/api/electron_api_v8_util.cc b/shell/common/api/electron_api_v8_util.cc index 9780d91c24..b54ac924e0 100644 --- a/shell/common/api/electron_api_v8_util.cc +++ b/shell/common/api/electron_api_v8_util.cc @@ -16,10 +16,6 @@ #include "url/origin.h" #include "v8/include/v8-profiler.h" -#if BUILDFLAG(ENABLE_REMOTE_MODULE) -#include "shell/common/api/remote/remote_callback_freer.h" -#endif - namespace std { // The hash function used by DoubleIDWeakMap. @@ -144,13 +140,6 @@ void Initialize(v8::Local exports, dict.SetMethod("deleteHiddenValue", &DeleteHiddenValue); dict.SetMethod("getObjectHash", &GetObjectHash); dict.SetMethod("takeHeapSnapshot", &TakeHeapSnapshot); -#if BUILDFLAG(ENABLE_REMOTE_MODULE) - dict.SetMethod("setRemoteCallbackFreer", - &electron::RemoteCallbackFreer::BindTo); - dict.SetMethod( - "createDoubleIDWeakMap", - &electron::api::KeyWeakMap>::Create); -#endif dict.SetMethod("requestGarbageCollectionForTesting", &RequestGarbageCollectionForTesting); dict.SetMethod("isSameOrigin", &IsSameOrigin); diff --git a/shell/common/api/remote/remote_callback_freer.cc b/shell/common/api/remote/remote_callback_freer.cc deleted file mode 100644 index 40dd4a706d..0000000000 --- a/shell/common/api/remote/remote_callback_freer.cc +++ /dev/null @@ -1,61 +0,0 @@ -// Copyright (c) 2016 GitHub, Inc. -// Use of this source code is governed by the MIT license that can be -// found in the LICENSE file. - -#include "shell/common/api/remote/remote_callback_freer.h" - -#include "base/strings/utf_string_conversions.h" -#include "base/values.h" -#include "content/public/browser/render_frame_host.h" -#include "content/public/browser/web_contents.h" -#include "electron/shell/common/api/api.mojom.h" -#include "mojo/public/cpp/bindings/associated_remote.h" -#include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h" - -namespace electron { - -// static -void RemoteCallbackFreer::BindTo(v8::Isolate* isolate, - v8::Local target, - int frame_id, - const std::string& context_id, - int object_id, - content::WebContents* web_contents) { - new RemoteCallbackFreer(isolate, target, frame_id, context_id, object_id, - web_contents); -} - -RemoteCallbackFreer::RemoteCallbackFreer(v8::Isolate* isolate, - v8::Local target, - int frame_id, - const std::string& context_id, - int object_id, - content::WebContents* web_contents) - : ObjectLifeMonitor(isolate, target), - content::WebContentsObserver(web_contents), - frame_id_(frame_id), - context_id_(context_id), - object_id_(object_id) {} - -RemoteCallbackFreer::~RemoteCallbackFreer() = default; - -void RemoteCallbackFreer::RunDestructor() { - auto frames = web_contents()->GetAllFrames(); - auto iter = std::find_if(frames.begin(), frames.end(), [this](auto* f) { - return f->GetRoutingID() == frame_id_; - }); - - if (iter != frames.end() && (*iter)->IsRenderFrameLive()) { - mojo::AssociatedRemote electron_renderer; - (*iter)->GetRemoteAssociatedInterfaces()->GetInterface(&electron_renderer); - electron_renderer->DereferenceRemoteJSCallback(context_id_, object_id_); - } - - Observe(nullptr); -} - -void RemoteCallbackFreer::RenderViewDeleted(content::RenderViewHost*) { - delete this; -} - -} // namespace electron diff --git a/shell/common/api/remote/remote_callback_freer.h b/shell/common/api/remote/remote_callback_freer.h deleted file mode 100644 index 7e9d95a2e0..0000000000 --- a/shell/common/api/remote/remote_callback_freer.h +++ /dev/null @@ -1,49 +0,0 @@ -// Copyright (c) 2016 GitHub, Inc. -// Use of this source code is governed by the MIT license that can be -// found in the LICENSE file. - -#ifndef SHELL_COMMON_API_REMOTE_REMOTE_CALLBACK_FREER_H_ -#define SHELL_COMMON_API_REMOTE_REMOTE_CALLBACK_FREER_H_ - -#include - -#include "content/public/browser/web_contents_observer.h" -#include "shell/common/api/object_life_monitor.h" - -namespace electron { - -class RemoteCallbackFreer : public ObjectLifeMonitor, - public content::WebContentsObserver { - public: - static void BindTo(v8::Isolate* isolate, - v8::Local target, - int frame_id, - const std::string& context_id, - int object_id, - content::WebContents* web_conents); - - protected: - RemoteCallbackFreer(v8::Isolate* isolate, - v8::Local target, - int frame_id, - const std::string& context_id, - int object_id, - content::WebContents* web_conents); - ~RemoteCallbackFreer() override; - - void RunDestructor() override; - - // content::WebContentsObserver: - void RenderViewDeleted(content::RenderViewHost*) override; - - private: - int frame_id_; - std::string context_id_; - int object_id_; - - DISALLOW_COPY_AND_ASSIGN(RemoteCallbackFreer); -}; - -} // namespace electron - -#endif // SHELL_COMMON_API_REMOTE_REMOTE_CALLBACK_FREER_H_ diff --git a/shell/renderer/electron_api_service_impl.cc b/shell/renderer/electron_api_service_impl.cc index 2468265736..fb0ce3266d 100644 --- a/shell/renderer/electron_api_service_impl.cc +++ b/shell/renderer/electron_api_service_impl.cc @@ -209,33 +209,6 @@ void ElectronApiServiceImpl::ReceivePostMessage( 0); } -#if BUILDFLAG(ENABLE_REMOTE_MODULE) -void ElectronApiServiceImpl::DereferenceRemoteJSCallback( - const std::string& context_id, - int32_t object_id) { - const auto* channel = "ELECTRON_RENDERER_RELEASE_CALLBACK"; - if (!document_created_) - return; - blink::WebLocalFrame* frame = render_frame()->GetWebFrame(); - if (!frame) - return; - - v8::Isolate* isolate = blink::MainThreadIsolate(); - v8::HandleScope handle_scope(isolate); - - v8::Local context = renderer_client_->GetContext(frame, isolate); - v8::Context::Scope context_scope(context); - - base::ListValue args; - args.AppendString(context_id); - args.AppendInteger(object_id); - - v8::Local v8_args = gin::ConvertToV8(isolate, args); - EmitIPCEvent(context, true /* internal */, channel, {}, v8_args, - 0 /* sender_id */); -} -#endif - void ElectronApiServiceImpl::NotifyUserActivation() { blink::WebLocalFrame* frame = render_frame()->GetWebFrame(); if (frame) diff --git a/shell/renderer/electron_api_service_impl.h b/shell/renderer/electron_api_service_impl.h index 8bcc3de2a3..56a12a2553 100644 --- a/shell/renderer/electron_api_service_impl.h +++ b/shell/renderer/electron_api_service_impl.h @@ -35,10 +35,6 @@ class ElectronApiServiceImpl : public mojom::ElectronRenderer, int32_t sender_id) override; void ReceivePostMessage(const std::string& channel, blink::TransferableMessage message) override; -#if BUILDFLAG(ENABLE_REMOTE_MODULE) - void DereferenceRemoteJSCallback(const std::string& context_id, - int32_t object_id) override; -#endif void NotifyUserActivation() override; void TakeHeapSnapshot(mojo::ScopedHandle file, TakeHeapSnapshotCallback callback) override; diff --git a/typings/internal-ambient.d.ts b/typings/internal-ambient.d.ts index ec0b8c4507..d4a941e7fa 100644 --- a/typings/internal-ambient.d.ts +++ b/typings/internal-ambient.d.ts @@ -38,13 +38,10 @@ declare namespace NodeJS { setHiddenValue(obj: any, key: string, value: T): void; deleteHiddenValue(obj: any, key: string): void; requestGarbageCollectionForTesting(): void; - createDoubleIDWeakMap(): ElectronInternal.KeyWeakMap<[string, number], V>; - setRemoteCallbackFreer(fn: Function, frameId: number, contextId: String, id: number, sender: any): void weaklyTrackValue(value: any): void; clearWeaklyTrackedValues(): void; getWeaklyTrackedValues(): any[]; addRemoteObjectRef(contextId: string, id: number): void; - setRemoteCallbackFreer(fn: Function, contextId: string, id: number, sender: any): void } type DataPipe = {