Skip to content

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

Merged
aduh95 merged 0 commit intonodejs:v25.x-stagingfrom
Flarna:v25-bp-62103
Mar 30, 2026
Merged

[v25.x backport] src: convert context_frame field in AsyncWrap to internal field#62357
aduh95 merged 0 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.

@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

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

nodejs-github-bot commented Mar 30, 2026

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

@aduh95 aduh95 merged commit 0e4af84 into nodejs:v25.x-staging Mar 30, 2026
21 checks passed
@aduh95
Copy link
Copy Markdown
Contributor

aduh95 commented Mar 30, 2026

Landed in 0e4af84

@Flarna Flarna deleted the v25-bp-62103 branch March 30, 2026 16:32
aduh95 pushed a commit that referenced this pull request Apr 10, 2026
@addaleax @aduh95
src: convert context_frame field in AsyncWrap to internal field
809acc8 
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: #62103
Backport-PR-URL: #62357
Reviewed-By: Anna Henningsen <anna@addaleax.net>
aduh95 pushed a commit that referenced this pull request Apr 10, 2026
@addaleax @aduh95
src: convert context_frame field in AsyncWrap to internal field
6788981 
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: #62103
Backport-PR-URL: #62357
Reviewed-By: Anna Henningsen <anna@addaleax.net>
aduh95 pushed a commit that referenced this pull request Apr 10, 2026
@addaleax @aduh95
src: convert context_frame field in AsyncWrap to internal field
7d67e5d 
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: #62103
Backport-PR-URL: #62357
Reviewed-By: Anna Henningsen <anna@addaleax.net>
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.

4 participants

@Flarna @nodejs-github-bot @aduh95 @addaleax