Commit d2b972e
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: #39019
PR-URL: #39095
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Andrey Pechkurov <apechkurov@gmail.com>
Reviewed-By: Danielle Adams <adamzdanielle@gmail.com>1 parent 78d2e0e commit d2b972e
2 files changed
Lines changed: 26 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -104,6 +104,11 @@ inline void AsyncHooks::SetJSPromiseHooks(v8::Local<v8::Function> init, | |||
| 104 | 104 | js_promise_hooks_[2].Reset(env()->isolate(), after); | |
| 105 | 105 | js_promise_hooks_[3].Reset(env()->isolate(), resolve); | |
| 106 | 106 | for (auto it = contexts_.begin(); it != contexts_.end(); it++) { | |
| 107 | + if (it->IsEmpty()) { | ||
| 108 | + it = contexts_.erase(it); | ||
| 109 | + it--; | ||
| 110 | + continue; | ||
| 111 | + } | ||
| 107 | 112 | PersistentToLocal::Weak(env()->isolate(), *it) | |
| 108 | 113 | ->SetPromiseHooks(init, before, after, resolve); | |
| 109 | 114 | } | |
@@ -256,8 +261,13 @@ inline void AsyncHooks::RemoveContext(v8::Local<v8::Context> ctx) { | |||
| 256 | 261 | v8::Isolate* isolate = env()->isolate(); | |
| 257 | 262 | v8::HandleScope handle_scope(isolate); | |
| 258 | 263 | for (auto it = contexts_.begin(); it != contexts_.end(); it++) { | |
| 264 | + if (it->IsEmpty()) { | ||
| 265 | + it = contexts_.erase(it); | ||
| 266 | + it--; | ||
| 267 | + continue; | ||
| 268 | + } | ||
| 259 | 269 | v8::Local<v8::Context> saved_context = | |
| 260 | - PersistentToLocal::Weak(env()->isolate(), *it); | ||
| 270 | + PersistentToLocal::Weak(isolate, *it); | ||
| 261 | 271 | if (saved_context == ctx) { | |
| 262 | 272 | it->Reset(); | |
| 263 | 273 | contexts_.erase(it); | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -0,0 +1,15 @@ | |||
| 1 | + // Flags: --expose-gc | ||
| 2 | + 'use strict'; | ||
| 3 | + | ||
| 4 | + require('../common'); | ||
| 5 | + const asyncHooks = require('async_hooks'); | ||
| 6 | + const vm = require('vm'); | ||
| 7 | + | ||
| 8 | + // This is a regression test for https://github.com/nodejs/node/issues/39019 | ||
| 9 | + // | ||
| 10 | + // It should not segfault. | ||
| 11 | + | ||
| 12 | + const hook = asyncHooks.createHook({ init() {} }).enable(); | ||
| 13 | + vm.createContext(); | ||
| 14 | + globalThis.gc(); | ||
| 15 | + hook.disable(); | ||
0 commit comments