inspector: tie objects lifetime to the thread they belong to

PR-URL: #22242
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Aleksei Koziatinskii <[email protected]>

Author: eugeneo

src/inspector/main_thread_interface.cc

@@ -3,6 +3,7 @@
 #include "node_mutex.h"
 #include "v8-inspector.h"
 
+#include <functional>
 #include <unicode/unistr.h>
 
 namespace node {
@@ -13,56 +14,72 @@ using v8_inspector::StringView;
 using v8_inspector::StringBuffer;
 
 template <typename T>
-class DeleteRequest : public Request {
+class DeletableWrapper : public Deletable {
  public:
-  explicit DeleteRequest(T* object) : object_(object) {}
-  void Call() override {
-    delete object_;
+  explicit DeletableWrapper(std::unique_ptr<T> object)
+                        : object_(std::move(object)) {}
+  ~DeletableWrapper() override = default;
+
+  static T* get(MainThreadInterface* thread, int id) {
+    return
+        static_cast<DeletableWrapper<T>*>(thread->GetObject(id))->object_.get();
   }
 
  private:
-  T* object_;
+  std::unique_ptr<T> object_;
 };
 
-template <typename Target, typename Arg>
-class SingleArgumentFunctionCall : public Request {
- public:
-  using Fn = void (Target::*)(Arg);
+template <typename T>
+std::unique_ptr<Deletable> WrapInDeletable(std::unique_ptr<T> object) {
+  return std::unique_ptr<DeletableWrapper<T>>(
+      new DeletableWrapper<T>(std::move(object)));
+}
 
-  SingleArgumentFunctionCall(Target* target, Fn fn, Arg argument)
-                             : target_(target),
-                               fn_(fn),
-                               arg_(std::move(argument)) {}
+template <typename Factory>
+class CreateObjectRequest : public Request {
+ public:
+  CreateObjectRequest(int object_id, Factory factory)
+                      : object_id_(object_id), factory_(std::move(factory)) {}
 
-  void Call() override {
-    Apply(target_, fn_, std::move(arg_));
+  void Call(MainThreadInterface* thread) {
+    thread->AddObject(object_id_, WrapInDeletable(factory_(thread)));
   }
 
  private:
-  template <typename Element>
-  void Apply(Element* target, Fn fn, Arg arg) {
-    (target->*fn)(std::move(arg));
+  int object_id_;
+  Factory factory_;
+};
+
+template <typename Factory>
+std::unique_ptr<Request> NewCreateRequest(int object_id, Factory factory) {
+  return std::unique_ptr<Request>(
+      new CreateObjectRequest<Factory>(object_id, std::move(factory)));
+}
+
+class DeleteRequest : public Request {
+ public:
+  explicit DeleteRequest(int object_id) : object_id_(object_id) {}
+
+  void Call(MainThreadInterface* thread) override {
+    thread->RemoveObject(object_id_);
   }
 
-  Target* target_;
-  Fn fn_;
-  Arg arg_;
+ private:
+  int object_id_;
 };
 
-class PostMessageRequest : public Request {
+template <typename Target, typename Fn>
+class CallRequest : public Request {
  public:
-  PostMessageRequest(InspectorSessionDelegate* delegate,
-                     StringView message)
-                     : delegate_(delegate),
-                       message_(StringBuffer::create(message)) {}
+  CallRequest(int id, Fn fn) : id_(id), fn_(std::move(fn)) {}
 
-  void Call() override {
-    delegate_->SendMessageToFrontend(message_->string());
+  void Call(MainThreadInterface* thread) override {
+    fn_(DeletableWrapper<Target>::get(thread, id_));
   }
 
  private:
-  InspectorSessionDelegate* delegate_;
-  std::unique_ptr<StringBuffer> message_;
+  int id_;
+  Fn fn_;
 };
 
 class DispatchMessagesTask : public v8::Task {
@@ -88,45 +105,63 @@ void DisposePairCallback(uv_handle_t* ref) {
 template <typename T>
 class AnotherThreadObjectReference {
  public:
-  // We create it on whatever thread, just make sure it gets disposed on the
-  // proper thread.
-  AnotherThreadObjectReference(std::shared_ptr<MainThreadHandle> thread,
-                               T* object)
-                               : thread_(thread), object_(object) {
+  AnotherThreadObjectReference(
+      std::shared_ptr<MainThreadHandle> thread, int object_id)
+      : thread_(thread), object_id_(object_id) {}
+
+  template <typename Factory>
+  AnotherThreadObjectReference(
+      std::shared_ptr<MainThreadHandle> thread, Factory factory)
+      : AnotherThreadObjectReference(thread, thread->newObjectId()) {
+    thread_->Post(NewCreateRequest(object_id_, std::move(factory)));
   }
   AnotherThreadObjectReference(AnotherThreadObjectReference&) = delete;
 
   ~AnotherThreadObjectReference() {
     // Disappearing thread may cause a memory leak
-    CHECK(thread_->Post(
-        std::unique_ptr<DeleteRequest<T>>(new DeleteRequest<T>(object_))));
-    object_ = nullptr;
+    thread_->Post(
+        std::unique_ptr<DeleteRequest>(new DeleteRequest(object_id_)));
   }
 
-  template <typename Fn, typename Arg>
-  void Post(Fn fn, Arg argument) const {
-    using R = SingleArgumentFunctionCall<T, Arg>;
-    thread_->Post(std::unique_ptr<R>(new R(object_, fn, std::move(argument))));
+  template <typename Fn>
+  void Call(Fn fn) const {
+    using Request = CallRequest<T, Fn>;
+    thread_->Post(std::unique_ptr<Request>(
+        new Request(object_id_, std::move(fn))));
   }
 
-  T* get() const {
-    return object_;
+  template <typename Arg>
+  void Call(void (T::*fn)(Arg), Arg argument) const {
+    Call(std::bind(Apply<Arg>, std::placeholders::_1, fn, std::move(argument)));
   }
 
  private:
+  // This has to use non-const reference to support std::bind with non-copyable
+  // types
+  template <typename Argument>
+  static void Apply(T* target, void (T::*fn)(Argument),
+    /* NOLINT (runtime/references) */ Argument& argument) {
+    (target->*fn)(std::move(argument));
+  }
+
   std::shared_ptr<MainThreadHandle> thread_;
-  T* object_;
+  const int object_id_;
 };
 
 class MainThreadSessionState {
  public:
-  MainThreadSessionState(
-      std::shared_ptr<MainThreadHandle> thread,
-      bool prevent_shutdown) : thread_(thread),
-                               prevent_shutdown_(prevent_shutdown) {}
+  MainThreadSessionState(MainThreadInterface* thread, bool prevent_shutdown)
+                         : thread_(thread),
+                           prevent_shutdown_(prevent_shutdown) {}
+
+  static std::unique_ptr<MainThreadSessionState> Create(
+      MainThreadInterface* thread, bool prevent_shutdown) {
+    return std::unique_ptr<MainThreadSessionState>(
+        new MainThreadSessionState(thread, prevent_shutdown));
+  }
 
   void Connect(std::unique_ptr<InspectorSessionDelegate> delegate) {
-    Agent* agent = thread_->GetInspectorAgent();
+    Agent* agent = thread_->inspector_agent();
     if (agent != nullptr)
       session_ = agent->Connect(std::move(delegate), prevent_shutdown_);
   }
@@ -136,7 +171,7 @@ class MainThreadSessionState {
   }
 
  private:
-  std::shared_ptr<MainThreadHandle> thread_;
+  MainThreadInterface* thread_;
   bool prevent_shutdown_;
   std::unique_ptr<InspectorSession> session_;
 };
@@ -148,12 +183,14 @@ class CrossThreadInspectorSession : public InspectorSession {
       std::shared_ptr<MainThreadHandle> thread,
       std::unique_ptr<InspectorSessionDelegate> delegate,
       bool prevent_shutdown)
-      : state_(thread, new MainThreadSessionState(thread, prevent_shutdown)) {
-    state_.Post(&MainThreadSessionState::Connect, std::move(delegate));
+      : state_(thread, std::bind(MainThreadSessionState::Create,
+                                 std::placeholders::_1,
+                                 prevent_shutdown)) {
+    state_.Call(&MainThreadSessionState::Connect, std::move(delegate));
   }
 
   void Dispatch(const StringView& message) override {
-    state_.Post(&MainThreadSessionState::Dispatch,
+    state_.Call(&MainThreadSessionState::Dispatch,
                 StringBuffer::create(message));
   }
 
@@ -163,13 +200,15 @@ class CrossThreadInspectorSession : public InspectorSession {
 
 class ThreadSafeDelegate : public InspectorSessionDelegate {
  public:
-  ThreadSafeDelegate(std::shared_ptr<MainThreadHandle> thread,
-                     std::unique_ptr<InspectorSessionDelegate> delegate)
-                     : thread_(thread), delegate_(thread, delegate.release()) {}
+  ThreadSafeDelegate(std::shared_ptr<MainThreadHandle> thread, int object_id)
+                     : thread_(thread), delegate_(thread, object_id) {}
 
   void SendMessageToFrontend(const v8_inspector::StringView& message) override {
-    thread_->Post(std::unique_ptr<Request>(
-        new PostMessageRequest(delegate_.get(), message)));
+    delegate_.Call(
+        [m = StringBuffer::create(message)]
+        (InspectorSessionDelegate* delegate) {
+      delegate->SendMessageToFrontend(m->string());
+    });
   }
 
  private:
@@ -252,7 +291,7 @@ void MainThreadInterface::DispatchMessages() {
       MessageQueue::value_type task;
       std::swap(dispatching_message_queue_.front(), task);
       dispatching_message_queue_.pop_front();
-      task->Call();
+      task->Call(this);
     }
   } while (had_messages);
   dispatching_messages_ = false;
@@ -264,6 +303,26 @@ std::shared_ptr<MainThreadHandle> MainThreadInterface::GetHandle() {
   return handle_;
 }
 
+void MainThreadInterface::AddObject(int id,
+                                    std::unique_ptr<Deletable> object) {
+  CHECK_NE(nullptr, object);
+  managed_objects_[id] = std::move(object);
+}
+
+void MainThreadInterface::RemoveObject(int id) {
+  CHECK_EQ(1, managed_objects_.erase(id));
+}
+
+Deletable* MainThreadInterface::GetObject(int id) {
+  auto iterator = managed_objects_.find(id);
+  // This would mean the object is requested after it was disposed, which is
+  // a coding error.
+  CHECK_NE(managed_objects_.end(), iterator);
+  Deletable* pointer = iterator->second.get();
+  CHECK_NE(nullptr, pointer);
+  return pointer;
+}
+
 std::unique_ptr<StringBuffer> Utf8ToStringView(const std::string& message) {
   icu::UnicodeString utf16 = icu::UnicodeString::fromUTF8(
       icu::StringPiece(message.data(), message.length()));
@@ -303,10 +362,12 @@ Agent* MainThreadHandle::GetInspectorAgent() {
 }
 
 std::unique_ptr<InspectorSessionDelegate>
-MainThreadHandle::MakeThreadSafeDelegate(
+MainThreadHandle::MakeDelegateThreadSafe(
     std::unique_ptr<InspectorSessionDelegate> delegate) {
+  int id = newObjectId();
+  main_thread_->AddObject(id, WrapInDeletable(std::move(delegate)));
   return std::unique_ptr<InspectorSessionDelegate>(
-      new ThreadSafeDelegate(shared_from_this(), std::move(delegate)));
+      new ThreadSafeDelegate(shared_from_this(), id));
 }
 
 bool MainThreadHandle::Expired() {

src/inspector/main_thread_interface.h

@@ -9,10 +9,10 @@
 #include "inspector_agent.h"
 #include "node_mutex.h"
 
+#include <atomic>
 #include <deque>
 #include <memory>
 #include <unordered_map>
-#include <unordered_set>
 
 namespace v8_inspector {
 class StringBuffer;
@@ -21,31 +21,41 @@ class StringView;
 
 namespace node {
 namespace inspector {
+class MainThreadInterface;
+
 class Request {
  public:
-  virtual void Call() = 0;
+  virtual void Call(MainThreadInterface*) = 0;
   virtual ~Request() {}
 };
 
+class Deletable {
+ public:
+  virtual ~Deletable() {}
+};
+
 std::unique_ptr<v8_inspector::StringBuffer> Utf8ToStringView(
     const std::string& message);
 
 using MessageQueue = std::deque<std::unique_ptr<Request>>;
-class MainThreadInterface;
 
 class MainThreadHandle : public std::enable_shared_from_this<MainThreadHandle> {
  public:
   explicit MainThreadHandle(MainThreadInterface* main_thread)
-                            : main_thread_(main_thread) {}
+                            : main_thread_(main_thread) {
+  }
   ~MainThreadHandle() {
     CHECK_NULL(main_thread_);  // main_thread_ should have called Reset
   }
   std::unique_ptr<InspectorSession> Connect(
       std::unique_ptr<InspectorSessionDelegate> delegate,
       bool prevent_shutdown);
+  int newObjectId() {
+    return ++next_object_id_;
+  }
   bool Post(std::unique_ptr<Request> request);
   Agent* GetInspectorAgent();
-  std::unique_ptr<InspectorSessionDelegate> MakeThreadSafeDelegate(
+  std::unique_ptr<InspectorSessionDelegate> MakeDelegateThreadSafe(
       std::unique_ptr<InspectorSessionDelegate> delegate);
   bool Expired();
 
@@ -55,6 +65,7 @@ class MainThreadHandle : public std::enable_shared_from_this<MainThreadHandle> {
   MainThreadInterface* main_thread_;
   Mutex block_lock_;
   int next_session_id_ = 0;
+  std::atomic_int next_object_id_ = {1};
 
   friend class MainThreadInterface;
 };
@@ -72,6 +83,9 @@ class MainThreadInterface {
   Agent* inspector_agent() {
     return agent_;
   }
+  void AddObject(int handle, std::unique_ptr<Deletable> object);
+  Deletable* GetObject(int id);
+  void RemoveObject(int handle);
 
  private:
   using AsyncAndInterface = std::pair<uv_async_t, MainThreadInterface*>;
@@ -92,6 +106,7 @@ class MainThreadInterface {
   v8::Platform* const platform_;
   DeleteFnPtr<AsyncAndInterface, CloseAsync> main_thread_request_;
   std::shared_ptr<MainThreadHandle> handle_;
+  std::unordered_map<int, std::unique_ptr<Deletable>> managed_objects_;
 };
 
 }  // namespace inspector