From 9d7ba98209d154ce4ea769d46740aee7b2b5e98b Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Fri, 15 May 2020 11:57:40 -0700 Subject: [PATCH] refactor: remove the RenderFrameFunctionStore and use privates to memory manage (#23592) --- filenames.gni | 2 - lib/renderer/api/context-bridge.ts | 7 +- lib/sandboxed_renderer/init.js | 6 + shell/common/api/electron_api_v8_util.cc | 28 +++ shell/common/gin_helper/callback.h | 1 + .../render_frame_function_store.cc | 49 ---- .../render_frame_function_store.h | 61 ----- .../api/electron_api_context_bridge.cc | 211 ++++++++---------- .../api/electron_api_context_bridge.h | 11 +- spec-main/api-context-bridge-spec.ts | 47 ++-- typings/internal-ambient.d.ts | 3 + typings/internal-electron.d.ts | 1 - 12 files changed, 166 insertions(+), 261 deletions(-) delete mode 100644 shell/renderer/api/context_bridge/render_frame_function_store.cc delete mode 100644 shell/renderer/api/context_bridge/render_frame_function_store.h diff --git a/filenames.gni b/filenames.gni index 17db0ce1f543..6fbe0ec51269 100644 --- a/filenames.gni +++ b/filenames.gni @@ -578,8 +578,6 @@ filenames = { "shell/common/world_ids.h", "shell/renderer/api/context_bridge/object_cache.cc", "shell/renderer/api/context_bridge/object_cache.h", - "shell/renderer/api/context_bridge/render_frame_function_store.cc", - "shell/renderer/api/context_bridge/render_frame_function_store.h", "shell/renderer/api/electron_api_context_bridge.cc", "shell/renderer/api/electron_api_context_bridge.h", "shell/renderer/api/electron_api_crash_reporter_renderer.cc", diff --git a/lib/renderer/api/context-bridge.ts b/lib/renderer/api/context-bridge.ts index 28c46e54f051..54b3716d6159 100644 --- a/lib/renderer/api/context-bridge.ts +++ b/lib/renderer/api/context-bridge.ts @@ -11,12 +11,9 @@ const contextBridge: Electron.ContextBridge = { exposeInMainWorld: (key: string, api: Record) => { checkContextIsolationEnabled(); return binding.exposeAPIInMainWorld(key, api); - }, - debugGC: () => binding._debugGCMaps({}) + } } as any; -if (!binding._debugGCMaps) delete contextBridge.debugGC; - export default contextBridge; export const internalContextBridge = { @@ -33,6 +30,6 @@ export const internalContextBridge = { isInMainWorld: () => binding._isCalledFromMainWorld() as boolean }; -if (binding._debugGCMaps) { +if (binding._isDebug) { contextBridge.internalContextBridge = internalContextBridge; } diff --git a/lib/sandboxed_renderer/init.js b/lib/sandboxed_renderer/init.js index 7de17d8435ea..4acd443d53df 100644 --- a/lib/sandboxed_renderer/init.js +++ b/lib/sandboxed_renderer/init.js @@ -113,6 +113,12 @@ function preloadRequire (module) { // Process command line arguments. const { hasSwitch } = process.electronBinding('command_line'); +// Similar to nodes --expose-internals flag, this exposes electronBinding so +// that tests can call it to get access to some test only bindings +if (hasSwitch('unsafely-expose-electron-internals-for-testing')) { + preloadProcess.electronBinding = process.electronBinding; +} + const contextIsolation = hasSwitch('context-isolation'); const isHiddenPage = hasSwitch('hidden-page'); const rendererProcessReuseEnabled = hasSwitch('disable-electron-site-instance-overrides'); diff --git a/shell/common/api/electron_api_v8_util.cc b/shell/common/api/electron_api_v8_util.cc index c7ed6b8fae10..dbe7f0a0d9ee 100644 --- a/shell/common/api/electron_api_v8_util.cc +++ b/shell/common/api/electron_api_v8_util.cc @@ -112,6 +112,29 @@ bool IsSameOrigin(const GURL& l, const GURL& r) { return url::Origin::Create(l).IsSameOriginWith(url::Origin::Create(r)); } +#ifdef DCHECK_IS_ON +std::vector> weakly_tracked_values; + +void WeaklyTrackValue(v8::Isolate* isolate, v8::Local value) { + v8::Global global_value(isolate, value); + global_value.SetWeak(); + weakly_tracked_values.push_back(std::move(global_value)); +} + +void ClearWeaklyTrackedValues() { + weakly_tracked_values.clear(); +} + +std::vector> GetWeaklyTrackedValues(v8::Isolate* isolate) { + std::vector> locals; + for (size_t i = 0; i < weakly_tracked_values.size(); i++) { + if (!weakly_tracked_values[i].IsEmpty()) + locals.push_back(weakly_tracked_values[i].Get(isolate)); + } + return locals; +} +#endif + void Initialize(v8::Local exports, v8::Local unused, v8::Local context, @@ -136,6 +159,11 @@ void Initialize(v8::Local exports, dict.SetMethod("requestGarbageCollectionForTesting", &RequestGarbageCollectionForTesting); dict.SetMethod("isSameOrigin", &IsSameOrigin); +#ifdef DCHECK_IS_ON + dict.SetMethod("getWeaklyTrackedValues", &GetWeaklyTrackedValues); + dict.SetMethod("clearWeaklyTrackedValues", &ClearWeaklyTrackedValues); + dict.SetMethod("weaklyTrackValue", &WeaklyTrackValue); +#endif } } // namespace diff --git a/shell/common/gin_helper/callback.h b/shell/common/gin_helper/callback.h index 97061886ac4e..a112f9731af5 100644 --- a/shell/common/gin_helper/callback.h +++ b/shell/common/gin_helper/callback.h @@ -9,6 +9,7 @@ #include #include "base/bind.h" +#include "gin/dictionary.h" #include "shell/common/gin_converters/std_converter.h" #include "shell/common/gin_helper/function_template.h" #include "shell/common/gin_helper/locker.h" diff --git a/shell/renderer/api/context_bridge/render_frame_function_store.cc b/shell/renderer/api/context_bridge/render_frame_function_store.cc deleted file mode 100644 index eb4c3c9c4235..000000000000 --- a/shell/renderer/api/context_bridge/render_frame_function_store.cc +++ /dev/null @@ -1,49 +0,0 @@ -// Copyright (c) 2019 Slack Technologies, Inc. -// Use of this source code is governed by the MIT license that can be -// found in the LICENSE file. - -#include "shell/renderer/api/context_bridge/render_frame_function_store.h" - -#include - -#include "shell/common/api/object_life_monitor.h" - -namespace electron { - -namespace api { - -namespace context_bridge { - -std::map& GetStoreMap() { - static base::NoDestructor> - store_map; - return *store_map; -} - -RenderFrameFunctionStore::RenderFrameFunctionStore( - content::RenderFrame* render_frame) - : content::RenderFrameObserver(render_frame), - routing_id_(render_frame->GetRoutingID()) {} - -RenderFrameFunctionStore::~RenderFrameFunctionStore() = default; - -void RenderFrameFunctionStore::OnDestruct() { - GetStoreMap().erase(routing_id_); - delete this; -} - -void RenderFrameFunctionStore::WillReleaseScriptContext( - v8::Local context, - int32_t world_id) { - base::EraseIf(functions_, [context](auto const& pair) { - v8::Local func_owning_context = - std::get<1>(pair.second).Get(context->GetIsolate()); - return func_owning_context == context; - }); -} - -} // namespace context_bridge - -} // namespace api - -} // namespace electron diff --git a/shell/renderer/api/context_bridge/render_frame_function_store.h b/shell/renderer/api/context_bridge/render_frame_function_store.h deleted file mode 100644 index f121e726ac68..000000000000 --- a/shell/renderer/api/context_bridge/render_frame_function_store.h +++ /dev/null @@ -1,61 +0,0 @@ -// Copyright (c) 2019 Slack Technologies, Inc. -// Use of this source code is governed by the MIT license that can be -// found in the LICENSE file. - -#ifndef SHELL_RENDERER_API_CONTEXT_BRIDGE_RENDER_FRAME_FUNCTION_STORE_H_ -#define SHELL_RENDERER_API_CONTEXT_BRIDGE_RENDER_FRAME_FUNCTION_STORE_H_ - -#include -#include - -#include "content/public/renderer/render_frame.h" -#include "content/public/renderer/render_frame_observer.h" -#include "shell/renderer/electron_render_frame_observer.h" -#include "third_party/blink/public/web/web_local_frame.h" - -namespace electron { - -namespace api { - -namespace context_bridge { - -using FunctionContextPair = - std::tuple, v8::Global>; - -class RenderFrameFunctionStore final : public content::RenderFrameObserver { - public: - explicit RenderFrameFunctionStore(content::RenderFrame* render_frame); - ~RenderFrameFunctionStore() override; - - // RenderFrameObserver implementation. - void OnDestruct() override; - void WillReleaseScriptContext(v8::Local context, - int32_t world_id) override; - - size_t take_func_id() { return next_func_id_++; } - - std::map& functions() { return functions_; } - - base::WeakPtr GetWeakPtr() { - return weak_factory_.GetWeakPtr(); - } - - private: - // func_id ==> { function, owning_context } - std::map functions_; - size_t next_func_id_ = 1; - - const int32_t routing_id_; - - base::WeakPtrFactory weak_factory_{this}; -}; - -std::map& GetStoreMap(); - -} // namespace context_bridge - -} // namespace api - -} // namespace electron - -#endif // SHELL_RENDERER_API_CONTEXT_BRIDGE_RENDER_FRAME_FUNCTION_STORE_H_ diff --git a/shell/renderer/api/electron_api_context_bridge.cc b/shell/renderer/api/electron_api_context_bridge.cc index 90ed012fb5ae..59816c745ac7 100644 --- a/shell/renderer/api/electron_api_context_bridge.cc +++ b/shell/renderer/api/electron_api_context_bridge.cc @@ -22,13 +22,20 @@ #include "shell/common/gin_helper/promise.h" #include "shell/common/node_includes.h" #include "shell/common/world_ids.h" -#include "shell/renderer/api/context_bridge/render_frame_function_store.h" #include "third_party/blink/public/web/web_local_frame.h" namespace electron { namespace api { +namespace context_bridge { + +const char* const kProxyFunctionPrivateKey = "electron_contextBridge_proxy_fn"; +const char* const kSupportsDynamicPropertiesPrivateKey = + "electron_contextBridge_supportsDynamicProperties"; + +} // namespace context_bridge + namespace { static int kMaxRecursion = 1000; @@ -48,17 +55,6 @@ content::RenderFrame* GetRenderFrame(const v8::Local& value) { return content::RenderFrame::FromWebFrame(frame); } -context_bridge::RenderFrameFunctionStore* GetOrCreateStore( - content::RenderFrame* render_frame) { - auto it = context_bridge::GetStoreMap().find(render_frame->GetRoutingID()); - if (it == context_bridge::GetStoreMap().end()) { - auto* store = new context_bridge::RenderFrameFunctionStore(render_frame); - context_bridge::GetStoreMap().emplace(render_frame->GetRoutingID(), store); - return store; - } - return it->second; -} - // Sourced from "extensions/renderer/v8_schema_registry.cc" // Recursively freezes every v8 object on |object|. bool DeepFreeze(const v8::Local& object, @@ -109,35 +105,27 @@ bool IsPlainArray(const v8::Local& arr) { return !arr->IsTypedArray(); } -class FunctionLifeMonitor final : public ObjectLifeMonitor { - public: - static void BindTo( - v8::Isolate* isolate, - v8::Local target, - base::WeakPtr store, - size_t func_id) { - new FunctionLifeMonitor(isolate, target, store, func_id); - } +void SetPrivate(v8::Local context, + v8::Local target, + const std::string& key, + v8::Local value) { + target + ->SetPrivate( + context, + v8::Private::ForApi(context->GetIsolate(), + gin::StringToV8(context->GetIsolate(), key)), + value) + .Check(); +} - protected: - FunctionLifeMonitor( - v8::Isolate* isolate, - v8::Local target, - base::WeakPtr store, - size_t func_id) - : ObjectLifeMonitor(isolate, target), store_(store), func_id_(func_id) {} - ~FunctionLifeMonitor() override = default; - - void RunDestructor() override { - if (!store_) - return; - store_->functions().erase(func_id_); - } - - private: - base::WeakPtr store_; - size_t func_id_; -}; +v8::MaybeLocal GetPrivate(v8::Local context, + v8::Local target, + const std::string& key) { + return target->GetPrivate( + context, + v8::Private::ForApi(context->GetIsolate(), + gin::StringToV8(context->GetIsolate(), key))); +} } // namespace @@ -145,7 +133,6 @@ v8::MaybeLocal PassValueToOtherContext( v8::Local source_context, v8::Local destination_context, v8::Local value, - context_bridge::RenderFrameFunctionStore* store, context_bridge::ObjectCache* object_cache, bool support_dynamic_properties, int recursion_depth) { @@ -169,22 +156,21 @@ v8::MaybeLocal PassValueToOtherContext( // the global handle at the right time. if (value->IsFunction()) { auto func = v8::Local::Cast(value); - v8::Global global_func(source_context->GetIsolate(), func); - v8::Global global_source(source_context->GetIsolate(), - source_context); - size_t func_id = store->take_func_id(); - store->functions()[func_id] = - std::make_tuple(std::move(global_func), std::move(global_source)); { v8::Context::Scope destination_scope(destination_context); - v8::Local proxy_func = gin_helper::CallbackToV8Leaked( - destination_context->GetIsolate(), - base::BindRepeating(&ProxyFunctionWrapper, store, func_id, - support_dynamic_properties)); - FunctionLifeMonitor::BindTo(destination_context->GetIsolate(), - v8::Local::Cast(proxy_func), - store->GetWeakPtr(), func_id); + v8::Local state = + v8::Object::New(destination_context->GetIsolate()); + SetPrivate(destination_context, state, + context_bridge::kProxyFunctionPrivateKey, func); + SetPrivate(destination_context, state, + context_bridge::kSupportsDynamicPropertiesPrivateKey, + gin::ConvertToV8(destination_context->GetIsolate(), + support_dynamic_properties)); + v8::Local proxy_func; + if (!v8::Function::New(destination_context, ProxyFunctionWrapper, state) + .ToLocal(&proxy_func)) + return v8::MaybeLocal(); object_cache->CacheProxiedObject(value, proxy_func); return v8::MaybeLocal(proxy_func); } @@ -214,7 +200,6 @@ v8::MaybeLocal PassValueToOtherContext( proxied_promise, v8::Isolate* isolate, v8::Global global_source_context, v8::Global global_destination_context, - context_bridge::RenderFrameFunctionStore* store, v8::Local result) { if (global_source_context.IsEmpty() || global_destination_context.IsEmpty()) @@ -223,13 +208,13 @@ v8::MaybeLocal PassValueToOtherContext( auto val = PassValueToOtherContext(global_source_context.Get(isolate), global_destination_context.Get(isolate), - result, store, &object_cache, false, 0); + result, &object_cache, false, 0); if (!val.IsEmpty()) proxied_promise->Resolve(val.ToLocalChecked()); }, proxied_promise, destination_context->GetIsolate(), std::move(global_then_source_context), - std::move(global_then_destination_context), store); + std::move(global_then_destination_context)); v8::Global global_catch_source_context( source_context->GetIsolate(), source_context); @@ -242,7 +227,6 @@ v8::MaybeLocal PassValueToOtherContext( proxied_promise, v8::Isolate* isolate, v8::Global global_source_context, v8::Global global_destination_context, - context_bridge::RenderFrameFunctionStore* store, v8::Local result) { if (global_source_context.IsEmpty() || global_destination_context.IsEmpty()) @@ -251,13 +235,13 @@ v8::MaybeLocal PassValueToOtherContext( auto val = PassValueToOtherContext(global_source_context.Get(isolate), global_destination_context.Get(isolate), - result, store, &object_cache, false, 0); + result, &object_cache, false, 0); if (!val.IsEmpty()) proxied_promise->Reject(val.ToLocalChecked()); }, proxied_promise, destination_context->GetIsolate(), std::move(global_catch_source_context), - std::move(global_catch_destination_context), store); + std::move(global_catch_destination_context)); ignore_result(source_promise->Then( source_context, @@ -291,7 +275,7 @@ v8::MaybeLocal PassValueToOtherContext( for (size_t i = 0; i < length; i++) { auto value_for_array = PassValueToOtherContext( source_context, destination_context, - arr->Get(source_context, i).ToLocalChecked(), store, object_cache, + arr->Get(source_context, i).ToLocalChecked(), object_cache, support_dynamic_properties, recursion_depth + 1); if (value_for_array.IsEmpty()) return v8::MaybeLocal(); @@ -309,7 +293,7 @@ v8::MaybeLocal PassValueToOtherContext( if (IsPlainObject(value)) { auto object_value = v8::Local::Cast(value); auto passed_value = CreateProxyForAPI( - object_value, source_context, destination_context, store, object_cache, + object_value, source_context, destination_context, object_cache, support_dynamic_properties, recursion_depth + 1); if (passed_value.IsEmpty()) return v8::MaybeLocal(); @@ -334,35 +318,45 @@ v8::MaybeLocal PassValueToOtherContext( } } -v8::Local ProxyFunctionWrapper( - context_bridge::RenderFrameFunctionStore* store, - size_t func_id, - bool support_dynamic_properties, - gin_helper::Arguments* args) { +void ProxyFunctionWrapper(const v8::FunctionCallbackInfo& info) { TRACE_EVENT0("electron", "ContextBridge::ProxyFunctionWrapper"); + CHECK(info.Data()->IsObject()); + v8::Local data = info.Data().As(); + bool support_dynamic_properties = false; + gin::Arguments args(info); // Context the proxy function was called from - v8::Local calling_context = args->isolate()->GetCurrentContext(); - // Context the function was created in - v8::Local func_owning_context = - std::get<1>(store->functions()[func_id]).Get(args->isolate()); + v8::Local calling_context = args.isolate()->GetCurrentContext(); + + // Pull the original function and its context off of the data private key + v8::MaybeLocal sdp_value = + GetPrivate(calling_context, data, + context_bridge::kSupportsDynamicPropertiesPrivateKey); + v8::MaybeLocal maybe_func = GetPrivate( + calling_context, data, context_bridge::kProxyFunctionPrivateKey); + v8::Local func_value; + if (sdp_value.IsEmpty() || maybe_func.IsEmpty() || + !gin::ConvertFromV8(args.isolate(), sdp_value.ToLocalChecked(), + &support_dynamic_properties) || + !maybe_func.ToLocal(&func_value)) + return; + + v8::Local func = v8::Local::Cast(func_value); + v8::Local func_owning_context = func->CreationContext(); { v8::Context::Scope func_owning_context_scope(func_owning_context); context_bridge::ObjectCache object_cache; - v8::Local func = - (std::get<0>(store->functions()[func_id])).Get(args->isolate()); - std::vector> original_args; std::vector> proxied_args; - args->GetRemaining(&original_args); + args.GetRemaining(&original_args); for (auto value : original_args) { - auto arg = PassValueToOtherContext(calling_context, func_owning_context, - value, store, &object_cache, - support_dynamic_properties, 0); + auto arg = + PassValueToOtherContext(calling_context, func_owning_context, value, + &object_cache, support_dynamic_properties, 0); if (arg.IsEmpty()) - return v8::Undefined(args->isolate()); + return; proxied_args.push_back(arg.ToLocalChecked()); } @@ -370,7 +364,7 @@ v8::Local ProxyFunctionWrapper( bool did_error = false; std::string error_message; { - v8::TryCatch try_catch(args->isolate()); + v8::TryCatch try_catch(args.isolate()); maybe_return_value = func->Call(func_owning_context, func, proxied_args.size(), proxied_args.data()); if (try_catch.HasCaught()) { @@ -378,7 +372,7 @@ v8::Local ProxyFunctionWrapper( auto message = try_catch.Message(); if (message.IsEmpty() || - !gin::ConvertFromV8(args->isolate(), message->Get(), + !gin::ConvertFromV8(args.isolate(), message->Get(), &error_message)) { error_message = "An unknown exception occurred in the isolated context, an error " @@ -389,20 +383,21 @@ v8::Local ProxyFunctionWrapper( if (did_error) { v8::Context::Scope calling_context_scope(calling_context); - args->ThrowError(error_message); - return v8::Local(); + args.isolate()->ThrowException( + v8::Exception::Error(gin::StringToV8(args.isolate(), error_message))); + return; } if (maybe_return_value.IsEmpty()) - return v8::Undefined(args->isolate()); + return; auto ret = PassValueToOtherContext(func_owning_context, calling_context, - maybe_return_value.ToLocalChecked(), store, + maybe_return_value.ToLocalChecked(), &object_cache, support_dynamic_properties, 0); if (ret.IsEmpty()) - return v8::Undefined(args->isolate()); - return ret.ToLocalChecked(); + return; + info.GetReturnValue().Set(ret.ToLocalChecked()); } } @@ -410,7 +405,6 @@ v8::MaybeLocal CreateProxyForAPI( const v8::Local& api_object, const v8::Local& source_context, const v8::Local& destination_context, - context_bridge::RenderFrameFunctionStore* store, context_bridge::ObjectCache* object_cache, bool support_dynamic_properties, int recursion_depth) { @@ -458,15 +452,13 @@ v8::MaybeLocal CreateProxyForAPI( v8::Local setter_proxy; if (!getter.IsEmpty()) { if (!PassValueToOtherContext(source_context, destination_context, - getter, store, object_cache, false, - 1) + getter, object_cache, false, 1) .ToLocal(&getter_proxy)) continue; } if (!setter.IsEmpty()) { if (!PassValueToOtherContext(source_context, destination_context, - setter, store, object_cache, false, - 1) + setter, object_cache, false, 1) .ToLocal(&setter_proxy)) continue; } @@ -484,7 +476,7 @@ v8::MaybeLocal CreateProxyForAPI( continue; auto passed_value = PassValueToOtherContext( - source_context, destination_context, value, store, object_cache, + source_context, destination_context, value, object_cache, support_dynamic_properties, recursion_depth + 1); if (passed_value.IsEmpty()) return v8::MaybeLocal(); @@ -495,24 +487,12 @@ v8::MaybeLocal CreateProxyForAPI( } } -#ifdef DCHECK_IS_ON -gin_helper::Dictionary DebugGC(gin_helper::Dictionary empty) { - auto* render_frame = GetRenderFrame(empty.GetHandle()); - auto* store = GetOrCreateStore(render_frame); - gin_helper::Dictionary ret = gin::Dictionary::CreateEmpty(empty.isolate()); - ret.Set("functionCount", store->functions().size()); - return ret; -} -#endif - void ExposeAPIInMainWorld(const std::string& key, v8::Local api_object, gin_helper::Arguments* args) { TRACE_EVENT1("electron", "ContextBridge::ExposeAPIInMainWorld", "key", key); auto* render_frame = GetRenderFrame(api_object); CHECK(render_frame); - context_bridge::RenderFrameFunctionStore* store = - GetOrCreateStore(render_frame); auto* frame = render_frame->GetWebFrame(); CHECK(frame); v8::Local main_context = frame->MainWorldScriptContext(); @@ -533,9 +513,8 @@ void ExposeAPIInMainWorld(const std::string& key, context_bridge::ObjectCache object_cache; v8::Context::Scope main_context_scope(main_context); - v8::MaybeLocal maybe_proxy = - CreateProxyForAPI(api_object, isolated_context, main_context, store, - &object_cache, false, 0); + v8::MaybeLocal maybe_proxy = CreateProxyForAPI( + api_object, isolated_context, main_context, &object_cache, false, 0); if (maybe_proxy.IsEmpty()) return; auto proxy = maybe_proxy.ToLocalChecked(); @@ -564,8 +543,6 @@ void OverrideGlobalValueFromIsolatedWorld( auto* render_frame = GetRenderFrame(value); CHECK(render_frame); - context_bridge::RenderFrameFunctionStore* store = - GetOrCreateStore(render_frame); auto* frame = render_frame->GetWebFrame(); CHECK(frame); v8::Local main_context = frame->MainWorldScriptContext(); @@ -578,9 +555,9 @@ void OverrideGlobalValueFromIsolatedWorld( { v8::Context::Scope main_context_scope(main_context); context_bridge::ObjectCache object_cache; - v8::MaybeLocal maybe_proxy = PassValueToOtherContext( - value->CreationContext(), main_context, value, store, &object_cache, - support_dynamic_properties, 1); + v8::MaybeLocal maybe_proxy = + PassValueToOtherContext(value->CreationContext(), main_context, value, + &object_cache, support_dynamic_properties, 1); DCHECK(!maybe_proxy.IsEmpty()); auto proxy = maybe_proxy.ToLocalChecked(); @@ -598,8 +575,6 @@ bool OverrideGlobalPropertyFromIsolatedWorld( auto* render_frame = GetRenderFrame(getter); CHECK(render_frame); - context_bridge::RenderFrameFunctionStore* store = - GetOrCreateStore(render_frame); auto* frame = render_frame->GetWebFrame(); CHECK(frame); v8::Local main_context = frame->MainWorldScriptContext(); @@ -618,14 +593,14 @@ bool OverrideGlobalPropertyFromIsolatedWorld( if (!getter->IsNullOrUndefined()) { v8::MaybeLocal maybe_getter_proxy = PassValueToOtherContext(getter->CreationContext(), main_context, - getter, store, &object_cache, false, 1); + getter, &object_cache, false, 1); DCHECK(!maybe_getter_proxy.IsEmpty()); getter_proxy = maybe_getter_proxy.ToLocalChecked(); } if (!setter->IsNullOrUndefined() && setter->IsObject()) { v8::MaybeLocal maybe_setter_proxy = PassValueToOtherContext(getter->CreationContext(), main_context, - setter, store, &object_cache, false, 1); + setter, &object_cache, false, 1); DCHECK(!maybe_setter_proxy.IsEmpty()); setter_proxy = maybe_setter_proxy.ToLocalChecked(); } @@ -667,7 +642,7 @@ void Initialize(v8::Local exports, dict.SetMethod("_isCalledFromMainWorld", &electron::api::IsCalledFromMainWorld); #ifdef DCHECK_IS_ON - dict.SetMethod("_debugGCMaps", &electron::api::DebugGC); + dict.Set("_isDebug", true); #endif } diff --git a/shell/renderer/api/electron_api_context_bridge.h b/shell/renderer/api/electron_api_context_bridge.h index 9c9baf6dee69..509e861c492a 100644 --- a/shell/renderer/api/electron_api_context_bridge.h +++ b/shell/renderer/api/electron_api_context_bridge.h @@ -16,21 +16,12 @@ namespace electron { namespace api { -namespace context_bridge { -class RenderFrameFunctionStore; -} - -v8::Local ProxyFunctionWrapper( - context_bridge::RenderFrameFunctionStore* store, - size_t func_id, - bool support_dynamic_properties, - gin_helper::Arguments* args); +void ProxyFunctionWrapper(const v8::FunctionCallbackInfo& info); v8::MaybeLocal CreateProxyForAPI( const v8::Local& api_object, const v8::Local& source_context, const v8::Local& target_context, - context_bridge::RenderFrameFunctionStore* store, context_bridge::ObjectCache* object_cache, bool support_dynamic_properties, int recursion_depth); diff --git a/spec-main/api-context-bridge-spec.ts b/spec-main/api-context-bridge-spec.ts index f0acc0e5e582..c962bb670437 100644 --- a/spec-main/api-context-bridge-spec.ts +++ b/spec-main/api-context-bridge-spec.ts @@ -78,7 +78,8 @@ describe('contextBridge', () => { contextIsolation: true, nodeIntegration: true, sandbox: useSandbox, - preload: path.resolve(tmpDir, 'preload.js') + preload: path.resolve(tmpDir, 'preload.js'), + additionalArguments: ['--unsafely-expose-electron-internals-for-testing'] } }); await w.loadURL(`http://127.0.0.1:${(server.address() as AddressInfo).port}`); @@ -88,15 +89,20 @@ describe('contextBridge', () => { w.webContents.executeJavaScript(`(${fn.toString()})(window)`); const getGCInfo = async (): Promise<{ - functionCount: number - objectCount: number - liveFromValues: number - liveProxyValues: number + trackedValues: number; }> => { const [, info] = await emittedOnce(ipcMain, 'gc-info', () => w.webContents.send('get-gc-info')); return info; }; + const forceGCOnWindow = async () => { + w.webContents.debugger.attach(); + await w.webContents.debugger.sendCommand('HeapProfiler.enable'); + await w.webContents.debugger.sendCommand('HeapProfiler.collectGarbage'); + await w.webContents.debugger.sendCommand('HeapProfiler.disable'); + w.webContents.debugger.detach(); + }; + it('should proxy numbers', async () => { await makeBindingWindow(() => { contextBridge.exposeInMainWorld('example', { @@ -341,45 +347,56 @@ describe('contextBridge', () => { if (!useSandbox) { it('should release the global hold on methods sent across contexts', async () => { await makeBindingWindow(() => { - require('electron').ipcRenderer.on('get-gc-info', e => e.sender.send('gc-info', contextBridge.debugGC())); + require('electron').ipcRenderer.on('get-gc-info', e => e.sender.send('gc-info', { trackedValues: process.electronBinding('v8_util').getWeaklyTrackedValues().length })); + const { weaklyTrackValue } = process.electronBinding('v8_util'); contextBridge.exposeInMainWorld('example', { - getFunction: () => () => 123 + getFunction: () => () => 123, + track: weaklyTrackValue }); }); await callWithBindings(async (root: any) => { root.GCRunner.run(); }); - const baseValue = (await getGCInfo()).functionCount; + expect((await getGCInfo()).trackedValues).to.equal(0); await callWithBindings(async (root: any) => { - root.x = [root.example.getFunction()]; + const fn = root.example.getFunction(); + root.example.track(fn); + root.x = [fn]; }); - expect((await getGCInfo()).functionCount).to.equal(baseValue + 1); + expect((await getGCInfo()).trackedValues).to.equal(1); await callWithBindings(async (root: any) => { root.x = []; root.GCRunner.run(); }); - expect((await getGCInfo()).functionCount).to.equal(baseValue); + expect((await getGCInfo()).trackedValues).to.equal(0); }); } if (useSandbox) { it('should not leak the global hold on methods sent across contexts when reloading a sandboxed renderer', async () => { await makeBindingWindow(() => { - require('electron').ipcRenderer.on('get-gc-info', e => e.sender.send('gc-info', contextBridge.debugGC())); + require('electron').ipcRenderer.on('get-gc-info', e => e.sender.send('gc-info', { trackedValues: process.electronBinding('v8_util').getWeaklyTrackedValues().length })); + const { weaklyTrackValue } = process.electronBinding('v8_util'); contextBridge.exposeInMainWorld('example', { - getFunction: () => () => 123 + getFunction: () => () => 123, + track: weaklyTrackValue }); require('electron').ipcRenderer.send('window-ready-for-tasking'); }); const loadPromise = emittedOnce(ipcMain, 'window-ready-for-tasking'); - const baseValue = (await getGCInfo()).functionCount; + expect((await getGCInfo()).trackedValues).to.equal(0); + await callWithBindings((root: any) => { + root.example.track(root.example.getFunction()); + }); + expect((await getGCInfo()).trackedValues).to.equal(1); await callWithBindings((root: any) => { root.location.reload(); }); await loadPromise; + await forceGCOnWindow(); // If this is ever "2" it means we leaked the exposed function and // therefore the entire context after a reload - expect((await getGCInfo()).functionCount).to.equal(baseValue); + expect((await getGCInfo()).trackedValues).to.equal(0); }); } diff --git a/typings/internal-ambient.d.ts b/typings/internal-ambient.d.ts index aec2b8da7457..b2062193bb39 100644 --- a/typings/internal-ambient.d.ts +++ b/typings/internal-ambient.d.ts @@ -40,6 +40,9 @@ declare namespace NodeJS { createIDWeakMap(): ElectronInternal.KeyWeakMap; 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[]; } interface Process { diff --git a/typings/internal-electron.d.ts b/typings/internal-electron.d.ts index a2cf9ed8d144..f485a4348ba2 100644 --- a/typings/internal-electron.d.ts +++ b/typings/internal-electron.d.ts @@ -20,7 +20,6 @@ declare namespace Electron { } interface ContextBridge { - debugGC(): { functionCount: number } internalContextBridge: { contextIsolationEnabled: boolean; overrideGlobalValueFromIsolatedWorld(keys: string[], value: any): void;