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 <mat@mmarchini.me> Reviewed-By: James M Snell <jasnell@gmail.com>

Author: addaleax

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);

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);

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_;
 };

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;
  }

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);

src/inspector_js_api.cc

@@ -84,7 +84,7 @@ class JSBindingsConnection : public AsyncWrap {
 
  private:
  Environment* env_;
- JSBindingsConnection* connection_;
+ BaseObjectPtr<JSBindingsConnection> connection_;
  };
 
  JSBindingsConnection(Environment* env,

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',