Skip to content

fix: runtime JS error that crashes GetPackageJSON #48293

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
codebytere merged 1 commit into from
Sep 30, 2025
Merged

fix: runtime JS error that crashes GetPackageJSON #48293

codebytere merged 1 commit into from
Sep 30, 2025

Conversation

@indutny-signal
Copy link
Contributor

@indutny-signal indutny-signal commented Sep 10, 2025 •
edited
Loading

Description of Change

We overriden the GetPackageJSON in Node.js to let us read files straight from the ASAR file instead of disk. The override works by providing a JS method with the limitation that it should not throw a runtime error. However, this invariant was accidentally violated by asar.splitPath that sometimes contrary to its' TypeScript definition returned false.

Checklist

cc @zcbenz @nikwen

Example of crash stack

0: libsystem_kernel.dylib +0x[REDACTED]
1: libsystem_c.dylib +0x0000000000078a38
2: node::OnFatalError(char const*, char const*)+0xf8 (node_errors.cc:593)
3: v8::Utils::ReportApiFailure(char const*, char const*)+0x4c (api.cc:272)
4: node::modules::BindingData::GetPackageJSON(node::Realm*, std::__Cr::basic_string_view<char, std::__Cr::char_traits<char>>, node::modules::BindingData::ErrorContext*)+0x15f8 (v8-local-handle.h:770)
5: node::modules::BindingData::TraverseParent(node::Realm*, std::__Cr::__fs::filesystem::path const&)+0x460 (node_modules.cc:396)
6: node::modules::BindingData::GetNearestParentPackageJSON(v8::FunctionCallbackInfo<v8::Value> const&)+0x270 (node_modules.cc:432)

Release Notes

Notes: fix: runtime JS error that crashes GetPackageJSON

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Sep 10, 2025
@indutny-signal
Copy link
Contributor Author

indutny-signal commented Sep 10, 2025 •
edited
Loading

FWIW, the type definition for asar.splitPath is

electron/typings/internal-ambient.d.ts

Lines 91 to 97 in f702327

splitPath(path: string): {
isAsar: false;
} | {
isAsar: true;
asarPath: string;
filePath: string;
};

@deepak1556 deepak1556 added semver/patch backwards-compatible bug fixes target/37-x-y PR should also be added to the "37-x-y" branch. target/38-x-y PR should also be added to the "38-x-y" branch. target/39-x-y PR should also be added to the "39-x-y" branch. labels Sep 10, 2025
deepak1556
deepak1556 approved these changes Sep 10, 2025
View reviewed changes
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Sep 11, 2025
@jkleinsc
Copy link
Member

jkleinsc commented Sep 15, 2025

@indutny-signal can you rebase your PR to pick up a fix for the compilation errors?

@indutny-signal
Copy link
Contributor Author

indutny-signal commented Sep 15, 2025

Force pushed! Signed the commit while I was there. Thank you!

@jkleinsc jkleinsc mentioned this pull request Sep 17, 2025
Merged
3 tasks
@jkleinsc
Copy link
Member

jkleinsc commented Sep 18, 2025

@indutny-signal sorry to ask - can you rebase again to pull in #48345? I tested this fix with your branch and it fixes the build issue seen here.

@indutny-signal
fix: runtime JS error that crashes GetPackageJSON
77bb87b 
We overriden the `GetPackageJSON` in Node.js to let us read files
straight from the ASAR file instead of disk. The override works by
providing a JS method with the limitation that it should not throw a
runtime error. However, this invariant was accidentally violated by
`asar.splitPath` that sometimes contrary to its' TypeScript definition
returned `false`.
@indutny-signal
Copy link
Contributor Author

indutny-signal commented Sep 18, 2025

@jkleinsc force pushed! np!

@indutny-signal
Copy link
Contributor Author

indutny-signal commented Sep 25, 2025

@jkleinsc no rush with reviewing it, but let me know if you want me to rebase again!

@codebytere
Copy link
Member

codebytere commented Sep 25, 2025

@indutny-signal reran ci and will merge when green!

@indutny-signal
Copy link
Contributor Author

indutny-signal commented Sep 25, 2025

Looks like it is still not green, but I think the test is flaky?

@indutny-signal
Copy link
Contributor Author

