FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

[v25.x backport] src: convert context_frame field in AsyncWrap to internal field by Flarna · Pull Request #62357 · nodejs/node · GitHub

/ node Public

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

Merged
aduh95 merged 0 commit into
nodejs:v25.x-stagingfrom
Flarna:v25-bp-62103
Mar 30, 2026
ConversationCommits (0)ChecksFiles changed
Merged

[v25.x backport] src: convert context_frame field in AsyncWrap to internal field#62357
aduh95 merged 0 commit into
nodejs:v25.x-stagingfrom
Flarna:v25-bp-62103

Conversation

Flarna commented Mar 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

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 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 labels Mar 20, 2026
Flarna marked this pull request as draft March 20, 2026 23:58
Flarna marked this pull request as ready for review March 21, 2026 22:44
Flarna marked this pull request as draft March 22, 2026 00:32
Flarna marked this pull request as ready for review March 22, 2026 21:40

Flarna commented Mar 22, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 commented Mar 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

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 added request-ci Add this label to start a Jenkins CI on a PR. author ready PRs that have at least one approval, no outstanding review comments, and a CI started. labels Mar 29, 2026
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 29, 2026

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as outdated.

Sign in to view

nodejs-github-bot commented Mar 30, 2026

Copy link
Copy Markdown
Collaborator

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

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

aduh95 commented Mar 30, 2026

Copy link
Copy Markdown
Contributor

Landed in 0e4af84

Flarna deleted the v25-bp-62103 branch March 30, 2026 16:32
aduh95 pushed a commit that referenced this pull request Apr 10, 2026
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
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>
juanarbol pushed a commit to juanarbol/node that referenced this pull request Jul 16, 2026
src: convert context_frame field in AsyncWrap to internal field
4c805f0 
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
Backport-PR-URL: nodejs#62357
Reviewed-By: Anna Henningsen <anna@addaleax.net>
juanarbol pushed a commit that referenced this pull request Jul 17, 2026
src: convert context_frame field in AsyncWrap to internal field
75d1348 
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>
juanarbol pushed a commit that referenced this pull request Jul 19, 2026
src: convert context_frame field in AsyncWrap to internal field
7ea1b92 
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>
marco-ippolito pushed a commit that referenced this pull request Jul 29, 2026
src: convert context_frame field in AsyncWrap to internal field
5ef953b 
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL