src: fixup Error.stackTraceLimit during snapshot building #55121

joyeecheung · 2024-09-25T18:19:10Z

src: parse --stack-trace-limit and use it in --trace-* flags

Previously, --trace-exit and --trace-sync-io doesn't take care
of --stack-trace-limit and always print a stack trace with maximum
size of 10. This patch parses --stack-trace-limit during
initialization and use the value in --trace-* flags.

src: fixup Error.stackTraceLimit during snapshot building

When V8 creates a context for snapshot building, it does not
install Error.stackTraceLimit. As a result, error.stack would
be undefined in the snapshot builder script unless users
explicitly initialize Error.stackTraceLimit, which may be
surprising.

This patch initializes Error.stackTraceLimit based on the
value of --stack-trace-limit to prevent the surprise. If
users have modified Error.stackTraceLimit in the builder
script, the modified value would be restored during
deserialization. Otherwise, the fixed up limit would be
deleted since V8 expects to find it unset and re-initialize
it during snapshot deserialization.

Fixes: #55100

Previously, --trace-exit and --trace-sync-io doesn't take care of --stack-trace-limit and always print a stack trace with maximum size of 10. This patch parses --stack-trace-limit during initialization and use the value in --trace-* flags.

When V8 creates a context for snapshot building, it does not install Error.stackTraceLimit. As a result, error.stack would be undefined in the snapshot builder script unless users explicitly initialize Error.stackTraceLimit, which may be surprising. This patch initializes Error.stackTraceLimit based on the value of --stack-trace-limit to prevent the surprise. If users have modified Error.stackTraceLimit in the builder script, the modified value would be restored during deserialization. Otherwise, the fixed up limit would be deleted since V8 expects to find it unset and re-initialize it during snapshot deserialization.

nodejs-github-bot · 2024-09-25T18:19:15Z

Review requested:

@nodejs/startup

joyeecheung

I feel that the Error.stackTraceLimit stuff should be documented (and the lack of WebAssembly in the snapshot building context too), but then neither cli.md nor the JS API doc in v8.md feel like the right place to talk about it, so I'll just leave it out and open a separate PR to split a dedicated doc about snapshots and cover them there.

nodejs-github-bot · 2024-09-25T18:35:03Z

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

codecov · 2024-09-25T19:43:53Z

Codecov Report

Attention: Patch coverage is 87.17949% with 10 lines in your changes missing coverage. Please review.

Project coverage is 88.24%. Comparing base (b64006c) to head (ceeee4d).
Report is 406 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/main/mksnapshot.js	87.75%	6 Missing ⚠️
src/node_options.cc	50.00%	2 Missing and 1 partial ⚠️
src/node_options-inl.h	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #55121   +/-   ##
=======================================
  Coverage   88.24%   88.24%           
=======================================
  Files         652      651    -1     
  Lines      183912   183960   +48     
  Branches    35862    35866    +4     
=======================================
+ Hits       162296   162342   +46     
+ Misses      14905    14901    -4     
- Partials     6711     6717    +6

Files with missing lines	Coverage Δ
lib/internal/v8/startup_snapshot.js	`99.23% <100.00%> (+0.06%)`	⬆️
src/env-inl.h	`96.56% <100.00%> (+0.01%)`	⬆️
src/env.cc	`85.57% <100.00%> (+0.02%)`	⬆️
src/env.h	`97.95% <ø> (-0.05%)`	⬇️
src/node_options.h	`98.22% <100.00%> (+0.01%)`	⬆️
src/node_options-inl.h	`80.80% <50.00%> (-0.28%)`	⬇️
src/node_options.cc	`87.39% <50.00%> (-0.66%)`	⬇️
lib/internal/main/mksnapshot.js	`95.90% <87.75%> (-1.39%)`	⬇️

... and 59 files with indirect coverage changes

legendecas

LGTM! Since we added addAfterUserSerializeCallback specifically, do we want to cover the case where Error.stackTraceLimit is modified in a user serialize callback?

joyeecheung · 2024-09-26T14:38:27Z

do we want to cover the case where Error.stackTraceLimit is modified in a user serialize callback?

hmm I think this PR already does it, but yeah it's missing a test. Will add one

legendecas · 2024-09-26T14:43:30Z

Yes, I meant a test coverage.

nodejs-github-bot · 2024-09-26T19:52:09Z

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

lib/internal/main/mksnapshot.js

Co-authored-by: Ashley Claymore <aclaymore@bloomberg.net>

