From 65e5a8a90cd85fc4d9036bc4b54e0905edd8b51a Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Wed, 11 Dec 2019 21:48:47 -0500
Subject: [PATCH] src: unregister Isolate with platform before disposing

I previously thought the order of these calls was no longer
relevant. I was wrong.

This commit undoes the changes from 312c02d25e9, adds a comment
explaining why I was wrong, and flips the order of the calls
elsewhere for consistency, the latter having been the goal
of 312c02d25e9.

Fixes: https://github.com/nodejs/node/issues/30846
Refs: https://github.com/nodejs/node/pull/30181

PR-URL: https://github.com/nodejs/node/pull/30909
Reviewed-By: Shelley Vohr <codebytere@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
---
 src/node.h                      | 1 +
 src/node_main_instance.cc       | 2 +-
 src/node_worker.cc              | 7 ++++++-
 test/cctest/node_test_fixture.h | 2 +-
 4 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/src/node.h b/src/node.h
index d6ec670ae73b7a..aadb60ea557f43 100644
--- a/src/node.h
+++ b/src/node.h
@@ -280,6 +280,7 @@ class NODE_EXTERN MultiIsolatePlatform : public v8::Platform {
 
   // This function may only be called once per `Isolate`, and discard any
   // pending delayed tasks scheduled for that isolate.
+  // This needs to be called right before calling `Isolate::Dispose()`.
   virtual void UnregisterIsolate(v8::Isolate* isolate) = 0;
 
   // The platform should call the passed function once all state associated
diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc
index eea847725300f0..86a857299f6e90 100644
--- a/src/node_main_instance.cc
+++ b/src/node_main_instance.cc
@@ -99,8 +99,8 @@ NodeMainInstance::~NodeMainInstance() {
   if (!owns_isolate_) {
     return;
   }
-  isolate_->Dispose();
   platform_->UnregisterIsolate(isolate_);
+  isolate_->Dispose();
 }
 
 int NodeMainInstance::Run() {
diff --git a/src/node_worker.cc b/src/node_worker.cc
index f1b2347d29cbe3..b89166e9f48ad5 100644
--- a/src/node_worker.cc
+++ b/src/node_worker.cc
@@ -192,8 +192,13 @@ class WorkerThreadData {
         *static_cast<bool*>(data) = true;
       }, &platform_finished);
 
-      isolate->Dispose();
+      // The order of these calls is important; if the Isolate is first disposed
+      // and then unregistered, there is a race condition window in which no
+      // new Isolate at the same address can successfully be registered with
+      // the platform.
+      // (Refs: https://github.com/nodejs/node/issues/30846)
       w_->platform_->UnregisterIsolate(isolate);
+      isolate->Dispose();
 
       // Wait until the platform has cleaned up all relevant resources.
       while (!platform_finished)
diff --git a/test/cctest/node_test_fixture.h b/test/cctest/node_test_fixture.h
index f6b80c860c1f58..cd669d23da3384 100644
--- a/test/cctest/node_test_fixture.h
+++ b/test/cctest/node_test_fixture.h
@@ -106,9 +106,9 @@ class NodeTestFixture : public ::testing::Test {
 
   void TearDown() override {
     isolate_->Exit();
-    isolate_->Dispose();
     platform->DrainTasks(isolate_);
     platform->UnregisterIsolate(isolate_);
+    isolate_->Dispose();
     isolate_ = nullptr;
   }
 };