From 19314d3cafc5ec1c4b1f1316b3f5d9744f8e9604 Mon Sep 17 00:00:00 2001 From: Jeremy Apthorp Date: Tue, 10 Mar 2020 18:16:58 -0700 Subject: [PATCH] fix: remove catch-all HandleScope (#22531) --- shell/app/node_main.cc | 84 ++++++++++--------- shell/browser/api/electron_api_app.cc | 7 +- shell/browser/api/electron_api_cookies.cc | 1 + shell/browser/api/electron_api_menu.cc | 1 + .../electron_api_service_worker_context.cc | 4 +- shell/browser/api/electron_api_url_loader.cc | 1 + shell/browser/api/event.cc | 13 ++- shell/browser/electron_autofill_driver.cc | 7 +- shell/browser/electron_browser_client.cc | 3 + shell/browser/electron_browser_main_parts.cc | 2 + shell/browser/electron_navigation_throttle.cc | 5 +- shell/browser/javascript_environment.cc | 20 +++-- shell/browser/javascript_environment.h | 3 - shell/browser/login_handler.cc | 3 +- shell/browser/net/node_stream_loader.cc | 1 + shell/browser/ui/file_dialog_mac.mm | 2 + shell/common/gin_helper/event_emitter.cc | 1 + shell/common/gin_helper/trackable_object.h | 4 + shell/common/gin_helper/wrappable.cc | 27 +++--- shell/common/gin_helper/wrappable_base.h | 3 +- spec-main/api-ipc-spec.ts | 14 ++++ 21 files changed, 131 insertions(+), 75 deletions(-) diff --git a/shell/app/node_main.cc b/shell/app/node_main.cc index 25f293f64d66..78017e6d8359 100644 --- a/shell/app/node_main.cc +++ b/shell/app/node_main.cc @@ -139,55 +139,59 @@ int NodeMain(int argc, char* argv[]) { JavascriptEnvironment gin_env(loop); v8::Isolate* isolate = gin_env.isolate(); - - node::IsolateData* isolate_data = - node::CreateIsolateData(isolate, loop, gin_env.platform()); - CHECK_NE(nullptr, isolate_data); - - v8::Locker locker(isolate); v8::Isolate::Scope isolate_scope(isolate); - v8::HandleScope handle_scope(isolate); + v8::Locker locker(isolate); + node::Environment* env = nullptr; + node::IsolateData* isolate_data = nullptr; + { + v8::HandleScope scope(isolate); - node::Environment* env = node::CreateEnvironment( - isolate_data, gin_env.context(), argc, argv, exec_argc, exec_argv); - CHECK_NE(nullptr, env); + isolate_data = node::CreateIsolateData(isolate, loop, gin_env.platform()); + CHECK_NE(nullptr, isolate_data); + + env = node::CreateEnvironment(isolate_data, gin_env.context(), argc, argv, + exec_argc, exec_argv); + CHECK_NE(nullptr, env); + + // TODO(codebytere): we shouldn't have to call this - upstream? + env->InitializeDiagnostics(); + + // This is needed in order to enable v8 host weakref hooks. + // TODO(codebytere): we shouldn't have to call this - upstream? + isolate->SetHostCleanupFinalizationGroupCallback( + HostCleanupFinalizationGroupCallback); + + gin_helper::Dictionary process(isolate, env->process_object()); +#if defined(OS_WIN) + process.SetMethod("log", &ElectronBindings::Log); +#endif + process.SetMethod("crash", &ElectronBindings::Crash); + + // Setup process.crashReporter.start in child node processes + gin_helper::Dictionary reporter = gin::Dictionary::CreateEmpty(isolate); + reporter.SetMethod("start", + &crash_reporter::CrashReporter::StartInstance); + +#if !defined(OS_LINUX) + reporter.SetMethod("addExtraParameter", &AddExtraParameter); + reporter.SetMethod("removeExtraParameter", &RemoveExtraParameter); +#endif + + process.Set("crashReporter", reporter); + + gin_helper::Dictionary versions; + if (process.Get("versions", &versions)) { + versions.SetReadOnly(ELECTRON_PROJECT_NAME, ELECTRON_VERSION_STRING); + } + } // Enable support for v8 inspector. NodeDebugger node_debugger(env); node_debugger.Start(); - // TODO(codebytere): we shouldn't have to call this - upstream? - env->InitializeDiagnostics(); - - // This is needed in order to enable v8 host weakref hooks. - // TODO(codebytere): we shouldn't have to call this - upstream? - isolate->SetHostCleanupFinalizationGroupCallback( - HostCleanupFinalizationGroupCallback); - - gin_helper::Dictionary process(isolate, env->process_object()); -#if defined(OS_WIN) - process.SetMethod("log", &ElectronBindings::Log); -#endif - process.SetMethod("crash", &ElectronBindings::Crash); - - // Setup process.crashReporter.start in child node processes - gin_helper::Dictionary reporter = gin::Dictionary::CreateEmpty(isolate); - reporter.SetMethod("start", &crash_reporter::CrashReporter::StartInstance); - -#if !defined(OS_LINUX) - reporter.SetMethod("addExtraParameter", &AddExtraParameter); - reporter.SetMethod("removeExtraParameter", &RemoveExtraParameter); -#endif - - process.Set("crashReporter", reporter); - - gin_helper::Dictionary versions; - if (process.Get("versions", &versions)) { - versions.SetReadOnly(ELECTRON_PROJECT_NAME, ELECTRON_VERSION_STRING); - } - // TODO(codebytere): we should try to handle this upstream. { + v8::HandleScope scope(isolate); node::InternalCallbackScope callback_scope( env, v8::Local(), {1, 0}, node::InternalCallbackScope::kAllowEmptyResource | diff --git a/shell/browser/api/electron_api_app.cc b/shell/browser/api/electron_api_app.cc index 20b6d8b0ce59..f3ac46c61cb4 100644 --- a/shell/browser/api/electron_api_app.cc +++ b/shell/browser/api/electron_api_app.cc @@ -788,8 +788,11 @@ void App::RenderProcessReady(content::RenderProcessHost* host) { // `RenderProcessPreferences`, so this is at least more explicit... content::WebContents* web_contents = ElectronBrowserClient::Get()->GetWebContentsFromProcessID(host->GetID()); - if (web_contents) - WebContents::FromOrCreate(v8::Isolate::GetCurrent(), web_contents); + if (web_contents) { + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + v8::HandleScope scope(isolate); + WebContents::FromOrCreate(isolate, web_contents); + } } void App::RenderProcessDisconnected(base::ProcessId host_pid) { diff --git a/shell/browser/api/electron_api_cookies.cc b/shell/browser/api/electron_api_cookies.cc index 31515b6c7307..88a899f9cde9 100644 --- a/shell/browser/api/electron_api_cookies.cc +++ b/shell/browser/api/electron_api_cookies.cc @@ -314,6 +314,7 @@ v8::Local Cookies::FlushStore() { } void Cookies::OnCookieChanged(const net::CookieChangeInfo& change) { + v8::HandleScope scope(isolate()); Emit("changed", gin::ConvertToV8(isolate(), change.cookie), gin::ConvertToV8(isolate(), change.cause), gin::ConvertToV8(isolate(), diff --git a/shell/browser/api/electron_api_menu.cc b/shell/browser/api/electron_api_menu.cc index c3f781293360..b952496b2f08 100644 --- a/shell/browser/api/electron_api_menu.cc +++ b/shell/browser/api/electron_api_menu.cc @@ -214,6 +214,7 @@ void Menu::OnMenuWillClose() { } void Menu::OnMenuWillShow() { + v8::HandleScope scope(isolate()); g_menus[weak_map_id()] = v8::Global(isolate(), GetWrapper()); Emit("menu-will-show"); } diff --git a/shell/browser/api/electron_api_service_worker_context.cc b/shell/browser/api/electron_api_service_worker_context.cc index 5f9f157c5b0f..4dc1d2cdf29e 100644 --- a/shell/browser/api/electron_api_service_worker_context.cc +++ b/shell/browser/api/electron_api_service_worker_context.cc @@ -85,8 +85,10 @@ ServiceWorkerContext::~ServiceWorkerContext() { void ServiceWorkerContext::OnReportConsoleMessage( int64_t version_id, const content::ConsoleMessage& message) { + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + v8::HandleScope scope(isolate); Emit("console-message", - gin::DataObjectBuilder(v8::Isolate::GetCurrent()) + gin::DataObjectBuilder(isolate) .Set("versionId", version_id) .Set("source", MessageSourceToString(message.source)) .Set("level", static_cast(message.message_level)) diff --git a/shell/browser/api/electron_api_url_loader.cc b/shell/browser/api/electron_api_url_loader.cc index 9735c301d7f3..b8aa5f1928e6 100644 --- a/shell/browser/api/electron_api_url_loader.cc +++ b/shell/browser/api/electron_api_url_loader.cc @@ -433,6 +433,7 @@ void SimpleURLLoaderWrapper::OnRetry(base::OnceClosure start_retry) {} void SimpleURLLoaderWrapper::OnResponseStarted( const GURL& final_url, const network::mojom::URLResponseHead& response_head) { + v8::HandleScope scope(isolate()); gin::Dictionary dict = gin::Dictionary::CreateEmpty(isolate()); dict.Set("statusCode", response_head.headers->response_code()); dict.Set("statusMessage", response_head.headers->GetStatusText()); diff --git a/shell/browser/api/event.cc b/shell/browser/api/event.cc index da8af70d7e74..4c19b974510c 100644 --- a/shell/browser/api/event.cc +++ b/shell/browser/api/event.cc @@ -6,8 +6,10 @@ #include +#include "gin/data_object_builder.h" #include "gin/object_template_builder.h" #include "shell/common/gin_converters/blink_converter.h" +#include "shell/common/gin_converters/std_converter.h" namespace gin_helper { @@ -15,7 +17,16 @@ gin::WrapperInfo Event::kWrapperInfo = {gin::kEmbedderNativeGin}; Event::Event() {} -Event::~Event() = default; +Event::~Event() { + if (callback_) { + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + v8::HandleScope scope(isolate); + auto message = gin::DataObjectBuilder(isolate) + .Set("error", "reply was never sent") + .Build(); + SendReply(isolate, message); + } +} void Event::SetCallback(InvokeCallback callback) { DCHECK(!callback_); diff --git a/shell/browser/electron_autofill_driver.cc b/shell/browser/electron_autofill_driver.cc index 4567816ee3da..6ce958861dae 100644 --- a/shell/browser/electron_autofill_driver.cc +++ b/shell/browser/electron_autofill_driver.cc @@ -28,10 +28,11 @@ void AutofillDriver::ShowAutofillPopup( const gfx::RectF& bounds, const std::vector& values, const std::vector& labels) { + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + v8::HandleScope scope(isolate); auto* web_contents = - api::WebContents::From( - v8::Isolate::GetCurrent(), - content::WebContents::FromRenderFrameHost(render_frame_host_)) + api::WebContents::From(isolate, content::WebContents::FromRenderFrameHost( + render_frame_host_)) .get(); if (!web_contents || !web_contents->owner_window()) return; diff --git a/shell/browser/electron_browser_client.cc b/shell/browser/electron_browser_client.cc index a49749faede6..a2fdc43f7cd9 100644 --- a/shell/browser/electron_browser_client.cc +++ b/shell/browser/electron_browser_client.cc @@ -1300,6 +1300,7 @@ bool ElectronBrowserClient::WillInterceptWebSocket( return false; v8::Isolate* isolate = v8::Isolate::GetCurrent(); + v8::HandleScope scope(isolate); auto* browser_context = frame->GetProcess()->GetBrowserContext(); auto web_request = api::WebRequest::FromOrCreate(isolate, browser_context); @@ -1320,6 +1321,7 @@ void ElectronBrowserClient::CreateWebSocket( mojo::PendingRemote handshake_client) { v8::Isolate* isolate = v8::Isolate::GetCurrent(); + v8::HandleScope scope(isolate); auto* browser_context = frame->GetProcess()->GetBrowserContext(); auto web_request = api::WebRequest::FromOrCreate(isolate, browser_context); DCHECK(web_request.get()); @@ -1345,6 +1347,7 @@ bool ElectronBrowserClient::WillCreateURLLoaderFactory( bool* disable_secure_dns, network::mojom::URLLoaderFactoryOverridePtr* factory_override) { v8::Isolate* isolate = v8::Isolate::GetCurrent(); + v8::HandleScope scope(isolate); api::Protocol* protocol = api::Protocol::FromWrappedClass(isolate, browser_context); DCHECK(protocol); diff --git a/shell/browser/electron_browser_main_parts.cc b/shell/browser/electron_browser_main_parts.cc index e026169142c2..7550b1f552e6 100644 --- a/shell/browser/electron_browser_main_parts.cc +++ b/shell/browser/electron_browser_main_parts.cc @@ -293,6 +293,8 @@ void ElectronBrowserMainParts::PostEarlyInitialization() { // avoid conflicts we only initialize our V8 environment after that. js_env_ = std::make_unique(node_bindings_->uv_loop()); + v8::HandleScope scope(js_env_->isolate()); + node_bindings_->Initialize(); // Create the global environment. node::Environment* env = node_bindings_->CreateEnvironment( diff --git a/shell/browser/electron_navigation_throttle.cc b/shell/browser/electron_navigation_throttle.cc index 5c0ef054421c..cc72510510f4 100644 --- a/shell/browser/electron_navigation_throttle.cc +++ b/shell/browser/electron_navigation_throttle.cc @@ -28,8 +28,9 @@ ElectronNavigationThrottle::WillRedirectRequest() { return PROCEED; } - auto api_contents = - electron::api::WebContents::From(v8::Isolate::GetCurrent(), contents); + v8::Isolate* isolate = v8::Isolate::GetCurrent(); + v8::HandleScope scope(isolate); + auto api_contents = electron::api::WebContents::From(isolate, contents); if (api_contents.IsEmpty()) { // No need to emit any event if the WebContents is not available in JS. return PROCEED; diff --git a/shell/browser/javascript_environment.cc b/shell/browser/javascript_environment.cc index d040ac39f2a3..746c5a1eeea0 100644 --- a/shell/browser/javascript_environment.cc +++ b/shell/browser/javascript_environment.cc @@ -29,13 +29,21 @@ JavascriptEnvironment::JavascriptEnvironment(uv_loop_t* event_loop) gin::IsolateHolder::IsolateType::kUtility, gin::IsolateHolder::IsolateCreationMode::kNormal, isolate_), - isolate_scope_(isolate_), - locker_(isolate_), - handle_scope_(isolate_), - context_(isolate_, node::NewContext(isolate_)), - context_scope_(v8::Local::New(isolate_, context_)) {} + locker_(isolate_) { + isolate_->Enter(); + v8::HandleScope scope(isolate_); + auto context = node::NewContext(isolate_); + context_ = v8::Global(isolate_, context); + context->Enter(); +} -JavascriptEnvironment::~JavascriptEnvironment() = default; +JavascriptEnvironment::~JavascriptEnvironment() { + { + v8::HandleScope scope(isolate_); + context_.Get(isolate_)->Exit(); + } + isolate_->Exit(); +} v8::Isolate* JavascriptEnvironment::Initialize(uv_loop_t* event_loop) { auto* cmd = base::CommandLine::ForCurrentProcess(); diff --git a/shell/browser/javascript_environment.h b/shell/browser/javascript_environment.h index 67a21a038665..183bf1b4e311 100644 --- a/shell/browser/javascript_environment.h +++ b/shell/browser/javascript_environment.h @@ -41,11 +41,8 @@ class JavascriptEnvironment { v8::Isolate* isolate_; gin::IsolateHolder isolate_holder_; - v8::Isolate::Scope isolate_scope_; v8::Locker locker_; - v8::HandleScope handle_scope_; v8::Global context_; - v8::Context::Scope context_scope_; std::unique_ptr microtasks_runner_; diff --git a/shell/browser/login_handler.cc b/shell/browser/login_handler.cc index 1b6c3c529deb..9bd887756f6a 100644 --- a/shell/browser/login_handler.cc +++ b/shell/browser/login_handler.cc @@ -51,6 +51,7 @@ void LoginHandler::EmitEvent( scoped_refptr response_headers, bool first_auth_attempt) { v8::Isolate* isolate = v8::Isolate::GetCurrent(); + v8::HandleScope scope(isolate); auto api_web_contents = api::WebContents::From(isolate, web_contents()); if (api_web_contents.IsEmpty()) { @@ -58,8 +59,6 @@ void LoginHandler::EmitEvent( return; } - v8::HandleScope scope(isolate); - auto details = gin::Dictionary::CreateEmpty(isolate); details.Set("url", url); diff --git a/shell/browser/net/node_stream_loader.cc b/shell/browser/net/node_stream_loader.cc index da43d782af2b..2b3b8414b813 100644 --- a/shell/browser/net/node_stream_loader.cc +++ b/shell/browser/net/node_stream_loader.cc @@ -92,6 +92,7 @@ void NodeStreamLoader::ReadMore() { } is_reading_ = true; auto weak = weak_factory_.GetWeakPtr(); + v8::HandleScope scope(isolate_); // buffer = emitter.read() v8::MaybeLocal ret = node::MakeCallback( isolate_, emitter_.Get(isolate_), "read", 0, nullptr, {0, 0}); diff --git a/shell/browser/ui/file_dialog_mac.mm b/shell/browser/ui/file_dialog_mac.mm index 08bef054cd9d..1ac4394de477 100644 --- a/shell/browser/ui/file_dialog_mac.mm +++ b/shell/browser/ui/file_dialog_mac.mm @@ -302,6 +302,7 @@ void OpenDialogCompletion(int chosen, NSOpenPanel* dialog, bool security_scoped_bookmarks, gin_helper::Promise promise) { + v8::HandleScope scope(promise.isolate()); gin_helper::Dictionary dict = gin::Dictionary::CreateEmpty(promise.isolate()); if (chosen == NSFileHandlingPanelCancelButton) { dict.Set("canceled", true); @@ -379,6 +380,7 @@ void SaveDialogCompletion(int chosen, NSSavePanel* dialog, bool security_scoped_bookmarks, gin_helper::Promise promise) { + v8::HandleScope scope(promise.isolate()); gin_helper::Dictionary dict = gin::Dictionary::CreateEmpty(promise.isolate()); if (chosen == NSFileHandlingPanelCancelButton) { dict.Set("canceled", true); diff --git a/shell/common/gin_helper/event_emitter.cc b/shell/common/gin_helper/event_emitter.cc index 0737b48802ea..d4d5ef712601 100644 --- a/shell/common/gin_helper/event_emitter.cc +++ b/shell/common/gin_helper/event_emitter.cc @@ -38,6 +38,7 @@ v8::Local CreateEvent(v8::Isolate* isolate, } v8::Local context = isolate->GetCurrentContext(); + CHECK(!context.IsEmpty()); v8::Local event = v8::Local::New(isolate, event_template) ->NewInstance(context) diff --git a/shell/common/gin_helper/trackable_object.h b/shell/common/gin_helper/trackable_object.h index 3a8ad50681e4..f9a56cf15135 100644 --- a/shell/common/gin_helper/trackable_object.h +++ b/shell/common/gin_helper/trackable_object.h @@ -55,13 +55,16 @@ class TrackableObject : public TrackableObjectBase, public EventEmitter { public: // Mark the JS object as destroyed. void MarkDestroyed() { + v8::HandleScope scope(gin_helper::Wrappable::isolate()); v8::Local wrapper = gin_helper::Wrappable::GetWrapper(); if (!wrapper.IsEmpty()) { wrapper->SetAlignedPointerInInternalField(0, nullptr); + gin_helper::WrappableBase::wrapper_.ClearWeak(); } } bool IsDestroyed() { + v8::HandleScope scope(gin_helper::Wrappable::isolate()); v8::Local wrapper = gin_helper::Wrappable::GetWrapper(); return wrapper->InternalFieldCount() == 0 || wrapper->GetAlignedPointerFromInternalField(0) == nullptr; @@ -72,6 +75,7 @@ class TrackableObject : public TrackableObjectBase, public EventEmitter { if (!weak_map_) return nullptr; + v8::HandleScope scope(isolate); v8::MaybeLocal object = weak_map_->Get(isolate, id); if (object.IsEmpty()) return nullptr; diff --git a/shell/common/gin_helper/wrappable.cc b/shell/common/gin_helper/wrappable.cc index 5a3ecd1081a0..7b288e9ac631 100644 --- a/shell/common/gin_helper/wrappable.cc +++ b/shell/common/gin_helper/wrappable.cc @@ -16,6 +16,7 @@ WrappableBase::~WrappableBase() { if (wrapper_.IsEmpty()) return; + v8::HandleScope scope(isolate()); GetWrapper()->SetAlignedPointerInInternalField(0, nullptr); wrapper_.ClearWeak(); wrapper_.Reset(); @@ -49,7 +50,8 @@ void WrappableBase::InitWith(v8::Isolate* isolate, isolate_ = isolate; wrapper->SetAlignedPointerInInternalField(0, this); wrapper_.Reset(isolate, wrapper); - wrapper_.SetWeak(this, FirstWeakCallback, v8::WeakCallbackType::kParameter); + wrapper_.SetWeak(this, FirstWeakCallback, + v8::WeakCallbackType::kInternalFields); // Call object._init if we have one. v8::Local init; @@ -62,24 +64,21 @@ void WrappableBase::InitWith(v8::Isolate* isolate, // static void WrappableBase::FirstWeakCallback( const v8::WeakCallbackInfo& data) { - WrappableBase* wrappable = data.GetParameter(); - wrappable->wrapper_.Reset(); - data.SetSecondPassCallback(SecondWeakCallback); + WrappableBase* wrappable = + static_cast(data.GetInternalField(0)); + if (wrappable) { + wrappable->wrapper_.Reset(); + data.SetSecondPassCallback(SecondWeakCallback); + } } // static void WrappableBase::SecondWeakCallback( const v8::WeakCallbackInfo& data) { - // Certain classes (for example api::WebContents and api::WebContentsView) - // are running JS code in the destructor, while V8 may crash when JS code - // runs inside weak callback. - // - // We work around this problem by delaying the deletion to next tick where - // garbage collection is done. - base::ThreadTaskRunnerHandle::Get()->PostNonNestableTask( - FROM_HERE, - base::BindOnce([](WrappableBase* wrappable) { delete wrappable; }, - base::Unretained(data.GetParameter()))); + WrappableBase* wrappable = + static_cast(data.GetInternalField(0)); + if (wrappable) + delete wrappable; } namespace internal { diff --git a/shell/common/gin_helper/wrappable_base.h b/shell/common/gin_helper/wrappable_base.h index e5c84512138e..40d227f79d11 100644 --- a/shell/common/gin_helper/wrappable_base.h +++ b/shell/common/gin_helper/wrappable_base.h @@ -52,6 +52,8 @@ class WrappableBase { // Helper to init with arguments. void InitWithArgs(gin::Arguments* args); + v8::Global wrapper_; // Weak + private: static void FirstWeakCallback( const v8::WeakCallbackInfo& data); @@ -59,7 +61,6 @@ class WrappableBase { const v8::WeakCallbackInfo& data); v8::Isolate* isolate_ = nullptr; - v8::Global wrapper_; // Weak DISALLOW_COPY_AND_ASSIGN(WrappableBase); }; diff --git a/spec-main/api-ipc-spec.ts b/spec-main/api-ipc-spec.ts index 43d505008003..518ed8b46e27 100644 --- a/spec-main/api-ipc-spec.ts +++ b/spec-main/api-ipc-spec.ts @@ -1,5 +1,8 @@ import { expect } from 'chai' import { BrowserWindow, ipcMain, IpcMainInvokeEvent } from 'electron' +import { emittedOnce } from './events-helpers' + +const v8Util = process.electronBinding('v8_util') describe('ipc module', () => { describe('invoke', () => { @@ -103,6 +106,17 @@ describe('ipc module', () => { ipcMain.removeHandler('test') } }) + + it('throws an error in the renderer if the reply callback is dropped', async () => { + // eslint-disable-next-line @typescript-eslint/no-unused-vars + ipcMain.handleOnce('test', () => new Promise(resolve => { + setTimeout(() => v8Util.requestGarbageCollectionForTesting()) + /* never resolve */ + })) + w.webContents.executeJavaScript(`(${rendererInvoke})()`) + const [, { error }] = await emittedOnce(ipcMain, 'result') + expect(error).to.match(/reply was never sent/) + }) }) describe('ordering', () => {