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 Backport-PR-URL: #35241 PR-URL: #32523 Reviewed-By: Matheus Marchini <[email protected]> Reviewed-By: James M Snell <[email protected]>

Author: addaleax

src/env.h

@@ -1273,6 +1273,9 @@ class Environment : public MemoryRetainer{
 inlinevoidset_process_exit_handler(
  std::function<void(Environment*, int)>&& handler);
 
+voidRunAndClearNativeImmediates(bool only_refed = false);
+voidRunAndClearInterrupts();
+
 private:
 inlinevoidThrowError(v8::Local<v8::Value> (*fun)(v8::Local<v8::String>),
 const char* errmsg);
@@ -1410,8 +1413,6 @@ class Environment : public MemoryRetainer{
 // yet or already have been destroyed.
 bool task_queues_async_initialized_ = false;
 
-voidRunAndClearNativeImmediates(bool only_refed = false);
-voidRunAndClearInterrupts();
  std::atomic<Environment**> interrupt_data_{nullptr};
 voidRequestInterruptFromV8();
 staticvoidCheckImmediate(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_;
 };
 
-classDispatchMessagesTask : publicv8::Task{
-public:
-explicitDispatchMessagesTask(std::weak_ptr<MainThreadInterface> thread)
- : thread_(thread){}
-
-voidRun() override{
-if (auto thread = thread_.lock()) thread->DispatchMessages();
- }
-
-private:
- std::weak_ptr<MainThreadInterface> thread_;
-};
-
 template <typename T>
 classAnotherThreadObjectReference{
 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();
 }
 
 voidMainThreadInterface::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>{
 classMainThreadInterface :
  public std::enable_shared_from_this<MainThreadInterface>{
 public:
-MainThreadInterface(Agent* agent, uv_loop_t*, v8::Isolate* isolate,
- v8::Platform* platform);
+explicitMainThreadInterface(Agent* agent);
 ~MainThreadInterface();
 
 voidDispatchMessages();
@@ -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

@@ -660,8 +660,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();
  }
@@ -697,10 +696,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:
 voidToggleAsyncHook(v8::Isolate* isolate,
 const v8::Global<v8::Function>& fn);

src/inspector_js_api.cc

@@ -83,7 +83,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.
 asyncfunctionensureListenerDoesNotInterrupt(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.
+awaitnewPromise(setImmediate);
+
 constcurrentTime=Date.now();
 letconsoleLogHappened=false;
 session.once('Runtime.consoleAPICalled',