src: add handle scope to OnFatalError()
#25775
Conversation
For the report generation, we use `Environment::GetCurrent(isolate)` which uses `isolate->GetCurrentContext()` under the hood, thus allocates a handle. Without a `HandleScope`, this is invalid. This might not strictly be allowed inside of `OnFatalError()`, but it won’t make anything worse either.
nodejs-github-bot
added
the
c++
There was a problem hiding this comment.
@addaleax: 2 questions:
Without a
HandleScope, this is invalid.
does that mean any caller of Environment::GetCurrent(isolate) create a handlescope in its frame? I thought that is required only if one is dealing with JS objects. I am not seeing this as followed anywhere else?
but it won’t make anything worse either.
do you think under extreme situations creation of HandleScope itself can lead to cascaded failures?
Yes.
We do always have a handle scope – there’s just usually already another one present.
I would assume that that’s the case for when e.g. creating a |
addaleax
added
the
author ready
|
Landed in 006aa63 |
For the report generation, we use `Environment::GetCurrent(isolate)` which uses `isolate->GetCurrentContext()` under the hood, thus allocates a handle. Without a `HandleScope`, this is invalid. This might not strictly be allowed inside of `OnFatalError()`, but it won’t make anything worse either. PR-URL: #25775 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]>
For the report generation, we use `Environment::GetCurrent(isolate)` which uses `isolate->GetCurrentContext()` under the hood, thus allocates a handle. Without a `HandleScope`, this is invalid. This might not strictly be allowed inside of `OnFatalError()`, but it won’t make anything worse either. PR-URL: #25775 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: James M Snell <[email protected]>
Do not require an explicit `HandleScope`, or the ability to create one, when using `Environment::GetCurrent()`. `isolate->InContext()` is used as an indicator that it is probably okay to create a `HandleScope`, see also the short discussion in nodejs#25775 (review).
Do not require an explicit `HandleScope`, or the ability to create one, when using `Environment::GetCurrent()`. `isolate->InContext()` is used as an indicator that it is probably okay to create a `HandleScope`, see also the short discussion in #25775 (review). PR-URL: #26376 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
Do not require an explicit `HandleScope`, or the ability to create one, when using `Environment::GetCurrent()`. `isolate->InContext()` is used as an indicator that it is probably okay to create a `HandleScope`, see also the short discussion in nodejs#25775 (review). PR-URL: nodejs#26376 Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>
For the report generation, we use
Environment::GetCurrent(isolate)which uses
isolate->GetCurrentContext()under the hood, thusallocates a handle. Without a
HandleScope, this is invalid.This might not strictly be allowed inside of
OnFatalError(),but it won’t make anything worse either.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes