Manage teardown with shared_ptr not CountDownLatch

It's hard to manage object lifecycle in the C++ side of PyMiniRacer
because objects are often referenced in callbacks sent into the Isolate
message pump. Before we tried to explicitly manage lifecycle, and used
a CountDownLatch to ensure all the async work was done before closing
out the Isolate. In this change, we just switch everything to shared_ptr
and let the Isolate shut itself down when nothing else refers to it.

src/v8_py_frontend/BUILD.gn

@@ -16,8 +16,6 @@ v8_shared_library("mini_racer") {
     "context_factory.cc",
     "context_holder.h",
     "context_holder.cc",
-    "count_down_latch.h",
-    "count_down_latch.cc",
     "gsl_stub.h",
     "heap_reporter.h",
     "heap_reporter.cc",

src/v8_py_frontend/binary_value.cc

@@ -16,21 +16,23 @@
 #include <sstream>
 #include <string>
 #include <string_view>
+#include <utility>
 #include "isolate_manager.h"
 
 namespace MiniRacer {
 
 IsolateObjectDeleter::IsolateObjectDeleter() : isolate_manager_(nullptr) {}
 
-IsolateObjectDeleter::IsolateObjectDeleter(IsolateManager* isolate_manager)
-    : isolate_manager_(isolate_manager) {}
+IsolateObjectDeleter::IsolateObjectDeleter(
+    std::shared_ptr<IsolateManager> isolate_manager)
+    : isolate_manager_(std::move(isolate_manager)) {}
 
 // NOLINTBEGIN(cppcoreguidelines-pro-type-union-access)
 
 BinaryValue::BinaryValue(IsolateObjectDeleter isolate_object_deleter,
                          v8::Local<v8::Context> context,
                          v8::Local<v8::Value> value)
-    : isolate_object_deleter_(isolate_object_deleter) {
+    : isolate_object_deleter_(std::move(isolate_object_deleter)) {
   if (value->IsNull()) {
     handle_.type = type_null;
   } else if (value->IsUndefined()) {
@@ -91,7 +93,7 @@ BinaryValue::BinaryValue(IsolateObjectDeleter isolate_object_deleter,
 BinaryValue::BinaryValue(IsolateObjectDeleter isolate_object_deleter,
                          std::string_view val,
                          BinaryTypes type)
-    : isolate_object_deleter_(isolate_object_deleter) {
+    : isolate_object_deleter_(std::move(isolate_object_deleter)) {
   handle_.len = val.size();
   handle_.type = type;
   msg_.resize(handle_.len + 1);
@@ -101,7 +103,7 @@ BinaryValue::BinaryValue(IsolateObjectDeleter isolate_object_deleter,
 }
 
 BinaryValue::BinaryValue(IsolateObjectDeleter isolate_object_deleter, bool val)
-    : isolate_object_deleter_(isolate_object_deleter) {
+    : isolate_object_deleter_(std::move(isolate_object_deleter)) {
   handle_.len = 0;
   handle_.type = type_bool;
   handle_.int_val = val ? 1 : 0;
@@ -110,7 +112,7 @@ BinaryValue::BinaryValue(IsolateObjectDeleter isolate_object_deleter, bool val)
 BinaryValue::BinaryValue(IsolateObjectDeleter isolate_object_deleter,
                          int64_t val,
                          BinaryTypes type)
-    : isolate_object_deleter_(isolate_object_deleter) {
+    : isolate_object_deleter_(std::move(isolate_object_deleter)) {
   handle_.len = 0;
   handle_.type = type;
   handle_.int_val = val;
@@ -119,7 +121,7 @@ BinaryValue::BinaryValue(IsolateObjectDeleter isolate_object_deleter,
 BinaryValue::BinaryValue(IsolateObjectDeleter isolate_object_deleter,
                          double val,
                          BinaryTypes type)
-    : isolate_object_deleter_(isolate_object_deleter) {
+    : isolate_object_deleter_(std::move(isolate_object_deleter)) {
   handle_.len = 0;
   handle_.type = type;
   handle_.double_val = val;
@@ -195,7 +197,7 @@ BinaryValue::BinaryValue(IsolateObjectDeleter isolate_object_deleter,
                          v8::Local<v8::Message> message,
                          v8::Local<v8::Value> exception_obj,
                          BinaryTypes result_type)
-    : BinaryValue(isolate_object_deleter,
+    : BinaryValue(std::move(isolate_object_deleter),
                   ExceptionToString(context, message, exception_obj),
                   result_type) {}
 
@@ -298,8 +300,9 @@ void BinaryValue::CreateBackingStoreRef(v8::Local<v8::Value> value) {
 
 // NOLINTEND(cppcoreguidelines-pro-type-union-access)
 
-BinaryValueFactory::BinaryValueFactory(IsolateManager* isolate_manager)
-    : isolate_manager_(isolate_manager) {}
+BinaryValueFactory::BinaryValueFactory(
+    std::shared_ptr<IsolateManager> isolate_manager)
+    : isolate_manager_(std::move(isolate_manager)) {}
 
 void BinaryValueFactory::Free(BinaryValueHandle* handle) {
   const std::lock_guard<std::mutex> lock(mutex_);

src/v8_py_frontend/binary_value.h

@@ -72,13 +72,14 @@ struct BinaryValueHandle {
 class IsolateObjectDeleter {
  public:
   IsolateObjectDeleter();
-  explicit IsolateObjectDeleter(IsolateManager* isolate_manager);
+  explicit IsolateObjectDeleter(
+      std::shared_ptr<IsolateManager> isolate_manager);
 
   template <typename T>
   void operator()(T* handle) const;
 
  private:
-  IsolateManager* isolate_manager_;
+  std::shared_ptr<IsolateManager> isolate_manager_;
 };
 
 class BinaryValue {
@@ -122,7 +123,7 @@ class BinaryValue {
 
 class BinaryValueFactory {
  public:
-  explicit BinaryValueFactory(IsolateManager* isolate_manager);
+  explicit BinaryValueFactory(std::shared_ptr<IsolateManager> isolate_manager);
 
   auto FromHandle(BinaryValueHandle* handle) -> BinaryValue::Ptr;
   void Free(BinaryValueHandle* handle);
@@ -132,7 +133,7 @@ class BinaryValueFactory {
   auto New(Params&&... params) -> BinaryValue::Ptr;
 
  private:
-  IsolateManager* isolate_manager_;
+  std::shared_ptr<IsolateManager> isolate_manager_;
   std::mutex mutex_;
   std::unordered_map<BinaryValueHandle*, std::shared_ptr<BinaryValue>> values_;
 };

src/v8_py_frontend/cancelable_task_runner.cc

@@ -44,7 +44,8 @@ auto CancelableTaskState::SetCompleteIfNotCanceled() -> bool {
   return true;
 }
 
-CancelableTaskRunner::CancelableTaskRunner(IsolateManager* isolate_manager)
+CancelableTaskRunner::CancelableTaskRunner(
+    const std::shared_ptr<IsolateManager>& isolate_manager)
     : isolate_manager_(isolate_manager),
       task_registry_(
           std::make_shared<CancelableTaskRegistry>(isolate_manager)) {}
@@ -53,8 +54,9 @@ void CancelableTaskRunner::Cancel(uint64_t task_id) {
   task_registry_->Cancel(task_id);
 }
 
-CancelableTaskRegistry::CancelableTaskRegistry(IsolateManager* isolate_manager)
-    : isolate_manager_(isolate_manager), next_task_id_(1) {}
+CancelableTaskRegistry::CancelableTaskRegistry(
+    std::shared_ptr<IsolateManager> isolate_manager)
+    : isolate_manager_(std::move(isolate_manager)), next_task_id_(1) {}
 
 auto CancelableTaskRegistry::Create(
     std::shared_ptr<CancelableTaskState> task_state) -> uint64_t {
@@ -79,7 +81,7 @@ void CancelableTaskRegistry::Cancel(uint64_t task_id) {
     }
     task_state = iter->second;
   }
-  task_state->Cancel(isolate_manager_);
+  task_state->Cancel(isolate_manager_.get());
 }
 
 CancelableTaskRegistryRemover::CancelableTaskRegistryRemover(

src/v8_py_frontend/cancelable_task_runner.h

@@ -16,7 +16,8 @@ class CancelableTaskRegistry;
 /** Grafts a concept of cancelable tasks on top of an IsolateManager. */
 class CancelableTaskRunner {
  public:
-  explicit CancelableTaskRunner(IsolateManager* isolate_manager);
+  explicit CancelableTaskRunner(
+      const std::shared_ptr<IsolateManager>& isolate_manager);
 
   /**
    * Schedule the given runnable.
@@ -39,22 +40,23 @@ class CancelableTaskRunner {
   void Cancel(uint64_t task_id);
 
  private:
-  IsolateManager* isolate_manager_;
+  std::shared_ptr<IsolateManager> isolate_manager_;
   std::shared_ptr<CancelableTaskRegistry> task_registry_;
 };
 
 class CancelableTaskState;
 
 class CancelableTaskRegistry {
  public:
-  explicit CancelableTaskRegistry(IsolateManager* isolate_manager);
+  explicit CancelableTaskRegistry(
+      std::shared_ptr<IsolateManager> isolate_manager);
 
   auto Create(std::shared_ptr<CancelableTaskState> task_state) -> uint64_t;
   void Remove(uint64_t task_id);
   void Cancel(uint64_t task_id);
 
  private:
-  IsolateManager* isolate_manager_;
+  std::shared_ptr<IsolateManager> isolate_manager_;
   std::mutex mutex_;
   uint64_t next_task_id_;
   std::unordered_map<uint64_t, std::shared_ptr<CancelableTaskState>> tasks_;

src/v8_py_frontend/code_evaluator.cc

@@ -9,23 +9,27 @@
 #include <v8-primitive.h>
 #include <v8-script.h>
 #include <v8-value.h>
+#include <memory>
+#include <utility>
 #include "binary_value.h"
+#include "context_holder.h"
 #include "isolate_memory_monitor.h"
 
 namespace MiniRacer {
 
-CodeEvaluator::CodeEvaluator(v8::Persistent<v8::Context>* context,
-                             BinaryValueFactory* bv_factory,
-                             IsolateMemoryMonitor* memory_monitor)
-    : context_(context),
-      bv_factory_(bv_factory),
-      memory_monitor_(memory_monitor) {}
+CodeEvaluator::CodeEvaluator(
+    std::shared_ptr<ContextHolder> context,
+    std::shared_ptr<BinaryValueFactory> bv_factory,
+    std::shared_ptr<IsolateMemoryMonitor> memory_monitor)
+    : context_(std::move(context)),
+      bv_factory_(std::move(bv_factory)),
+      memory_monitor_(std::move(memory_monitor)) {}
 
 auto CodeEvaluator::Eval(v8::Isolate* isolate,
                          BinaryValue* code_ptr) -> BinaryValue::Ptr {
   const v8::Isolate::Scope isolate_scope(isolate);
   const v8::HandleScope handle_scope(isolate);
-  const v8::Local<v8::Context> context = context_->Get(isolate);
+  const v8::Local<v8::Context> context = context_->Get()->Get(isolate);
   const v8::Context::Scope context_scope(context);
 
   const v8::TryCatch trycatch(isolate);

src/v8_py_frontend/code_evaluator.h

@@ -1,29 +1,30 @@
 #ifndef INCLUDE_MINI_RACER_CODE_EVALUATOR_H
 #define INCLUDE_MINI_RACER_CODE_EVALUATOR_H
 
-#include <v8-context.h>
 #include <v8-exception.h>
 #include <v8-isolate.h>
 #include <v8-local-handle.h>
 #include <v8-persistent-handle.h>
+#include <memory>
 #include "binary_value.h"
+#include "context_holder.h"
 #include "isolate_memory_monitor.h"
 
 namespace MiniRacer {
 
 /** Parse and run arbitrary scripts within an isolate. */
 class CodeEvaluator {
  public:
-  CodeEvaluator(v8::Persistent<v8::Context>* context,
-                BinaryValueFactory* bv_factory,
-                IsolateMemoryMonitor* memory_monitor);
+  CodeEvaluator(std::shared_ptr<ContextHolder> context,
+                std::shared_ptr<BinaryValueFactory> bv_factory,
+                std::shared_ptr<IsolateMemoryMonitor> memory_monitor);
 
   auto Eval(v8::Isolate* isolate, BinaryValue* code_ptr) -> BinaryValue::Ptr;
 
  private:
-  v8::Persistent<v8::Context>* context_;
-  BinaryValueFactory* bv_factory_;
-  IsolateMemoryMonitor* memory_monitor_;
+  std::shared_ptr<ContextHolder> context_;
+  std::shared_ptr<BinaryValueFactory> bv_factory_;
+  std::shared_ptr<IsolateMemoryMonitor> memory_monitor_;
 };
 
 }  // end namespace MiniRacer