nodejs-github-bot · 2024-09-27T12:05:59Z

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

nodejs-github-bot · 2024-09-27T15:35:05Z

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

nodejs-github-bot · 2024-09-29T17:38:55Z

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

nodejs-github-bot · 2024-09-30T12:56:29Z

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

nodejs-github-bot · 2024-09-30T15:05:14Z

Landed in 371ed85...33bbf37

Previously, --trace-exit and --trace-sync-io doesn't take care of --stack-trace-limit and always print a stack trace with maximum size of 10. This patch parses --stack-trace-limit during initialization and use the value in --trace-* flags. PR-URL: #55121 Fixes: #55100 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

When V8 creates a context for snapshot building, it does not install Error.stackTraceLimit. As a result, error.stack would be undefined in the snapshot builder script unless users explicitly initialize Error.stackTraceLimit, which may be surprising. This patch initializes Error.stackTraceLimit based on the value of --stack-trace-limit to prevent the surprise. If users have modified Error.stackTraceLimit in the builder script, the modified value would be restored during deserialization. Otherwise, the fixed up limit would be deleted since V8 expects to find it unset and re-initialize it during snapshot deserialization. PR-URL: #55121 Fixes: #55100 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

Previously, --trace-exit and --trace-sync-io doesn't take care of --stack-trace-limit and always print a stack trace with maximum size of 10. This patch parses --stack-trace-limit during initialization and use the value in --trace-* flags. PR-URL: #55121 Fixes: #55100 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

When V8 creates a context for snapshot building, it does not install Error.stackTraceLimit. As a result, error.stack would be undefined in the snapshot builder script unless users explicitly initialize Error.stackTraceLimit, which may be surprising. This patch initializes Error.stackTraceLimit based on the value of --stack-trace-limit to prevent the surprise. If users have modified Error.stackTraceLimit in the builder script, the modified value would be restored during deserialization. Otherwise, the fixed up limit would be deleted since V8 expects to find it unset and re-initialize it during snapshot deserialization. PR-URL: #55121 Fixes: #55100 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

Previously, --trace-exit and --trace-sync-io doesn't take care of --stack-trace-limit and always print a stack trace with maximum size of 10. This patch parses --stack-trace-limit during initialization and use the value in --trace-* flags. PR-URL: #55121 Fixes: #55100 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

When V8 creates a context for snapshot building, it does not install Error.stackTraceLimit. As a result, error.stack would be undefined in the snapshot builder script unless users explicitly initialize Error.stackTraceLimit, which may be surprising. This patch initializes Error.stackTraceLimit based on the value of --stack-trace-limit to prevent the surprise. If users have modified Error.stackTraceLimit in the builder script, the modified value would be restored during deserialization. Otherwise, the fixed up limit would be deleted since V8 expects to find it unset and re-initialize it during snapshot deserialization. PR-URL: #55121 Fixes: #55100 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

Previously, --trace-exit and --trace-sync-io doesn't take care of --stack-trace-limit and always print a stack trace with maximum size of 10. This patch parses --stack-trace-limit during initialization and use the value in --trace-* flags. PR-URL: nodejs#55121 Fixes: nodejs#55100 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

When V8 creates a context for snapshot building, it does not install Error.stackTraceLimit. As a result, error.stack would be undefined in the snapshot builder script unless users explicitly initialize Error.stackTraceLimit, which may be surprising. This patch initializes Error.stackTraceLimit based on the value of --stack-trace-limit to prevent the surprise. If users have modified Error.stackTraceLimit in the builder script, the modified value would be restored during deserialization. Otherwise, the fixed up limit would be deleted since V8 expects to find it unset and re-initialize it during snapshot deserialization. PR-URL: nodejs#55121 Fixes: nodejs#55100 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

Previously, --trace-exit and --trace-sync-io doesn't take care of --stack-trace-limit and always print a stack trace with maximum size of 10. This patch parses --stack-trace-limit during initialization and use the value in --trace-* flags. PR-URL: nodejs#55121 Fixes: nodejs#55100 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

When V8 creates a context for snapshot building, it does not install Error.stackTraceLimit. As a result, error.stack would be undefined in the snapshot builder script unless users explicitly initialize Error.stackTraceLimit, which may be surprising. This patch initializes Error.stackTraceLimit based on the value of --stack-trace-limit to prevent the surprise. If users have modified Error.stackTraceLimit in the builder script, the modified value would be restored during deserialization. Otherwise, the fixed up limit would be deleted since V8 expects to find it unset and re-initialize it during snapshot deserialization. PR-URL: nodejs#55121 Fixes: nodejs#55100 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

