From 075472d6ef34e6a7bb78c88a7be0cf26c7d5be5d Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Wed, 29 Apr 2020 15:51:41 -0700 Subject: [PATCH] fix: do not leak IPC or context bridge promises (#23321) --- .../api/electron_api_context_bridge.cc | 48 +++++++++++++------ .../renderer/api/electron_api_ipc_renderer.cc | 30 +++++++++--- 2 files changed, 57 insertions(+), 21 deletions(-) diff --git a/shell/renderer/api/electron_api_context_bridge.cc b/shell/renderer/api/electron_api_context_bridge.cc index 5aa4cb27a3a6..a8478d239cdd 100644 --- a/shell/renderer/api/electron_api_context_bridge.cc +++ b/shell/renderer/api/electron_api_context_bridge.cc @@ -5,6 +5,7 @@ #include "shell/renderer/api/electron_api_context_bridge.h" #include +#include #include #include #include @@ -195,18 +196,32 @@ v8::MaybeLocal PassValueToOtherContext( v8::Context::Scope destination_scope(destination_context); { auto source_promise = v8::Local::Cast(value); - auto* proxied_promise = new gin_helper::Promise>( - destination_context->GetIsolate()); + // Make the promise a shared_ptr so that when the original promise is + // freed the proxy promise is correctly freed as well instead of being + // left dangling + std::shared_ptr>> + proxied_promise(new gin_helper::Promise>( + destination_context->GetIsolate())); v8::Local proxied_promise_handle = proxied_promise->GetHandle(); + v8::Global global_then_source_context( + source_context->GetIsolate(), source_context); + v8::Global global_then_destination_context( + destination_context->GetIsolate(), destination_context); + global_then_source_context.SetWeak(); + global_then_destination_context.SetWeak(); auto then_cb = base::BindOnce( - [](gin_helper::Promise>* proxied_promise, + [](std::shared_ptr>> + 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()) + return; context_bridge::ObjectCache object_cache; auto val = PassValueToOtherContext(global_source_context.Get(isolate), @@ -214,20 +229,28 @@ v8::MaybeLocal PassValueToOtherContext( result, store, &object_cache, false, 0); if (!val.IsEmpty()) proxied_promise->Resolve(val.ToLocalChecked()); - delete proxied_promise; }, proxied_promise, destination_context->GetIsolate(), - v8::Global(source_context->GetIsolate(), source_context), - v8::Global(destination_context->GetIsolate(), - destination_context), - store); + std::move(global_then_source_context), + std::move(global_then_destination_context), store); + + v8::Global global_catch_source_context( + source_context->GetIsolate(), source_context); + v8::Global global_catch_destination_context( + destination_context->GetIsolate(), destination_context); + global_catch_source_context.SetWeak(); + global_catch_destination_context.SetWeak(); auto catch_cb = base::BindOnce( - [](gin_helper::Promise>* proxied_promise, + [](std::shared_ptr>> + 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()) + return; context_bridge::ObjectCache object_cache; auto val = PassValueToOtherContext(global_source_context.Get(isolate), @@ -235,13 +258,10 @@ v8::MaybeLocal PassValueToOtherContext( result, store, &object_cache, false, 0); if (!val.IsEmpty()) proxied_promise->Reject(val.ToLocalChecked()); - delete proxied_promise; }, proxied_promise, destination_context->GetIsolate(), - v8::Global(source_context->GetIsolate(), source_context), - v8::Global(destination_context->GetIsolate(), - destination_context), - store); + std::move(global_catch_source_context), + std::move(global_catch_destination_context), store); ignore_result(source_promise->Then( source_context, diff --git a/shell/renderer/api/electron_api_ipc_renderer.cc b/shell/renderer/api/electron_api_ipc_renderer.cc index 7ab46686e564..a70ebfa240dd 100644 --- a/shell/renderer/api/electron_api_ipc_renderer.cc +++ b/shell/renderer/api/electron_api_ipc_renderer.cc @@ -7,6 +7,7 @@ #include "base/task/post_task.h" #include "base/values.h" #include "content/public/renderer/render_frame.h" +#include "content/public/renderer/render_frame_observer.h" #include "gin/dictionary.h" #include "gin/handle.h" #include "gin/object_template_builder.h" @@ -36,7 +37,8 @@ RenderFrame* GetCurrentRenderFrame() { return RenderFrame::FromWebFrame(frame); } -class IPCRenderer : public gin::Wrappable { +class IPCRenderer : public gin::Wrappable, + public content::RenderFrameObserver { public: static gin::WrapperInfo kWrapperInfo; @@ -44,19 +46,32 @@ class IPCRenderer : public gin::Wrappable { return gin::CreateHandle(isolate, new IPCRenderer(isolate)); } - explicit IPCRenderer(v8::Isolate* isolate) { + explicit IPCRenderer(v8::Isolate* isolate) + : content::RenderFrameObserver(GetCurrentRenderFrame()) { RenderFrame* render_frame = GetCurrentRenderFrame(); DCHECK(render_frame); + weak_context_ = + v8::Global(isolate, isolate->GetCurrentContext()); + weak_context_.SetWeak(); render_frame->GetRemoteInterfaces()->GetInterface( mojo::MakeRequest(&electron_browser_ptr_)); } + void OnDestruct() override { electron_browser_ptr_.reset(); } + + void WillReleaseScriptContext(v8::Local context, + int32_t world_id) override { + if (weak_context_.IsEmpty() || + weak_context_.Get(context->GetIsolate()) == context) + electron_browser_ptr_.reset(); + } + // gin::Wrappable: gin::ObjectTemplateBuilder GetObjectTemplateBuilder( v8::Isolate* isolate) override { return gin::Wrappable::GetObjectTemplateBuilder(isolate) - .SetMethod("send", &IPCRenderer::Send) + .SetMethod("send", &IPCRenderer::SendMessage) .SetMethod("sendSync", &IPCRenderer::SendSync) .SetMethod("sendTo", &IPCRenderer::SendTo) .SetMethod("sendToHost", &IPCRenderer::SendToHost) @@ -67,10 +82,10 @@ class IPCRenderer : public gin::Wrappable { const char* GetTypeName() override { return "IPCRenderer"; } private: - void Send(v8::Isolate* isolate, - bool internal, - const std::string& channel, - v8::Local arguments) { + void SendMessage(v8::Isolate* isolate, + bool internal, + const std::string& channel, + v8::Local arguments) { blink::CloneableMessage message; if (!electron::SerializeV8Value(isolate, arguments, &message)) { return; @@ -175,6 +190,7 @@ class IPCRenderer : public gin::Wrappable { return electron::DeserializeV8Value(isolate, result); } + v8::Global weak_context_; electron::mojom::ElectronBrowserPtr electron_browser_ptr_; };