From 07a049ef1b555d5016e13f92c548c4dc40bdfd93 Mon Sep 17 00:00:00 2001
From: Jeremy Apthorp <jeremya@chromium.org>
Date: Fri, 20 Mar 2020 14:15:55 -0700
Subject: [PATCH] chore: stop leaking v8 environment (#22761)

---
 .../browser/api/electron_api_event_emitter.cc | 12 ++++++----
 shell/browser/electron_browser_main_parts.cc  |  9 --------
 shell/browser/electron_browser_main_parts.h   |  2 +-
 shell/common/gin_helper/destroyable.cc        | 23 ++++++++++++-------
 4 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/shell/browser/api/electron_api_event_emitter.cc b/shell/browser/api/electron_api_event_emitter.cc
index 40dfbe96559..cb9397e4a99 100644
--- a/shell/browser/api/electron_api_event_emitter.cc
+++ b/shell/browser/api/electron_api_event_emitter.cc
@@ -6,6 +6,7 @@
 
 #include "base/bind.h"
 #include "base/callback.h"
+#include "base/no_destructor.h"
 #include "gin/dictionary.h"
 #include "shell/common/gin_converters/callback_converter.h"
 #include "shell/common/node_includes.h"
@@ -13,11 +14,14 @@
 
 namespace {
 
-v8::Global<v8::Object> event_emitter_prototype;
+v8::Global<v8::Object>* GetEventEmitterPrototypeReference() {
+  static base::NoDestructor<v8::Global<v8::Object>> event_emitter_prototype;
+  return event_emitter_prototype.get();
+}
 
 void SetEventEmitterPrototype(v8::Isolate* isolate,
                               v8::Local<v8::Object> proto) {
-  event_emitter_prototype.Reset(isolate, proto);
+  GetEventEmitterPrototypeReference()->Reset(isolate, proto);
 }
 
 void Initialize(v8::Local<v8::Object> exports,
@@ -36,8 +40,8 @@ void Initialize(v8::Local<v8::Object> exports,
 namespace electron {
 
 v8::Local<v8::Object> GetEventEmitterPrototype(v8::Isolate* isolate) {
-  CHECK(!event_emitter_prototype.IsEmpty());
-  return event_emitter_prototype.Get(isolate);
+  CHECK(!GetEventEmitterPrototypeReference()->IsEmpty());
+  return GetEventEmitterPrototypeReference()->Get(isolate);
 }
 
 }  // namespace electron
diff --git a/shell/browser/electron_browser_main_parts.cc b/shell/browser/electron_browser_main_parts.cc
index ecfde4846a6..5363c224e76 100644
--- a/shell/browser/electron_browser_main_parts.cc
+++ b/shell/browser/electron_browser_main_parts.cc
@@ -227,15 +227,6 @@ ElectronBrowserMainParts::ElectronBrowserMainParts(
 
 ElectronBrowserMainParts::~ElectronBrowserMainParts() {
   asar::ClearArchives();
-  // Leak the JavascriptEnvironment on exit.
-  // This is to work around the bug that V8 would be waiting for background
-  // tasks to finish on exit, while somehow it waits forever in Electron, more
-  // about this can be found at
-  // https://github.com/electron/electron/issues/4767. On the other handle there
-  // is actually no need to gracefully shutdown V8 on exit in the main process,
-  // we already ensured all necessary resources get cleaned up, and it would
-  // make quitting faster.
-  ignore_result(js_env_.release());
 }
 
 // static
diff --git a/shell/browser/electron_browser_main_parts.h b/shell/browser/electron_browser_main_parts.h
index 6a72ea90c08..e006eb1a5e8 100644
--- a/shell/browser/electron_browser_main_parts.h
+++ b/shell/browser/electron_browser_main_parts.h
@@ -129,8 +129,8 @@ class ElectronBrowserMainParts : public content::BrowserMainParts {
   // Pointer to exit code.
   int* exit_code_ = nullptr;
 
-  std::unique_ptr<Browser> browser_;
   std::unique_ptr<JavascriptEnvironment> js_env_;
+  std::unique_ptr<Browser> browser_;
   std::unique_ptr<NodeBindings> node_bindings_;
   std::unique_ptr<ElectronBindings> electron_bindings_;
   std::unique_ptr<NodeEnvironment> node_env_;
diff --git a/shell/common/gin_helper/destroyable.cc b/shell/common/gin_helper/destroyable.cc
index 007f40fb7b3..4e77fa91fee 100644
--- a/shell/common/gin_helper/destroyable.cc
+++ b/shell/common/gin_helper/destroyable.cc
@@ -4,6 +4,7 @@
 
 #include "shell/common/gin_helper/destroyable.h"
 
+#include "base/no_destructor.h"
 #include "gin/converter.h"
 #include "shell/common/gin_helper/wrappable_base.h"
 
@@ -11,9 +12,15 @@ namespace gin_helper {
 
 namespace {
 
-// Cached function templates, leaked on exit. (They are leaked in V8 anyway.)
-v8::Global<v8::FunctionTemplate> g_destroy_func;
-v8::Global<v8::FunctionTemplate> g_is_destroyed_func;
+v8::Global<v8::FunctionTemplate>* GetDestroyFunc() {
+  static base::NoDestructor<v8::Global<v8::FunctionTemplate>> destroy_func;
+  return destroy_func.get();
+}
+
+v8::Global<v8::FunctionTemplate>* GetIsDestroyedFunc() {
+  static base::NoDestructor<v8::Global<v8::FunctionTemplate>> is_destroyed_func;
+  return is_destroyed_func.get();
+}
 
 void DestroyFunc(const v8::FunctionCallbackInfo<v8::Value>& info) {
   v8::Local<v8::Object> holder = info.Holder();
@@ -45,22 +52,22 @@ bool Destroyable::IsDestroyed(v8::Local<v8::Object> object) {
 void Destroyable::MakeDestroyable(v8::Isolate* isolate,
                                   v8::Local<v8::FunctionTemplate> prototype) {
   // Cache the FunctionTemplate of "destroy" and "isDestroyed".
-  if (g_destroy_func.IsEmpty()) {
+  if (GetDestroyFunc()->IsEmpty()) {
     auto templ = v8::FunctionTemplate::New(isolate, DestroyFunc);
     templ->RemovePrototype();
-    g_destroy_func.Reset(isolate, templ);
+    GetDestroyFunc()->Reset(isolate, templ);
     templ = v8::FunctionTemplate::New(isolate, IsDestroyedFunc);
     templ->RemovePrototype();
-    g_is_destroyed_func.Reset(isolate, templ);
+    GetIsDestroyedFunc()->Reset(isolate, templ);
   }
 
   auto proto_templ = prototype->PrototypeTemplate();
   proto_templ->Set(
       gin::StringToSymbol(isolate, "destroy"),
-      v8::Local<v8::FunctionTemplate>::New(isolate, g_destroy_func));
+      v8::Local<v8::FunctionTemplate>::New(isolate, *GetDestroyFunc()));
   proto_templ->Set(
       gin::StringToSymbol(isolate, "isDestroyed"),
-      v8::Local<v8::FunctionTemplate>::New(isolate, g_is_destroyed_func));
+      v8::Local<v8::FunctionTemplate>::New(isolate, *GetIsDestroyedFunc()));
 }
 
 }  // namespace gin_helper