fix: correct utility process exit code on Windows#50256
fix: correct utility process exit code on Windows#50256jkleinsc merged 2 commits intoelectron:mainfrom
Conversation
|
💖 Thanks for opening this pull request! 💖 Semantic PR titlesWe use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of commit messages with semantic prefixes:
Commit signingThis repo enforces commit signatures for all incoming PRs. PR tipsThings that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
|
@Jomy2323, could you add test coverage for this case? Be sure to test high values on all platforms, and confirm the behavior is correct for each platform. This change likely needs to be scoped to only do the |
|
@dsanders11, of course, ill look into that! Thank you for the review! |
deepak1556
left a comment
There was a problem hiding this comment.
Thanks the change is good, however I would prefer that the UtilityProcessWrapper::HandleTermination only takes uint32_t exit codes, there is no reason for it to require uint64_t, it must have been accidentally added due to UtilityProcessWrapper::Shutdown but that just has user generated success values. Its only OnServiceProcessCrashed or OnServiceProcessTerminatedNormally that return platform specific exit codes which is what we care about and the mojo disconnection already carries uint32_t for Node.js process exit codes OnServiceProcessDisconnected.
|
Okay, ill look into that. Thank you for your suggestion and feedback! |
|
@Jomy2323 we require signed commits for our PRS: https://www.electronjs.org/docs/latest/development/pull-requests#commit-signing. Please sign your commits so that we can merge this PR. |
On Windows, process exit codes are 32-bit unsigned integers (DWORD). When passed from Chromium to Electron as a signed int and then implicitly converted to uint64_t, values with the high bit set (e.g., NTSTATUS codes) undergo sign extension, producing incorrect values. Cast the exit code to uint32_t before widening to uint64_t to prevent sign extension and preserve the original Windows exit code. Fixes electron#49455
826be53 to
440ae3f
Compare
|
@jkleinsc there you go. Thank you for your help! |
|
Congrats on merging your first pull request! 🎉🎉🎉 |
|
Release Notes Persisted
|
|
I have automatically backported this PR to "42-x-y", please check out #50385 |
|
I have automatically backported this PR to "41-x-y", please check out #50386 |
|
I have automatically backported this PR to "40-x-y", please check out #50387 |
On Windows, process exit codes are 32-bit unsigned integers (DWORD). When passed from Chromium to Electron as a signed int and then implicitly converted to uint64_t, values with the high bit set (e.g., NTSTATUS codes) undergo sign extension, producing incorrect values.
Cast the exit code to uint32_t before widening to uint64_t to prevent sign extension and preserve the original Windows exit code.
Fixes #49455
Description of Change
Earlier today I opened a PR for this issue, however, having used chat gpt to write its description, I'm afraid the electron bot tagged it as AI spam. That is not the case, the bug fix was a simple two line fix done by myself after investigating the cause. The problem was Chrome converts the windows error code from an unsigned 32 bit integer to a signed 32 bit integer, and then electron casts it to a 64 bit integer and does a sign extension. So for errors with highest bit set to 1 it will extend with 32 bits set to 1 on the left, giving us the unreal error codes. I ran the tests regarding the utilityProcess and all 55 tests passed.
Here you can see the print of the tests successfully passing:
Thanks for your attention and sorry to have bothered you earlier today.
Checklist
npm testpassesRelease Notes
Notes: Fixed utilityProcess exit event reporting incorrect exit codes on Windows when the exit code has the high bit
set.