nodejs/node#55121

* chore: bump node in DEPS to v22.11.0 * src: move evp stuff to ncrypto nodejs/node#54911 * crypto: add Date fields for validTo and validFrom nodejs/node#54159 * module: fix discrepancy between .ts and .js nodejs/node#54461 * esm: do not interpret "main" as a URL nodejs/node#55003 * src: modernize likely/unlikely hints nodejs/node#55155 * chore: update patch indices * crypto: add validFromDate and validToDate fields to X509Certificate nodejs/node#54159 * chore: fixup perfetto patch * fix: clang warning in simdjson * src: add receiver to fast api callback methods nodejs/node#54408 * chore: fixup revert patch * fixup! esm: do not interpret "main" as a URL * fixup! crypto: add Date fields for validTo and validFrom * fix: move ArrayBuffer test patch * src: fixup Error.stackTraceLimit during snapshot building nodejs/node#55121 * fix: bad rebase * chore: fixup amaro * chore: address feedback from review * src: revert filesystem::path changes nodejs/node#55015 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

nodejs/node#55121

* chore: bump node in DEPS to v22.13.0 * chore: bump node in DEPS to v22.13.1 * src: move evp stuff to ncrypto nodejs/node#54911 * crypto: add Date fields for validTo and validFrom nodejs/node#54159 * module: fix discrepancy between .ts and .js nodejs/node#54461 * esm: do not interpret "main" as a URL nodejs/node#55003 * src: modernize likely/unlikely hints nodejs/node#55155 * chore: update patch indices * crypto: add validFromDate and validToDate fields to X509Certificate nodejs/node#54159 * chore: fixup perfetto patch * fix: clang warning in simdjson * src: add receiver to fast api callback methods nodejs/node#54408 * chore: fixup revert patch * fixup! esm: do not interpret "main" as a URL * fixup! crypto: add Date fields for validTo and validFrom * fix: move ArrayBuffer test patch * src: fixup Error.stackTraceLimit during snapshot building nodejs/node#55121 * fix: bad rebase * chore: fixup amaro * chore: address feedback from review * src: revert filesystem::path changes nodejs/node#55015 * chore: fixup GN build file * nodejs/node#55529 * nodejs/node#55798 * nodejs/node#55530 * module: simplify --inspect-brk handling nodejs/node#55679 * src: fix outdated js2c.cc references nodejs/node#56133 * crypto: include openssl/rand.h explicitly nodejs/node#55425 * build: use variable for crypto dep path nodejs/node#55928 * crypto: fix RSA_PKCS1_PADDING error message nodejs/node#55629 * build: use variable for simdutf path nodejs/node#56196 * test,crypto: make crypto tests work with BoringSSL nodejs/node#55491 * fix: suppress clang -Wdeprecated-declarations in libuv libuv/libuv#4486 * deps: update libuv to 1.49.1 nodejs/node#55114 * test: make test-node-output-v8-warning more flexible nodejs/node#55401 * [v22.x] Revert "v8: enable maglev on supported architectures" nodejs/node#54384 * fix: potential WIN32_LEAN_AND_MEAN redefinition c-ares/c-ares#869 * deps: update nghttp2 to 1.64.0 nodejs/node#55559 * src: provide workaround for container-overflow nodejs/node#55591 * build: use variable for simdutf path nodejs/node#56196 * chore: fixup patch indices * fixup! module: simplify --inspect-brk handling * lib: fix fs.readdir recursive async nodejs/node#56041 * lib: avoid excluding symlinks in recursive fs.readdir with filetypes nodejs/node#55714 This doesn't currently play well with ASAR - this should be fixed in a follow up * test: disable CJS permission test for config.main This has diverged as a result of our revert of src,lb: reducing C++ calls of esm legacy main resolve * fixup! lib: fix fs.readdir recursive async * deps: update libuv to 1.49.1 nodejs/node#55114 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

joyeecheung added 2 commits September 25, 2024 20:16

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. labels Sep 25, 2024

joyeecheung requested a review from legendecas September 25, 2024 18:19

joyeecheung commented Sep 25, 2024

View reviewed changes

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2024

targos approved these changes Sep 26, 2024

View reviewed changes

legendecas approved these changes Sep 26, 2024

