src,worker: fix race of WorkerHeapSnapshotTaker

[ywave620]

Fix: #44515 #39686
maybe more... there are serval crash reports about worker heap snapshot

With one extra line of code, this concurency bug can be easily demonstrated and reproduced.
Since BaseObjectPtr is not thread safe(no synchronization around checking and manipulating the refcnt), the fix is to ensure WorkerHeapSnapshotTaker is not shared by worker thread and main thread

--- a/src/node_worker.cc
+++ b/src/node_worker.cc
@@ -771,28 +771,45 @@ void Worker::TakeHeapSnapshot(const FunctionCallbackInfo<Value>& args) {
       ->NewInstance(env->context()).ToLocal(&wrap)) {
     return;
   }
   BaseObjectPtr<WorkerHeapSnapshotTaker> taker =
       MakeDetachedBaseObject<WorkerHeapSnapshotTaker>(env, wrap);
 
   // Interrupt the worker thread and take a snapshot, then schedule a call
   // on the parent thread that turns that snapshot into a readable stream.
   bool scheduled = w->RequestInterrupt([taker, env](Environment* worker_env) {
+     // executed in worker thread
+    // now worker thread holds one strong BaseObjectPtr

      heap::HeapSnapshotPointer snapshot {
         worker_env->isolate()->GetHeapProfiler()->TakeHeapSnapshot() };
      CHECK(snapshot);
      env->SetImmediateThreadsafe(
+        // executed in main thread
+       // now main thread holds another strong BaseObjectPtr

         [taker, snapshot = std::move(snapshot)](Environment* env) mutable {
           HandleScope handle_scope(env->isolate());
           Context::Scope context_scope(env->context());
 
           AsyncHooks::DefaultTriggerAsyncIdScope trigger_id_scope(taker.get());
           BaseObjectPtr<AsyncWrap> stream = heap::CreateHeapSnapshotStream(
               env, std::move(snapshot));
           Local<Value> args[] = { stream->object() };
           taker->MakeCallback(env->ondone_string(), arraysize(args), args);
         }, CallbackFlags::kUnrefed);
+      
+    // wait for the main thread to finish executing the callback above
+    // remember to include <thread> and <chrono> in the head of this file
+    std::this_thread::sleep_for(std::chrono::milliseconds(2000));
+    
+    // now the worker thread holds the only one strong BaseObjectPtr refering to the
+    // WorkerHeapSnapshotTaker instance and it is destructed here, as a result, that
+    // WorkerHeapSnapshotTaker instance is destructed here, too.
+    // The destructor try to access the associated JS object, however, since that JS object 
+    // is owned by main thread's Isolate, the v8 complains:
+    // HandleScope::HandleScope Entering the V8 API without proper locking in place
   });
   args.GetReturnValue().Set(scheduled ? taker->object() : Local<Object>());
 }
 
 void Worker::LoopIdleTime(const FunctionCallbackInfo<Value>& args) {

[ywave620]

test/sequential/test-watch-mode.mjs failed. However, it is a known issue, fixed in #44739

[ywave620]

@addaleax Could you help moving this PR forward?

[ywave620]

@mojodna

Hey, any idea? :)

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/46966/

[ywave620]

Shall we proceed to get this PR landed?

[juanarbol]

LGTM; I'm AFK, unless someone else is faster than me. I will land this one. Thanks for this 💚

[ywave620]

Another 3 failure of running parallel/test-worker-heap-snapshot caused by this bug nodejs/reliability#395

[nodejs-github-bot]

Landed in 66cedb4