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 OpenURL crash on macOS Sequoia #305

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

beetee17
Copy link

Refer to the discussion at #301 for details.

@@ -12,7 +12,7 @@

@available(iOS 14, macOS 11, tvOS 14, watchOS 7, *)
private enum OpenURLKey: DependencyKey {
static let liveValue = OpenURLEffect { url in
static let liveValue = OpenURLEffect { @MainActor url in
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static let liveValue = OpenURLEffect { @MainActor url in
// NB: This must be annotated '@MainActor' to avoid a Swift bug:
// https://github.com/pointfreeco/swift-dependencies/discussions/301
static let liveValue = OpenURLEffect { @MainActor url in

@stephencelis
Copy link
Member

@beetee17 Thanks, and sorry for the delay! This time of year has been busy.

Do you think it's possible to get a unit test in place that demonstrates the problem? Ideally we would have CI catch any regressions if someone were to refactor away that @MainActor annotation. I've at the very least added a suggestion to comment on the inclusion of it.

beetee17 and others added 2 commits January 13, 2025 22:25
Further testing has revealed that the crash stems from the completion handler for `openURL`. This may be a bummer for use-cases that require it to check whether an app was available to handle a URL scheme.

An attempt was made to have the best of both worlds by returning `.systemAction` in the `OpenURLAction` handler, however this causes the completion handler to never return.

Therefore, the only real fix is to always return `.handled`, which makes it equivalent to the watchOS implementation that always yields `true`.
@beetee17
Copy link
Author

beetee17 commented Jan 13, 2025

@stephencelis, sorry for the delay as well. I was finally able to gain access to macOS Sequoia 15.0 on an M1 Mac Mini for testing. I was able to verify the root cause of the crash, however, the fix comes with a drawback.

The following is copy-pasted from the latest commit message, let me know if more details are required:

Further testing has revealed that the crash stems from the completion handler for openURL. This may be a bummer for use-cases that require it to check whether an app was available to handle a URL scheme.

An attempt was made to have the best of both worlds by returning .systemAction in the OpenURLAction handler, however this causes the completion handler to never return.

Therefore, the only real fix is to always return .handled, which makes it equivalent to the watchOS implementation that always yields true.

Overall, it may be cleaner to use the watchOS implementation instead, I only left the implementation as it is to document my initial attempt to work around the crash. Feel free to change it or let me know and I will change it as well.

The unit test to catch regressions is simple, it simply runs the live value and ensures that the function returns without crashing. However, since the live implementation was hidden behind the private enum OpenURLKey, I changed the access modifier to internal for the purpose of testing.

EDIT: I actually am unsure why the earlier @MainActor fix worked for my users, as that still causes a crash when running the test. Regardless, it is invalid and I will also update the app with these most recent findings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants