Skip to content

[v25.x backport] src: convert context_frame field in AsyncWrap to internal field#62357

Open
Flarna wants to merge 1 commit intonodejs:v25.x-stagingfrom
Flarna:v25-bp-62103
Open

[v25.x backport] src: convert context_frame field in AsyncWrap to internal field#62357
Flarna wants to merge 1 commit intonodejs:v25.x-stagingfrom
Flarna:v25-bp-62103

Conversation

@Flarna
Copy link
Copy Markdown
Member

@Flarna Flarna commented Mar 20, 2026 •
edited
Loading

Backport the second (7dd5ca8 / 6ad5f7c) commit of #62103 as it failed in 25.8.1 release. The other two commits were included in 25.8.1

Refs: #62103

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v25.x Issues that can be reproduced on v25.x or PRs targeting the v25.x-staging branch. labels Mar 20, 2026
@Flarna Flarna marked this pull request as draft March 20, 2026 23:58
@Flarna Flarna marked this pull request as ready for review March 21, 2026 22:44
@Flarna Flarna marked this pull request as draft March 22, 2026 00:32
@Flarna Flarna marked this pull request as ready for review March 22, 2026 21:40
@Flarna
Copy link
Copy Markdown
Member Author

Flarna commented Mar 22, 2026 •
edited
Loading

Finally I was able to reproduce the failed test locally by using clang (19.1.1) like in Github CI instead gcc (13.3.0).
Different behavior between clang and gcc smells fishy.
As far as I know main uses also clang but this test is green on main which is strange.
I found also that this test fails only for release builds, not debug builds which is again something I do not like.

I'm not that a big expert on V8 internals/snapshots.

@addaleax @joyeecheung any hints welcome how to find out why v25 shows these instability in snapshot creation.

@addaleax @Flarna
src: convert context_frame field in AsyncWrap to internal field
7ba4549 
Using an internal field instead of a `v8::Global<>` removes
an unnecessary memory leak footgun.

This includes a test that demonstrates the issue, albeit
using internal APIs.

It's worth noting that if this PR is not accepted, we'd still
be missing memory tracking for the `context_frame_` field,
and we'd need to add it through our memory tracking API.

PR-URL: nodejs#62103
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
@Flarna
Copy link
Copy Markdown
Member Author

Flarna commented Mar 29, 2026 •
edited
Loading

I found a bit more:

  • it's green if ASLR is disabled (setarch -R)
  • it's green if I pass --hash_seed=42

Anyhow, rebasing to tip of v25.x-staging made it also green.
It seems the commit 67b854d somehow causes this but honestly no idea why.

My best guess is that there is some sort of instability in snapshot generation which is just not visible on main as of now - and now also not on v25.

@Flarna Flarna added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Mar 29, 2026
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 29, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Mar 29, 2026

CI: https://ci.nodejs.org/job/node-test-pull-request/72213/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v25.x Issues that can be reproduced on v25.x or PRs targeting the v25.x-staging branch.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Flarna @nodejs-github-bot @addaleax