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

[v22.x backport] src: initialize privateSymbols for per_context by jazelly · Pull Request #58217 · nodejs/node · GitHub

/ node Public

[v22.x backport] src: initialize privateSymbols for per_context - #58217

Closed
jazelly wants to merge 2 commits into
nodejs:v22.x-stagingfrom
jazelly:backport-57479
Closed

[v22.x backport] src: initialize privateSymbols for per_context #58217
jazelly wants to merge 2 commits into
nodejs:v22.x-stagingfrom
jazelly:backport-57479

Conversation

jazelly commented May 7, 2025
edited by aduh95
Loading

Copy link
Copy Markdown
Member

Backport 2 commits where the second one was rebased on the first one.

src: ensure primordials are initialized exactly once
src: initialize privateSymbols for per_context

The manual backport bypassed the usage of Get/SetPrototypeV2 that does not exist on v22.x.

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/realm
  • @nodejs/startup

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. v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch. labels May 7, 2025
jazelly added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label May 7, 2025
jazelly closed this May 10, 2025

Copy link
Copy Markdown
Member

@jazelly Any reason why this was closed? Would you like to pick it up again?

jazelly commented May 13, 2025

Copy link
Copy Markdown
Member Author

I got a couple of JS crash/OOM in GHA like https://github.com/nodejs/node/actions/runs/14944261967/job/41985588150?pr=58217

I was thinking maybe this cannot be backported without backporting #55453? Was closing this to give me more time to debug this.

Copy link
Copy Markdown
Member

I think these crashes are irrelevant to this change.

jazelly reopened this May 13, 2025

jazelly commented May 13, 2025
edited
Loading

Copy link
Copy Markdown
Member Author

For this GHA failed build on windows

D:\a\node\node\deps\v8\include\v8-internal.h(1159,36): error C2607: static assertion failed [D:\a\node\node\libnode.vcxproj]
  (compiling source file '/src/node_messaging.cc')
  
D:\a\node\node\deps\v8\include\v8-internal.h(1180,37): error C7683: you cannot create a reference to 'void' [D:\a\node\node\libnode.vcxproj]
  (compiling source file '/src/node_messaging.cc')

I got it reproduced in my windows machine, and reverting #57578 fixed it.

It failed due to a major change that lives in v23 and after but not before, specifically this one.

The LocalVector was introduced before/in v22, but it was then stablized in v23 with that change. I don't think #57578 should be back ported to v22.

Not sure if we have v22.x-staging build periodically. cause I believe the v22.x staging won't build on windows successfully with that LocalVector change.

Copy link
Copy Markdown
Member

Not sure if we have v22.x-staging build periodically. cause I believe the v22.x staging won't build on windows successfully with that LocalVector change.

We have daily builds of v22.x-staging https://ci.nodejs.org/view/Node.js%20Daily/job/node-daily-v22.x-staging/
(Link won't work right now for most people as the Jenkins CI is restricted at the moment while we're preparing this week's security release.)

It looks like Windows builds have been failing since 7 May with e0a025a -- they were passing the day before with 3f5899f.

jazelly commented May 13, 2025
edited
Loading

Copy link
Copy Markdown
Member Author

I got it reproduced in my windows machine, and reverting #57578 fixed it.

Sorry I put a wrong link before. I was talking about this back port commit from #57733

it looks like Windows builds have been failing since 7 May with e0a025a

#57733 was back ported to v22.x-staging 5 hours before that, so it could be the one that causing the failed build.

If the failure log on windows about the the static assertion, then I think we need to revert that back port.

D:\a\node\node\deps\v8\include\v8-internal.h(1159,36): error C2607: static assertion failed [D:\a\node\node\libnode.vcxproj]
  (compiling source file '/src/node_messaging.cc')
  
D:\a\node\node\deps\v8\include\v8-internal.h(1180,37): error C7683: you cannot create a reference to 'void' [D:\a\node\node\libnode.vcxproj]
  (compiling source file '/src/node_messaging.cc')

I can raise a separate revert PR to revert backport c408a7f. But I am not sure about the OOM on linux, which I guess are also failing the v22.x-staging build.

RafaelGSS requested a review from a team as a code owner May 14, 2025 20:50

aduh95 commented May 16, 2025

Copy link
Copy Markdown
Contributor

@jazelly thanks for investigating that, I'm able to confirm that c408a7f is the culprit:
https://ci.nodejs.org/job/node-test-commit-windows-fanned/70430/ passes and https://ci.nodejs.org/job/node-test-commit-windows-fanned/70436/ doesn't.

I've removed it from the staging branch

jazelly force-pushed the backport-57479 branch 2 times, most recently from e611888 to 1e5c8a2 Compare May 17, 2025 01:00
aduh95 force-pushed the v22.x-staging branch 3 times, most recently from e0a025a to d03f13a Compare May 17, 2025 13:54
aduh95 changed the title [v22.x backport] Backport 57519 and 57479 [v22.x backport] src: initialize privateSymbols for per_context May 17, 2025

Copy link
Copy Markdown
Collaborator

Copy link
Copy Markdown
Collaborator

legendecas and others added 2 commits May 18, 2025 22:35
PR-URL: nodejs#57519
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#57479
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

Copy link
Copy Markdown
Collaborator

aduh95 commented May 19, 2025

Copy link
Copy Markdown
Contributor

Landed in 6ea250e...a376d10

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v22.x Issues that can be reproduced on v22.x or PRs targeting the v22.x-staging branch.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants


Back | FazBrowse Home | New Git URL