View reviewed changes

fixup! src: fixup Error.stackTraceLimit during snapshot building

644043c

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 26, 2024

acutmore reviewed Sep 26, 2024

View reviewed changes

lib/internal/main/mksnapshot.js Outdated Show resolved Hide resolved

lib/internal/main/mksnapshot.js Outdated Show resolved Hide resolved

lib/internal/main/mksnapshot.js Outdated Show resolved Hide resolved

lib/internal/main/mksnapshot.js Outdated Show resolved Hide resolved

fixup! src: fixup Error.stackTraceLimit during snapshot building

ceeee4d

Co-authored-by: Ashley Claymore <aclaymore@bloomberg.net>

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 27, 2024

legendecas approved these changes Sep 27, 2024

View reviewed changes

joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 27, 2024

joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Sep 30, 2024

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 30, 2024

nodejs-github-bot closed this Sep 30, 2024

aduh95 mentioned this pull request Oct 9, 2024

V22.10.0 proposal #55343

Merged

marco-ippolito added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Nov 16, 2024

codebytere added a commit to electron/electron that referenced this pull request Nov 22, 2024

src: fixup Error.stackTraceLimit during snapshot building

666967f

nodejs/node#55121

codebytere added a commit to electron/electron that referenced this pull request Dec 3, 2024

src: fixup Error.stackTraceLimit during snapshot building

4b7affd

nodejs/node#55121

codebytere added a commit to electron/electron that referenced this pull request Dec 4, 2024

src: fixup Error.stackTraceLimit during snapshot building

53f47e7

nodejs/node#55121

codebytere added a commit to electron/electron that referenced this pull request Jan 22, 2025

src: fixup Error.stackTraceLimit during snapshot building

6b51859

nodejs/node#55121

codebytere added a commit to electron/electron that referenced this pull request Feb 3, 2025

src: fixup Error.stackTraceLimit during snapshot building

db9f032

nodejs/node#55121

codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025

src: fixup Error.stackTraceLimit during snapshot building

e7ab6fc

nodejs/node#55121

codebytere added a commit to electron/electron that referenced this pull request Feb 10, 2025

src: fixup Error.stackTraceLimit during snapshot building

3a8825a

nodejs/node#55121

codebytere added a commit to electron/electron that referenced this pull request Feb 14, 2025

src: fixup Error.stackTraceLimit during snapshot building

7fd709a

nodejs/node#55121

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: fixup Error.stackTraceLimit during snapshot building #55121

src: fixup Error.stackTraceLimit during snapshot building #55121

joyeecheung commented Sep 25, 2024 •

edited

Loading

nodejs-github-bot commented Sep 25, 2024

joyeecheung left a comment

nodejs-github-bot commented Sep 25, 2024

codecov bot commented Sep 25, 2024 •

edited

Loading

legendecas left a comment

joyeecheung commented Sep 26, 2024

legendecas commented Sep 26, 2024 •

edited

Loading

nodejs-github-bot commented Sep 26, 2024

nodejs-github-bot commented Sep 27, 2024

nodejs-github-bot commented Sep 27, 2024

nodejs-github-bot commented Sep 29, 2024

nodejs-github-bot commented Sep 30, 2024

nodejs-github-bot commented Sep 30, 2024

src: fixup Error.stackTraceLimit during snapshot building #55121

src: fixup Error.stackTraceLimit during snapshot building #55121

Conversation

joyeecheung commented Sep 25, 2024 • edited Loading

src: parse --stack-trace-limit and use it in --trace-* flags

src: fixup Error.stackTraceLimit during snapshot building

nodejs-github-bot commented Sep 25, 2024

joyeecheung left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Sep 25, 2024

codecov bot commented Sep 25, 2024 • edited Loading

Codecov Report

legendecas left a comment

Choose a reason for hiding this comment

joyeecheung commented Sep 26, 2024

legendecas commented Sep 26, 2024 • edited Loading

nodejs-github-bot commented Sep 26, 2024

nodejs-github-bot commented Sep 27, 2024

nodejs-github-bot commented Sep 27, 2024

nodejs-github-bot commented Sep 29, 2024

nodejs-github-bot commented Sep 30, 2024

nodejs-github-bot commented Sep 30, 2024

joyeecheung commented Sep 25, 2024 •

edited

Loading

codecov bot commented Sep 25, 2024 •

edited

Loading

legendecas commented Sep 26, 2024 •

edited

Loading