Skip to content

Conversation

joyeecheung
Copy link
Member

The string writing/reading was intended for debugging info in snapshot, which had a CHECK_GT(length, 0) check, it then got repurposed for SEA resource writing/reading and turned into a helper for string views, but was not updated to handle empty views, causing occasional crash in the CI when the read is protected. This patch fixes it.

Fixes: #50740

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Mar 7, 2024
@targos
Copy link
Member

targos commented Mar 7, 2024

The functions are supposed to return something.

The string writing/reading was intended for debugging info
in snapshot, which had a CHECK_GT(length, 0) check, it then
got repurposed for SEA resource writing/reading and turned
into a helper for string views, but was not updated to handle
empty views, causing occasional crash in the CI when the
read is protected. This patch fixes it.
@joyeecheung
Copy link
Member Author

oops, forgot to commit my local changes before pushing

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 7, 2024
@nodejs-github-bot
Copy link
Collaborator

@targos
Copy link
Member

targos commented Mar 7, 2024

This seems to break tests instead of removing flakyness.

@joyeecheung
Copy link
Member Author

Hmm, need to update the assertion in LoadEnvironment..

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 7, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Mar 10, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 10, 2024
@nodejs-github-bot nodejs-github-bot merged commit 1f19316 into nodejs:main Mar 10, 2024
@nodejs-github-bot
Copy link
Collaborator

Landed in 1f19316

rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
The string writing/reading was intended for debugging info
in snapshot, which had a CHECK_GT(length, 0) check, it then
got repurposed for SEA resource writing/reading and turned
into a helper for string views, but was not updated to handle
empty views, causing occasional crash in the CI when the
read is protected. This patch fixes it.

PR-URL: nodejs#52000
Fixes: nodejs#50740
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
marco-ippolito pushed a commit that referenced this pull request May 2, 2024
The string writing/reading was intended for debugging info
in snapshot, which had a CHECK_GT(length, 0) check, it then
got repurposed for SEA resource writing/reading and turned
into a helper for string views, but was not updated to handle
empty views, causing occasional crash in the CI when the
read is protected. This patch fixes it.

PR-URL: #52000
Fixes: #50740
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
jcbhmr pushed a commit to jcbhmr/node that referenced this pull request May 15, 2024
The string writing/reading was intended for debugging info
in snapshot, which had a CHECK_GT(length, 0) check, it then
got repurposed for SEA resource writing/reading and turned
into a helper for string views, but was not updated to handle
empty views, causing occasional crash in the CI when the
read is protected. This patch fixes it.

PR-URL: nodejs#52000
Fixes: nodejs#50740
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
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-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SEA tests run into SIGSEGV during SEA execution
5 participants