indutny-signal commented Sep 30, 2025

Should we try once more? 😅

@codebytere codebytere merged commit 6f9cd71 into electron:main Sep 30, 2025
324 of 337 checks passed
@release-clerk
Copy link

release-clerk bot commented Sep 30, 2025

Release Notes Persisted

fix: runtime JS error that crashes GetPackageJSON

This was referenced Sep 30, 2025
Merged
Merged
Merged
@trop
Copy link
Contributor

trop bot commented Sep 30, 2025

I have automatically backported this PR to "37-x-y", please check out #48423

@trop trop bot added in-flight/37-x-y and removed target/37-x-y PR should also be added to the "37-x-y" branch. labels Sep 30, 2025
@trop
Copy link
Contributor

trop bot commented Sep 30, 2025

I have automatically backported this PR to "38-x-y", please check out #48424

@trop
Copy link
Contributor

trop bot commented Sep 30, 2025

I have automatically backported this PR to "39-x-y", please check out #48425

@trop trop bot added in-flight/38-x-y in-flight/39-x-y and removed target/38-x-y PR should also be added to the "38-x-y" branch. target/39-x-y PR should also be added to the "39-x-y" branch. labels Sep 30, 2025
@indutny-signal
Copy link
Contributor Author

indutny-signal commented Sep 30, 2025

Aww, thank you!

@indutny-signal indutny-signal deleted the fix/asar-override-crash branch September 30, 2025 16:46
@trop trop bot added merged/39-x-y PR was merged to the "39-x-y" branch. merged/37-x-y PR was merged to the "37-x-y" branch. merged/38-x-y PR was merged to the "38-x-y" branch. and removed in-flight/39-x-y in-flight/37-x-y in-flight/38-x-y labels Oct 2, 2025
TheCommieAxolotl pushed a commit to TheCommieAxolotl/electron that referenced this pull request Nov 2, 2025
@indutny-signal @TheCommieAxolotl
fix: runtime JS error that crashes GetPackageJSON (electron#48293)
6c10725 
We overriden the `GetPackageJSON` in Node.js to let us read files
straight from the ASAR file instead of disk. The override works by
providing a JS method with the limitation that it should not throw a
runtime error. However, this invariant was accidentally violated by
`asar.splitPath` that sometimes contrary to its' TypeScript definition
returned `false`.
nilayarya pushed a commit to nilayarya/electron that referenced this pull request Nov 21, 2025
@indutny-signal
fix: runtime JS error that crashes GetPackageJSON (electron#48293)
238197a 
We overriden the `GetPackageJSON` in Node.js to let us read files
straight from the ASAR file instead of disk. The override works by
providing a JS method with the limitation that it should not throw a
runtime error. However, this invariant was accidentally violated by
`asar.splitPath` that sometimes contrary to its' TypeScript definition
returned `false`.
nilayarya added a commit to nilayarya/electron that referenced this pull request Nov 21, 2025
@nilayarya
fix: runtime JS error that crashes GetPackageJSON (electron#48293)
946f153 
We overriden the `GetPackageJSON` in Node.js to let us read files
straight from the ASAR file instead of disk. The override works by
providing a JS method with the limitation that it should not throw a
runtime error. However, this invariant was accidentally violated by
`asar.splitPath` that sometimes contrary to its' TypeScript definition
returned `false`.
nilayarya added a commit to nilayarya/electron that referenced this pull request Nov 21, 2025
@nilayarya
fix: runtime JS error that crashes GetPackageJSON (electron#48293)
8500152 
We overriden the `GetPackageJSON` in Node.js to let us read files
straight from the ASAR file instead of disk. The override works by
providing a JS method with the limitation that it should not throw a
runtime error. However, this invariant was accidentally violated by
`asar.splitPath` that sometimes contrary to its' TypeScript definition
returned `false`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deepak1556 deepak1556 deepak1556 approved these changes

@codebytere codebytere codebytere approved these changes

Assignees

No one assigned

Labels

merged/37-x-y PR was merged to the "37-x-y" branch. merged/38-x-y PR was merged to the "38-x-y" branch. merged/39-x-y PR was merged to the "39-x-y" branch. semver/patch backwards-compatible bug fixes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@indutny-signal @jkleinsc @codebytere @deepak1556