Make sure V8 Function passed to native code are destroyed on UI thread

atom/common/native_mate_converters/callback.cc

@@ -5,6 +5,9 @@
 #include "atom/common/native_mate_converters/callback.h"
 
 #include "atom/browser/atom_browser_main_parts.h"
+#include "content/public/browser/browser_thread.h"
+
+using content::BrowserThread;
 
 namespace mate {
 
@@ -56,31 +59,72 @@ v8::Local<v8::Value> BindFunctionWith(v8::Isolate* isolate,
 
 }  // namespace
 
+// Destroy the class on UI thread when possible.
+struct DeleteOnUIThread {
+  template<typename T>
+  static void Destruct(const T* x) {
+    if (Locker::IsBrowserProcess() &&
+        !BrowserThread::CurrentlyOn(BrowserThread::UI)) {
+      BrowserThread::DeleteSoon(BrowserThread::UI, FROM_HERE, x);
+    } else {
+      delete x;
+    }
+  }
+};
+
+// Like v8::Global, but ref-counted.
+template<typename T>
+class RefCountedGlobal : public base::RefCountedThreadSafe<RefCountedGlobal<T>,
+                                                           DeleteOnUIThread> {
+ public:
+  RefCountedGlobal(v8::Isolate* isolate, v8::Local<v8::Value> value)
+      : handle_(isolate, v8::Local<T>::Cast(value)),
+        weak_factory_(this) {
+    // In browser process, we need to ensure the V8 handle is destroyed before
+    // the JavaScript env gets destroyed.
+    if (Locker::IsBrowserProcess() && atom::AtomBrowserMainParts::Get()) {
+      atom::AtomBrowserMainParts::Get()->RegisterDestructionCallback(
+          base::Bind(&RefCountedGlobal<T>::FreeHandle,
+                     weak_factory_.GetWeakPtr()));
+    }
+  }
+
+  bool IsAlive() const {
+    return !handle_.IsEmpty();
+  }
+
+  v8::Local<T> NewHandle(v8::Isolate* isolate) const {
+    return v8::Local<T>::New(isolate, handle_);
+  }
+
+ private:
+  void FreeHandle() {
+    handle_.Reset();
+  }
+
+  v8::Global<T> handle_;
+  base::WeakPtrFactory<RefCountedGlobal<T>> weak_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(RefCountedGlobal);
+};
+
 SafeV8Function::SafeV8Function(v8::Isolate* isolate, v8::Local<v8::Value> value)
-    : v8_function_(new RefCountedPersistent<v8::Function>(isolate, value)),
+    : v8_function_(new RefCountedGlobal<v8::Function>(isolate, value)) {
-      weak_factory_(this) {
-  Init();
 }
 
 SafeV8Function::SafeV8Function(const SafeV8Function& other)
-    : v8_function_(other.v8_function_),
+    : v8_function_(other.v8_function_) {
-      weak_factory_(this) {
-  Init();
 }
 
-v8::Local<v8::Function> SafeV8Function::NewHandle() const {
+SafeV8Function::~SafeV8Function() {
-  return v8_function_->NewHandle();
 }
 
-void SafeV8Function::Init() {
+bool SafeV8Function::IsAlive() const {
-  if (Locker::IsBrowserProcess() && atom::AtomBrowserMainParts::Get())
+  return v8_function_.get() && v8_function_->IsAlive();
-    atom::AtomBrowserMainParts::Get()->RegisterDestructionCallback(
-        base::Bind(&SafeV8Function::FreeHandle, weak_factory_.GetWeakPtr()));
 }
 
-void SafeV8Function::FreeHandle() {
+v8::Local<v8::Function> SafeV8Function::NewHandle(v8::Isolate* isolate) const {
-  Locker locker(v8_function_->isolate());
+  return v8_function_->NewHandle(isolate);
-  v8_function_ = nullptr;
 }
 
 v8::Local<v8::Value> CreateFunctionFromTranslater(

atom/common/native_mate_converters/callback.h

@@ -19,23 +19,21 @@ namespace mate {
 
 namespace internal {
 
-// Manages the V8 function with RAII, and automatically cleans the handle when
+template<typename T>
-// JavaScript context is destroyed, even when the class is not destroyed.
+class RefCountedGlobal;
+
+// Manages the V8 function with RAII.
 class SafeV8Function {
  public:
   SafeV8Function(v8::Isolate* isolate, v8::Local<v8::Value> value);
   SafeV8Function(const SafeV8Function& other);
+  ~SafeV8Function();
 
-  bool is_alive() const { return v8_function_.get(); }
+  bool IsAlive() const;
-
+  v8::Local<v8::Function> NewHandle(v8::Isolate* isolate) const;
-  v8::Local<v8::Function> NewHandle() const;
 
  private:
-  void Init();
+  scoped_refptr<RefCountedGlobal<v8::Function>> v8_function_;
-  void FreeHandle();
-
-  scoped_refptr<RefCountedPersistent<v8::Function>> v8_function_;
-  base::WeakPtrFactory<SafeV8Function> weak_factory_;
 };
 
 // Helper to invoke a V8 function with C++ parameters.
@@ -49,12 +47,12 @@ struct V8FunctionInvoker<v8::Local<v8::Value>(ArgTypes...)> {
                                  ArgTypes... raw) {
     Locker locker(isolate);
     v8::EscapableHandleScope handle_scope(isolate);
-    if (!function.is_alive())
+    if (!function.IsAlive())
       return v8::Null(isolate);
     scoped_ptr<blink::WebScopedRunV8Script> script_scope(
         Locker::IsBrowserProcess() ?
         nullptr : new blink::WebScopedRunV8Script(isolate));
-    v8::Local<v8::Function> holder = function.NewHandle();
+    v8::Local<v8::Function> holder = function.NewHandle(isolate);
     v8::Local<v8::Context> context = holder->CreationContext();
     v8::Context::Scope context_scope(context);
     std::vector<v8::Local<v8::Value>> args = { ConvertToV8(isolate, raw)... };
@@ -70,12 +68,12 @@ struct V8FunctionInvoker<void(ArgTypes...)> {
                  ArgTypes... raw) {
     Locker locker(isolate);
     v8::HandleScope handle_scope(isolate);
-    if (!function.is_alive())
+    if (!function.IsAlive())
       return;
     scoped_ptr<blink::WebScopedRunV8Script> script_scope(
         Locker::IsBrowserProcess() ?
         nullptr : new blink::WebScopedRunV8Script(isolate));
-    v8::Local<v8::Function> holder = function.NewHandle();
+    v8::Local<v8::Function> holder = function.NewHandle(isolate);
     v8::Local<v8::Context> context = holder->CreationContext();
     v8::Context::Scope context_scope(context);
     std::vector<v8::Local<v8::Value>> args = { ConvertToV8(isolate, raw)... };
@@ -91,12 +89,12 @@ struct V8FunctionInvoker<ReturnType(ArgTypes...)> {
     Locker locker(isolate);
     v8::HandleScope handle_scope(isolate);
     ReturnType ret = ReturnType();
-    if (!function.is_alive())
+    if (!function.IsAlive())
       return ret;
     scoped_ptr<blink::WebScopedRunV8Script> script_scope(
         Locker::IsBrowserProcess() ?
         nullptr : new blink::WebScopedRunV8Script(isolate));
-    v8::Local<v8::Function> holder = function.NewHandle();
+    v8::Local<v8::Function> holder = function.NewHandle(isolate);
     v8::Local<v8::Context> context = holder->CreationContext();
     v8::Context::Scope context_scope(context);
     std::vector<v8::Local<v8::Value>> args = { ConvertToV8(isolate, raw)... };