Conversation
The `Environment::destroy_ids_timer_handle` should be cleaned up by `Environment::CleanupHandles()`. Fix that by adding it to the list. This partially fixes a cctest. Ref: nodejs#14206
The `IsolateData` instance is created before the `Environment` instance, so free in reverse order. Fixes: nodejs#14206
This reverts commit 75bf8a9. Ref: nodejs#14206 Ref: nodejs#14317
This reverts commit 95ab966. Ref: nodejs#14206 Ref: nodejs#14246
nodejs-github-bot
added
the
c++
|
I couldn’t reproduce the crash locally, but we’ve had this issue before with the same symptoms, and valgrind went from complaining to not complaining, so I’m feeling very confident that this is sufficient. |
| RegisterHandleCleanup( | ||
| reinterpret_cast<uv_handle_t*>(&destroy_ids_timer_handle_), | ||
| close_and_finish, | ||
| nullptr); |
There was a problem hiding this comment.
@trevnorris FYI, this comes from 1e44fd9. I probably would have made the same mistake, but I wanted to let you know :)
I'm able to reproduce this contently on arm here and I'll run this PR and report back. |
danbev
left a comment
danbev
left a comment
There was a problem hiding this comment.
Thanks for fixing this and sorry about causing it!
refack
left a comment
refack
left a comment
There was a problem hiding this comment.
May your prefered deity bless you!
|
I'm +1 for fast tracking this (change is not big, and arm7 CI machine seems to consistently fail on this now) |
|
/cc @nodejs/testing @jBarz |
|
Let’s wait for CI to come back, but yes, I’m okay with fast-tracking as well, these aren’t so terribly complex fixes. |
|
CI is as green as the Windows XP default background! Landed in 97c4394...c80d400 |
The `Environment::destroy_ids_timer_handle` should be cleaned up by `Environment::CleanupHandles()`. Fix that by adding it to the list. This partially fixes a cctest. Ref: #14206 PR-URL: #14749 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
The `IsolateData` instance is created before the `Environment` instance, so free in reverse order. Fixes: #14206 PR-URL: #14749 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
The `Environment::destroy_ids_timer_handle` should be cleaned up by `Environment::CleanupHandles()`. Fix that by adding it to the list. This partially fixes a cctest. Ref: #14206 PR-URL: #14749 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
The `IsolateData` instance is created before the `Environment` instance, so free in reverse order. Fixes: #14206 PR-URL: #14749 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
|
Should this be backported to |
7 participants
Fixes: #14206
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
src/env, test