src: use env->RequestInterrupt() for inspector MainThreadInterface

This simplifies the code significantly, and removes the dependency of the inspector code on the availability of a `MultiIsolatePlatform` (by removing the dependency on a platform altogether). It also fixes a memory leak that occurs when `RequestInterrupt()` is used, but the interrupt handler is never called before the Isolate is destroyed. One downside is that this leads to a slight change in timing, because inspector messages are not dispatched in a re-entrant way. This means having to harden one test to account for that possibility by making sure that the stack is always clear through a `setImmediate()`. This does not affect the assertion made by the test, which is that messages will not be delivered synchronously while other code is executing. #32415 PR-URL: #32523 Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: James M Snell <[email protected]>
nodejs · Apr 10, 2020 · 6f9f546 · 6f9f546
1 parent 32e3a6b
commit 6f9f546
Show file tree

Hide file tree

Showing 7 changed files with 23 additions and 44 deletions.
diff --git a/src/env.h b/src/env.h
@@ -1249,6 +1249,9 @@ class Environment : public MemoryRetainer {
   inline void set_process_exit_handler(
       std::function<void(Environment*, int)>&& handler);
 
+  void RunAndClearNativeImmediates(bool only_refed = false);
+  void RunAndClearInterrupts();
+
  private:
   template <typename Fn>
   inline void CreateImmediate(Fn&& cb, bool ref);
@@ -1421,8 +1424,6 @@ class Environment : public MemoryRetainer {
   // yet or already have been destroyed.
   bool task_queues_async_initialized_ = false;
 
-  void RunAndClearNativeImmediates(bool only_refed = false);
-  void RunAndClearInterrupts();
   std::atomic<Environment**> interrupt_data_ {nullptr};
   void RequestInterruptFromV8();
   static void CheckImmediate(uv_check_t* handle);

diff --git a/src/inspector/main_thread_interface.cc b/src/inspector/main_thread_interface.cc
@@ -1,5 +1,6 @@
 #include "main_thread_interface.h"
 
+#include "env-inl.h"
 #include "node_mutex.h"
 #include "v8-inspector.h"
 #include "util-inl.h"
@@ -85,19 +86,6 @@ class CallRequest : public Request {
   Fn fn_;
 };
 
-class DispatchMessagesTask : public v8::Task {
- public:
-  explicit DispatchMessagesTask(std::weak_ptr<MainThreadInterface> thread)
-                                : thread_(thread) {}
-
-  void Run() override {
-    if (auto thread = thread_.lock()) thread->DispatchMessages();
-  }
-
- private:
-  std::weak_ptr<MainThreadInterface> thread_;
-};
-
 template <typename T>
 class AnotherThreadObjectReference {
  public:
@@ -212,36 +200,23 @@ class ThreadSafeDelegate : public InspectorSessionDelegate {
 }  // namespace
 
 
-MainThreadInterface::MainThreadInterface(Agent* agent, uv_loop_t* loop,
-                                         v8::Isolate* isolate,
-                                         v8::Platform* platform)
-                                         : agent_(agent), isolate_(isolate),
-                                           platform_(platform) {
-}
+MainThreadInterface::MainThreadInterface(Agent* agent) : agent_(agent) {}
 
 MainThreadInterface::~MainThreadInterface() {
   if (handle_)
     handle_->Reset();
 }
 
 void MainThreadInterface::Post(std::unique_ptr<Request> request) {
+  CHECK_NOT_NULL(agent_);
   Mutex::ScopedLock scoped_lock(requests_lock_);
   bool needs_notify = requests_.empty();
   requests_.push_back(std::move(request));
   if (needs_notify) {
-    if (isolate_ != nullptr && platform_ != nullptr) {
-      std::shared_ptr<v8::TaskRunner> taskrunner =
-          platform_->GetForegroundTaskRunner(isolate_);
-      std::weak_ptr<MainThreadInterface>* interface_ptr =
-          new std::weak_ptr<MainThreadInterface>(shared_from_this());
-      taskrunner->PostTask(
-          std::make_unique<DispatchMessagesTask>(*interface_ptr));
-      isolate_->RequestInterrupt([](v8::Isolate* isolate, void* opaque) {
-        std::unique_ptr<std::weak_ptr<MainThreadInterface>> interface_ptr {
-          static_cast<std::weak_ptr<MainThreadInterface>*>(opaque) };
-        if (auto iface = interface_ptr->lock()) iface->DispatchMessages();
-      }, static_cast<void*>(interface_ptr));
-    }
+    std::weak_ptr<MainThreadInterface> weak_self {shared_from_this()};
+    agent_->env()->RequestInterrupt([weak_self](Environment*) {
+      if (auto iface = weak_self.lock()) iface->DispatchMessages();
+    });
   }
   incoming_message_cond_.Broadcast(scoped_lock);
 }
@@ -274,7 +249,7 @@ void MainThreadInterface::DispatchMessages() {
       std::swap(dispatching_message_queue_.front(), task);
       dispatching_message_queue_.pop_front();
 
-      v8::SealHandleScope seal_handle_scope(isolate_);
+      v8::SealHandleScope seal_handle_scope(agent_->env()->isolate());
       task->Call(this);
     }
   } while (had_messages);

diff --git a/src/inspector/main_thread_interface.h b/src/inspector/main_thread_interface.h
@@ -72,8 +72,7 @@ class MainThreadHandle : public std::enable_shared_from_this<MainThreadHandle> {
 class MainThreadInterface :
     public std::enable_shared_from_this<MainThreadInterface> {
  public:
-  MainThreadInterface(Agent* agent, uv_loop_t*, v8::Isolate* isolate,
-                      v8::Platform* platform);
+  explicit MainThreadInterface(Agent* agent);
   ~MainThreadInterface();
 
   void DispatchMessages();
@@ -98,8 +97,6 @@ class MainThreadInterface :
   ConditionVariable incoming_message_cond_;
   // Used from any thread
   Agent* const agent_;
-  v8::Isolate* const isolate_;
-  v8::Platform* const platform_;
   std::shared_ptr<MainThreadHandle> handle_;
   std::unordered_map<int, std::unique_ptr<Deletable>> managed_objects_;
 };

diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc
@@ -662,8 +662,7 @@ class NodeInspectorClient : public V8InspectorClient {
   std::shared_ptr<MainThreadHandle> getThreadHandle() {
     if (!interface_) {
       interface_ = std::make_shared<MainThreadInterface>(
-          env_->inspector_agent(), env_->event_loop(), env_->isolate(),
-          env_->isolate_data()->platform());
+          env_->inspector_agent());
     }
     return interface_->GetHandle();
   }
@@ -699,10 +698,9 @@ class NodeInspectorClient : public V8InspectorClient {
 
     running_nested_loop_ = true;
 
-    MultiIsolatePlatform* platform = env_->isolate_data()->platform();
     while (shouldRunMessageLoop()) {
       if (interface_) interface_->WaitForFrontendEvent();
-      while (platform->FlushForegroundTasks(env_->isolate())) {}
+      env_->RunAndClearInterrupts();
     }
     running_nested_loop_ = false;
   }

diff --git a/src/inspector_agent.h b/src/inspector_agent.h
@@ -116,6 +116,8 @@ class Agent {
   // Interface for interacting with inspectors in worker threads
   std::shared_ptr<WorkerManager> GetWorkerManager();
 
+  inline Environment* env() const { return parent_env_; }
+
  private:
   void ToggleAsyncHook(v8::Isolate* isolate,
                        const v8::Global<v8::Function>& fn);

diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc
@@ -84,7 +84,7 @@ class JSBindingsConnection : public AsyncWrap {
 
    private:
     Environment* env_;
-    JSBindingsConnection* connection_;
+    BaseObjectPtr<JSBindingsConnection> connection_;
   };
 
   JSBindingsConnection(Environment* env,

diff --git a/test/parallel/test-inspector-connect-main-thread.js b/test/parallel/test-inspector-connect-main-thread.js
@@ -76,6 +76,12 @@ function doConsoleLog(arrayBuffer) {
 // and do not interrupt the main code. Interrupting the code flow
 // can lead to unexpected behaviors.
 async function ensureListenerDoesNotInterrupt(session) {
+  // Make sure that the following code is not affected by the fact that it may
+  // run inside an inspector message callback, during which other inspector
+  // message callbacks (such as the one triggered by doConsoleLog()) would
+  // not be processed.
+  await new Promise(setImmediate);
+
   const currentTime = Date.now();
   let consoleLogHappened = false;
   session.once('Runtime.consoleAPICalled',