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 crash in dispatch queue assertion with background tasks #7510

Merged

Conversation

buggmagnet
Copy link
Contributor

@buggmagnet buggmagnet commented Jan 23, 2025

This PR should fix the crash we've been encountering in the latest beta version of the app.
It does so by forcing the registered tasks completion handlers to run on the main thread of the application.
It also removes some of the nonisolated(unsafe) markers that were added in those completion handlers.


This change is Reviewable

@buggmagnet buggmagnet added the iOS Issues related to iOS label Jan 23, 2025
@buggmagnet buggmagnet self-assigned this Jan 23, 2025
Copy link

linear bot commented Jan 23, 2025

@buggmagnet buggmagnet force-pushed the investigate-crash-in-test-flight-for-mullvadvpn-app-ios-1033 branch from 14eefef to 29e3d4b Compare January 23, 2025 15:18
Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 7 files at r1.
Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @buggmagnet)


ios/MullvadVPN/TunnelManager/TunnelManager.swift line 228 at r1 (raw file):

            completionHandler: { [weak self] result in
                guard let self else { return }
                DispatchQueue.main.async {

I know these are my changes, but what ensures that this completion handler will be executed on the main actor here?


ios/MullvadVPN/TunnelManager/TunnelManager.swift line 503 at r1 (raw file):

            }

            completionHandler(error)

Should this really be removed?

Copy link
Contributor Author

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 7 files reviewed, 2 unresolved discussions (waiting on @pinkisemils)


ios/MullvadVPN/TunnelManager/TunnelManager.swift line 228 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

I know these are my changes, but what ensures that this completion handler will be executed on the main actor here?

It doesn't need to be, also I think we don't use it anywhere AFAiCT 🤔


ios/MullvadVPN/TunnelManager/TunnelManager.swift line 503 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Should this really be removed?

It should not, thanks for catching this !

@buggmagnet buggmagnet force-pushed the investigate-crash-in-test-flight-for-mullvadvpn-app-ios-1033 branch from d4b9c20 to 9f2382b Compare January 23, 2025 16:01
Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @buggmagnet)


ios/Operations/ResultOperation.swift line 134 at r3 (raw file):

    }

    private func dispatchAsyncOn(_ queue: DispatchQueue?, _ block: @escaping @Sendable () -> Void) {

Does this block have to be Sendable? It could just be sending IMO.
Isn't it the case that anything that is Sendable can be trivially sent without causing any extra headaches for the caller?

Copy link
Contributor Author

@buggmagnet buggmagnet left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 7 files reviewed, 1 unresolved discussion (waiting on @pinkisemils)


ios/Operations/ResultOperation.swift line 134 at r3 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Does this block have to be Sendable? It could just be sending IMO.
Isn't it the case that anything that is Sendable can be trivially sent without causing any extra headaches for the caller?

yes, DispatchQueue.async requires the block to be @Sendable

Copy link
Collaborator

@pinkisemils pinkisemils left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@SteffenErn SteffenErn left a comment

Choose a reason for hiding this comment

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

Doesn't crash anymore on iOS 18.1.1

Reviewed 6 of 7 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@pinkisemils pinkisemils force-pushed the investigate-crash-in-test-flight-for-mullvadvpn-app-ios-1033 branch from 1b29e0a to f027814 Compare January 24, 2025 10:56
@pinkisemils pinkisemils merged commit 6753dba into main Jan 24, 2025
11 checks passed
@pinkisemils pinkisemils deleted the investigate-crash-in-test-flight-for-mullvadvpn-app-ios-1033 branch January 24, 2025 11:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iOS Issues related to iOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants