Skip to content
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.

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

fix: about panel crash #37373

Merged
merged 2 commits into from
Feb 28, 2023
Merged

fix: about panel crash #37373

merged 2 commits into from
Feb 28, 2023

Conversation

clavin
Copy link
Member

@clavin clavin commented Feb 21, 2023

Description of Change

This code causes a crash bc the about_panel_options_ gets initialized as a base::Value, thus not a Dict, and the calls to GetDict() fail. This just changes that type to a base::Value::Dict and everything 🪄 magically ✨ works. Added a sanity test as a cherry on top.

EDIT: This PR now also unifies every platform by making showAboutPanel run asynchronously, where before it varied platform to platform. I think this would be the sensible default for the about panel (fire and forget), but if "run code after closing" is desired, either recommend users to pursue a different solution (e.g. use a BrowserWindow) or we can choose to implement the callback proper later. This will be broken into a separate PR.

Release Notes

Notes: Setting the about panel's options no longer crashes.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Feb 21, 2023
@codebytere codebytere added semver/patch backwards-compatible bug fixes target/22-x-y PR should also be added to the "22-x-y" branch. target/23-x-y PR should also be added to the "23-x-y" branch. target/24-x-y PR should also be added to the "24-x-y" branch. labels Feb 22, 2023
@MarshallOfSound
Copy link
Member

@deepak1556 It actually crashed inside SetAboutPanelOptions --> https://github.com/electron/electron/pull/37373/files#diff-43f417c3f78558ea55277f32e1a078b6bb6c19be7699a2cf61c7020d78f1110dL540 because GetDict() was invalid on an uninitialized base value

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Feb 22, 2023
Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

@clavin this is stalling/failing on Windows

@clavin
Copy link
Member Author

clavin commented Feb 24, 2023

Yeah I'm trying to get a build going on my Windows machine to figure out why the app is just hanging when it gets to the new tests

@clavin
Copy link
Member Author

clavin commented Feb 24, 2023

Apparently app.showAboutPanel() is a blocking call on Windows and Linux, but not on macOS 😶

@codebytere
Copy link
Member

@clavin imo just gate it to linux for now

@clavin clavin changed the title fix: about panel is a base::Value::Dict fix: about panel crash & always async Feb 27, 2023
@clavin
Copy link
Member Author

clavin commented Feb 27, 2023

I fixed the issue by making this API non-blocking on all platforms, instead of just on one. That might mean this should have a different semver? To be clear, the docs didn't state the behavior one way or the other, but this still could theoretically cause code relying on the blocking (if there is any???) to execute earlier than intended.

@clavin
Copy link
Member Author

clavin commented Feb 27, 2023

I'm going to mark this as needing API review, just in case.

@deepak1556
Copy link
Member

It would be good to split the async behavior change to a different PR for api-wg review, the dictionary crash fix is good to merge irrespective of it.

Copy link
Member

@erickzhao erickzhao left a comment

Choose a reason for hiding this comment

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

API LGTM

@clavin
Copy link
Member Author

clavin commented Feb 28, 2023

I did both at the same time because I wanted to add a test covering the crash, which is impossible when the method is synchronous :/

But I'm happy to split the test & async change both into a separate PR if it makes things easier :)

@deepak1556
Copy link
Member

I think the test for setAboutPanelOptions is good enough for this PR to cover #37373 (comment), adding test for showAboutPanel can be in the async refactor PR.

@clavin clavin changed the title fix: about panel crash & always async fix: about panel crash Feb 28, 2023
@clavin
Copy link
Member Author

clavin commented Feb 28, 2023

Done 🫡 This PR now only has the crash fix and the API change will be split into a separate PR

@jkleinsc jkleinsc merged commit 2e03bdb into main Feb 28, 2023
@release-clerk
Copy link

release-clerk bot commented Feb 28, 2023

Release Notes Persisted

Setting the about panel's options no longer crashes.

@trop
Copy link
Contributor

trop bot commented Feb 28, 2023

I was unable to backport this PR to "22-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot removed the target/22-x-y PR should also be added to the "22-x-y" branch. label Feb 28, 2023
@trop
Copy link
Contributor

trop bot commented Feb 28, 2023

I was unable to backport this PR to "23-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot added needs-manual-bp/22-x-y needs-manual-bp/23-x-y and removed target/23-x-y PR should also be added to the "23-x-y" branch. labels Feb 28, 2023
@trop trop bot mentioned this pull request Feb 28, 2023
@trop
Copy link
Contributor

trop bot commented Feb 28, 2023

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

@trop trop bot added in-flight/24-x-y merged/24-x-y PR was merged to the "24-x-y" branch and removed target/24-x-y PR should also be added to the "24-x-y" branch. in-flight/24-x-y labels Feb 28, 2023
@clavin clavin deleted the clavin/about-panel-dict branch March 1, 2023 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/24-x-y PR was merged to the "24-x-y" branch semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants