async_hooks: check for empty contexts before removing

This way we don't end up attempting to SetPromiseHooks on contexts that have already been collected. Fixes: nodejs#39019 PR-URL: nodejs#39095 Backport-PR-URL: nodejs#38577 Reviewed-By: Stephen Belanger <[email protected]> Reviewed-By: Gerhard Stöbich <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Andrey Pechkurov <[email protected]> Reviewed-By: Danielle Adams <[email protected]>
foxxyz · Oct 18, 2021 · d40cbd5 · d40cbd5
1 parent 10053d0
commit d40cbd5
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 1 deletion.
diff --git a/src/env-inl.h b/src/env-inl.h
@@ -127,6 +127,11 @@ inline void AsyncHooks::SetJSPromiseHooks(v8::Local<v8::Function> init,
   js_promise_hooks_[2].Reset(env()->isolate(), after);
   js_promise_hooks_[3].Reset(env()->isolate(), resolve);
   for (auto it = contexts_.begin(); it != contexts_.end(); it++) {
+    if (it->IsEmpty()) {
+      it = contexts_.erase(it);
+      it--;
+      continue;
+    }
     PersistentToLocal::Weak(env()->isolate(), *it)
         ->SetPromiseHooks(init, before, after, resolve);
   }
@@ -279,8 +284,13 @@ inline void AsyncHooks::RemoveContext(v8::Local<v8::Context> ctx) {
   v8::Isolate* isolate = env()->isolate();
   v8::HandleScope handle_scope(isolate);
   for (auto it = contexts_.begin(); it != contexts_.end(); it++) {
+    if (it->IsEmpty()) {
+      it = contexts_.erase(it);
+      it--;
+      continue;
+    }
     v8::Local<v8::Context> saved_context =
-      PersistentToLocal::Weak(env()->isolate(), *it);
+      PersistentToLocal::Weak(isolate, *it);
     if (saved_context == ctx) {
       it->Reset();
       contexts_.erase(it);

diff --git a/test/parallel/test-async-hooks-vm-gc.js b/test/parallel/test-async-hooks-vm-gc.js
@@ -0,0 +1,15 @@
+// Flags: --expose-gc
+'use strict';
+
+require('../common');
+const asyncHooks = require('async_hooks');
+const vm = require('vm');
+
+// This is a regression test for https://github.com/nodejs/node/issues/39019
+//
+// It should not segfault.
+
+const hook = asyncHooks.createHook({ init() {} }).enable();
+vm.createContext();
+globalThis.gc();
+hook.disable();