fix(apple): Correctly handle stopTunnel and completionHandlers - #9308
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Tested on macOS / iOS. |
There was a problem hiding this comment.
Pull Request Overview
This PR ensures that handleAppMessage and its completionHandler invocations always run on the MainActor, preventing potential pointer invalidation across different executor contexts.
- Mark
handleAppMessageas@MainActor - Wrap
completionHandlercalls inMainActor.runto enforce same actor context
Comments suppressed due to low confidence (2)
swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift:178
- [nitpick] Redundant actor hop: since
handleAppMessageis already@MainActorandTaskinherits that context, you can invokecompletionHandler?(nil)directly withoutMainActor.run.
await MainActor.run { completionHandler?(nil) }
swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift:249
- [nitpick] Similarly,
completionHandler?(data)can be called directly within the@MainActorcontext instead of wrapping it inMainActor.run.
await MainActor.run { completionHandler?(data) }
| let data = withUnsafeBytes(of: size) { Data($0) } | ||
|
|
||
| completionHandler?(data) | ||
| // Ensure completionHandler is called on the same queue as handleAppMessage |
There was a problem hiding this comment.
[nitpick] The comment refers to a "queue", but this change is about actor isolation. Consider updating it to mention the MainActor context for clarity.
|
Sentry Issue: APPLE-CLIENT-5J |
|
I'm not sure this is the actual cause of the issue, but it is one issue. |
|
The other (minor) issue is that Task closure could return after the PacketTunnelProvider instance had deinit'd, which would cause something like this. |
jamilbk
changed the title
|
Sentry Issue: APPLE-CLIENT-5F |
|
|
||
| func signOut() async throws { | ||
| try await sendMessageWithoutResponse(ProviderMessage.signOut) | ||
| try stop() |
There was a problem hiding this comment.
When explicitly requested by the user, we call stopTunnel from this side of the IPC, which is what Apple says to do.
2d472d2 to
13b619f
Compare
| {/* When you cut a release, remove any solved issues from the "known issues" lists over in `client-apps`. This must not be done when the issue's PR merges. */} | ||
| <Unreleased> | ||
| <ChangeItem pull="9308"> | ||
| Fixes a minor crash in the network extension that could occur when |
There was a problem hiding this comment.
I don't think there is such a thing as a minor crash. Maybe change that to rare?
There was a problem hiding this comment.
Yeah I can update it. The crash only reliably occurred during sign out, so no user impact.
|
Sentry Issue: APPLE-CLIENT-5G |
This PR fixes two crashes related to lifetimes on Apple:
completionHandlerwas being called from within a Task executor context, which could be different from the one the IPC call was received ongetLogFolderSizetask could return and attempt to callcompletionHandlerafter the PacketTunnelProvider deinit'dstopTunnelmanually. Apple explicitly says not to do this. Instead, we must callcancelTunnelWithError(nil)when we want to stop the tunnel from e.g. theonDisconnect. Apple with then call ourstopTunneloverride. The downside is that we have no control over theNEProviderStopReasonreceived in this callback, but we don't use it anyway. Instead, we write the reason to a temporary file and read it from the GUI process when we detect a status change todisconnected. When that occurs, we're able to show a UI notification (macOS only - iOS can show this notification from the PacketTunnelProvider itself).