fix(apple): Correctly handle stopTunnel and completionHandlers (#9308)

This PR fixes two crashes related to lifetimes on Apple:

- `completionHandler` was being called from within a Task executor
context, which could be different from the one the IPC call was received
on
- The `getLogFolderSize` task could return and attempt to call
`completionHandler` after the PacketTunnelProvider deinit'd
- We were calling the completionHandler from `stopTunnel` manually.
Apple explicitly says not to do this. Instead, we must call
`cancelTunnelWithError(nil)` when we want to stop the tunnel from e.g.
the `onDisconnect`. Apple with then call our `stopTunnel` override. The
downside is that we have no control over the `NEProviderStopReason`
received 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 to `disconnected`. When that occurs, we're able
to show a UI notification (macOS only - iOS can show this notification
from the PacketTunnelProvider itself).

swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/IPCClient.swift

@@ -65,6 +65,7 @@ class IPCClient {
 
   func signOut() async throws {
     try await sendMessageWithoutResponse(ProviderMessage.signOut)
+    try stop()
   }
 
   func stop() throws {

swift/apple/FirezoneKit/Sources/FirezoneKit/Helpers/SharedAccess.swift

@@ -7,17 +7,6 @@
 import Foundation
 
 public struct SharedAccess {
-  public enum Error: Swift.Error {
-    case unableToWriteToFile(URL, Swift.Error)
-
-    var localizedDescription: String {
-      switch self {
-      case .unableToWriteToFile(let url, let error):
-        return "Unable to write to \(url): \(error)"
-      }
-    }
-  }
-
   public static var baseFolderURL: URL {
     guard
       let url = FileManager.default.containerURL(

swift/apple/FirezoneNetworkExtension/Adapter.swift

@@ -425,16 +425,24 @@ extension Adapter: CallbackHandlerDelegate {
     workQueue.async { [weak self] in
       guard let self = self else { return }
 
-      // Set a default stop reason. In the future, we may have more to act upon in
-      // different ways.
-      var reason: NEProviderStopReason = .connectionFailed
-
+      // If auth expired/is invalid, delete stored token and save the reason why so the GUI can act upon it.
       if error.isAuthenticationError() {
-        reason = .authenticationCanceled
+        do {
+          try Token.delete()
+          let reason: NEProviderStopReason = .authenticationCanceled
+          try String(reason.rawValue).write(
+            to: SharedAccess.providerStopReasonURL, atomically: true, encoding: .utf8)
+        } catch {
+          Log.error(error)
+        }
+#if os(iOS)
+        // iOS notifications should be shown from the tunnel process
+        SessionNotification.showSignedOutNotificationiOS()
+#endif
       }
 
-      // Start the process of telling the system to shut us down
-      self.packetTunnelProvider?.stopTunnel(with: reason) {}
+      // Tell the system to shut us down
+      self.packetTunnelProvider?.cancelTunnelWithError(nil)
     }
   }
 

swift/apple/FirezoneNetworkExtension/PacketTunnelProvider.swift

@@ -23,6 +23,8 @@ class PacketTunnelProvider: NEPacketTunnelProvider {
     case idle
   }
 
+  private var getLogFolderSizeTask: Task<Void, Never>?
+
   private var logExportState: LogExportState = .idle
   private var tunnelConfiguration: TunnelConfiguration?
   private let defaults = UserDefaults.standard
@@ -37,6 +39,10 @@ class PacketTunnelProvider: NEPacketTunnelProvider {
     self.tunnelConfiguration = TunnelConfiguration.tryLoad()
   }
 
+  deinit {
+    getLogFolderSizeTask?.cancel()
+  }
+
   override func startTunnel(
     options: [String: NSObject]?,
     completionHandler: @escaping (Error?) -> Void
@@ -120,42 +126,16 @@ class PacketTunnelProvider: NEPacketTunnelProvider {
   ) {
     Log.log("stopTunnel: Reason: \(reason)")
 
-    do {
-      // There's no good way to send data like this from the
-      // Network Extension to the GUI, so save it to a file for the GUI to read upon
-      // either status change or the next launch.
-      try String(reason.rawValue).write(
-        to: SharedAccess.providerStopReasonURL, atomically: true, encoding: .utf8)
-    } catch {
-      Log.error(
-        SharedAccess.Error.unableToWriteToFile(
-          SharedAccess.providerStopReasonURL,
-          error
-        )
-      )
-    }
-
-    if case .authenticationCanceled = reason {
-      // This was triggered from onDisconnect, so try to clear our token
-      do { try Token.delete() } catch { Log.error(error) }
-
-#if os(iOS)
-      // iOS notifications should be shown from the tunnel process
-      SessionNotification.showSignedOutNotificationiOS()
-#endif
-    }
-
     // handles both connlib-initiated and user-initiated stops
     adapter?.stop()
 
-    cancelTunnelWithError(nil)
     super.stopTunnel(with: reason, completionHandler: completionHandler)
-    completionHandler()
   }
 
   // It would be helpful to be able to encapsulate Errors here. To do that
   // we need to update ProviderMessage to encode/decode Result to and from Data.
   // TODO: Move to a more abstract IPC protocol
+  @MainActor
   override func handleAppMessage(_ message: Data, completionHandler: ((Data?) -> Void)? = nil) {
     do {
       let providerMessage = try PropertyListDecoder().decode(ProviderMessage.self, from: message)
@@ -169,16 +149,7 @@ class PacketTunnelProvider: NEPacketTunnelProvider {
         completionHandler?(nil)
 
       case .signOut:
-        do {
-          try Token.delete()
-          Task {
-            await stopTunnel(with: .userInitiated)
-            completionHandler?(nil)
-          }
-        } catch {
-          Log.error(error)
-          completionHandler?(nil)
-        }
+        do { try Token.delete() } catch { Log.error(error) }
       case .getResourceList(let hash):
         guard let adapter = adapter
         else {
@@ -239,11 +210,13 @@ class PacketTunnelProvider: NEPacketTunnelProvider {
       return
     }
 
-    Task {
+    getLogFolderSizeTask = Task {
       let size = await Log.size(of: logFolderURL)
       let data = withUnsafeBytes(of: size) { Data($0) }
 
-      completionHandler?(data)
+      // Ensure completionHandler is called on the same actor as handleAppMessage and isn't cancelled by deinit
+      if getLogFolderSizeTask?.isCancelled ?? true { return }
+      await MainActor.run { completionHandler?(data) }
     }
   }
 

website/src/components/Changelog/Apple.tsx

@@ -25,6 +25,10 @@ export default function Apple() {
     <Entries downloadLinks={downloadLinks} title="macOS / iOS">
       {/* 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
+          viewing the Diagnostic Logs tab in app settings.
+        </ChangeItem>
         <ChangeItem pull="9242">
           Fixes a rare bug that could prevent certain IPv6 DNS upstream
           resolvers from being used if they contained an interface scope