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 OpenUrlExternally potentially crashing applications due to misconfigurations (and log errors instead) #5989

Merged

Conversation

yesseruser
Copy link
Contributor

@yesseruser yesseruser commented Sep 12, 2023

Added a try/catch statement to all overrides of the osu.Framework.Platform.GameHost.OpenUrlExternally method (in Desktop, IOS and Android platforms) for the bodies of the methods (excluding the url valid check).
Fixes ppy/osu#24696

Added a try/catch statement to all overrides of the osu.Framework.Platform.GameHost.OpenUrlExternally method (in Desktop, IOS and Android platforms) for the bodies of the methods (excluding the url valid check).
osu.Framework.Android/AndroidGameHost.cs Outdated Show resolved Hide resolved
osu.Framework.iOS/IOSGameHost.cs Outdated Show resolved Hide resolved
osu.Framework.Android/AndroidGameHost.cs Outdated Show resolved Hide resolved
osu.Framework/Platform/DesktopGameHost.cs Outdated Show resolved Hide resolved
@yesseruser
Copy link
Contributor Author

@bdach All of your suggestions have been resolved.

@bdach bdach changed the title Added a try/catch statement to OpenUrlExternally Fix OpenUrlExternally potentially crashing applications due to misconfigurations (and log errors instead) Sep 14, 2023
Not really worth it to split out the const, for one, and even if it was,
then it _definitely_ should not live in `Logger` of all places.
@bdach
Copy link
Collaborator

bdach commented Sep 14, 2023

The added Logger constant was massively weird and I've reverted it in 9bc79c6. The general advice is: if you don't see things like error messages extracted anywhere already, probably best to not do it.

Other than that, this is probably fine, as far as I can tell. I'm not sure the iOS variant makes sense, because I'm not sure if InvokeOnMainThread() marshals error correctly in a way that can even make the try-catch work. I can see a world wherein it does completely nothing and the try-catch should instead be moved inside the InvokeOnMainThread() callback. But I'll leave testing that to someone with an actual device.

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

see above

@yesseruser
Copy link
Contributor Author

Okay, thank you.

@peppy
Copy link
Sponsor Member

peppy commented Sep 15, 2023

I'm not sure the iOS variant makes sense, because I'm not sure if InvokeOnMainThread()

I don't think it does, but I also don't think it matters. This is mainly for desktop applications where it's been throwing.

@peppy peppy merged commit 52c58b5 into ppy:master Sep 15, 2023
11 of 13 checks passed
@yesseruser yesseruser deleted the #24696-OpenUrlExternally-exception-handling branch September 15, 2023 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenUrlExternally() can crash game on broken configurations
3 participants