Fix NSE Leak (#2022)

2023-11-03 21:30:58 +01:00
parent 07d342482e
commit f4d164b2e7
11 changed files with 77 additions and 22 deletions
--- a/ElementX.xcodeproj/project.pbxproj
+++ b/ElementX.xcodeproj/project.pbxproj
@@ -6592,7 +6592,7 @@
 			repositoryURL = "https://github.com/matrix-org/matrix-rust-components-swift";
 			requirement = {
 				kind = exactVersion;
-				version = "0.0.2-october23";
+				version = "0.0.1-november23";
 			};
 		};
 		821C67C9A7F8CC3FD41B28B4 /* XCRemoteSwiftPackageReference "emojibase-bindings" */ = {
--- a/ElementX.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
+++ b/ElementX.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
@@ -130,8 +130,8 @@
      "kind" : "remoteSourceControl",
      "location" : "https://github.com/matrix-org/matrix-rust-components-swift",
      "state" : {
-        "revision" : "48f7062bd94debe766820a28c77f40a6e51900c3",
-        "version" : "0.0.2-october23"
+        "revision" : "29870facdcf257e9cd79ee0eacd52b7425b92736",
+        "version" : "0.0.1-november23"
      }
    },
    {
--- a/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift
+++ b/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift
@@ -1,4 +1,4 @@
-// Generated using Sourcery 2.1.2 — https://github.com/krzysztofzablocki/Sourcery
+// Generated using Sourcery 2.1.1 — https://github.com/krzysztofzablocki/Sourcery
 // DO NOT EDIT

 // swiftlint:disable all
--- a/ElementX/Sources/Mocks/Generated/SDKGeneratedMocks.swift
+++ b/ElementX/Sources/Mocks/Generated/SDKGeneratedMocks.swift
@@ -575,13 +575,18 @@ class SDKClientMock: SDKClientProtocol {
    }
    public var setDelegateDelegateReceivedDelegate: ClientDelegate?
    public var setDelegateDelegateReceivedInvocations: [ClientDelegate?] = []
-    public var setDelegateDelegateClosure: ((ClientDelegate?) -> Void)?
+    public var setDelegateDelegateReturnValue: TaskHandle?
+    public var setDelegateDelegateClosure: ((ClientDelegate?) -> TaskHandle?)?

-    public func setDelegate(delegate: ClientDelegate?) {
+    public func setDelegate(delegate: ClientDelegate?) -> TaskHandle? {
        setDelegateDelegateCallsCount += 1
        setDelegateDelegateReceivedDelegate = delegate
        setDelegateDelegateReceivedInvocations.append(delegate)
-        setDelegateDelegateClosure?(delegate)
+        if let setDelegateDelegateClosure = setDelegateDelegateClosure {
+            return setDelegateDelegateClosure(delegate)
+        } else {
+            return setDelegateDelegateReturnValue
+        }
    }
    //MARK: - setDisplayName

--- a/ElementX/Sources/Services/Client/ClientProxy.swift
+++ b/ElementX/Sources/Services/Client/ClientProxy.swift
@@ -36,6 +36,8 @@ class ClientProxy: ClientProxyProtocol {
    private var syncService: SyncService?
    private var syncServiceStateUpdateTaskHandle: TaskHandle?
    
+    private var delegateHandle: TaskHandle?
+    
    // These following summary providers both operate on the same allRooms() list but
    // can apply their own filtering and pagination
    private(set) var roomSummaryProvider: RoomSummaryProviderProtocol?
@@ -69,8 +71,10 @@ class ClientProxy: ClientProxyProtocol {
    private var hasEncounteredAuthError = false
    
    deinit {
-        client.setDelegate(delegate: nil)
-        stopSync()
+        stopSync { [delegateHandle] in
+            // The delegate handle needs to be cancelled always after the sync stops
+            delegateHandle?.cancel()
+        }
    }
    
    let callbacks = PassthroughSubject<ClientProxyCallback, Never>()
@@ -98,7 +102,7 @@ class ClientProxy: ClientProxyProtocol {
        
        secureBackupController = SecureBackupController(encryption: client.encryption())

-        client.setDelegate(delegate: ClientDelegateWrapper { [weak self] isSoftLogout in
+        delegateHandle = client.setDelegate(delegate: ClientDelegateWrapper { [weak self] isSoftLogout in
            self?.hasEncounteredAuthError = true
            self?.callbacks.send(.receivedAuthError(isSoftLogout: isSoftLogout))
        })
@@ -168,10 +172,17 @@ class ClientProxy: ClientProxyProtocol {
    }
    
    func stopSync() {
+        stopSync(completion: nil)
+    }
+    
+    private func stopSync(completion: (() -> Void)?) {
        MXLog.info("Stopping sync")
        
        Task {
            do {
+                defer {
+                    completion?()
+                }
                try await syncService?.stop()
            } catch {
                MXLog.error("Failed stopping the sync service with error: \(error)")
--- a/ElementX/Sources/Services/ElementCall/ElementCallWidgetDriver.swift
+++ b/ElementX/Sources/Services/ElementCall/ElementCallWidgetDriver.swift
@@ -67,7 +67,8 @@ class ElementCallWidgetDriver: WidgetCapabilitiesProvider, ElementCallWidgetDriv
                                                                                 skipLobby: true,
                                                                                 confineToRoom: true,
                                                                                 font: nil,
-                                                                                 analyticsId: nil)) else {
+                                                                                 analyticsId: nil,
+                                                                                 encryption: .unencrypted)) else {
            return .failure(.failedBuildingWidgetSettings)
        }
        
--- a/ElementX/Sources/Services/Media/Provider/MediaFileHandleProxy.swift
+++ b/ElementX/Sources/Services/Media/Provider/MediaFileHandleProxy.swift
@@ -59,9 +59,13 @@ extension MediaFileHandleProxy: Hashable {
 /// An unmanaged file handle that can be created direct from a URL.
 ///
 /// This type allows for mocking but doesn't provide the automatic clean-up mechanism provided by the SDK.
-private struct UnmanagedMediaFileHandle: MediaFileHandleProtocol {
+private class UnmanagedMediaFileHandle: MediaFileHandleProtocol {
    let url: URL
    
+    init(url: URL) {
+        self.url = url
+    }
+    
    func path() -> String {
        url.path()
    }
--- a/NSE/Sources/NotificationServiceExtension.swift
+++ b/NSE/Sources/NotificationServiceExtension.swift
@@ -18,15 +18,38 @@ import Intents
 import MatrixRustSDK
 import UserNotifications

+// The lifecycle of the NSE looks something like the following:
+//  1)  App receives notification
+//  2)  System creates an instance of the extension class
+//      and calls `didReceive` in the background
+//  3)  Extension processes messages / displays whatever
+//      notifications it needs to
+//  4)  Extension notifies its work is complete by calling
+//      the contentHandler
+//  5)  If the extension takes too long to perform its work
+//      (more than 30s), it will be notified and immediately
+//      terminated
+//
+// Note that the NSE does *not* always spawn a new process to
+// handle a new notification and will also try and process notifications
+// in parallel. `didReceive` could be called twice for the same process,
+// but it will always be called on different threads. It may or may not be
+// called on the same instance of `NotificationService` as a previous
+// notification.
+//
+// We keep a global `environment` singleton to ensure that our app context,
+// database, logging, etc. are only ever setup once per *process*
+
+private let settings = NSESettings()
+private let notificationContentBuilder = NotificationContentBuilder(messageEventStringBuilder: RoomMessageEventStringBuilder(attributedStringBuilder: AttributedStringBuilder(permalinkBaseURL: .homeDirectory,
+                                                                                                                                                                              mentionBuilder: PlainMentionBuilder())))
+private let keychainController = KeychainController(service: .sessions,
+                                                    accessGroup: InfoPlistReader.main.keychainAccessGroupIdentifier)
+private var userSessions = [String: NSEUserSession]()
+
 class NotificationServiceExtension: UNNotificationServiceExtension {
-    private let settings = NSESettings()
-    private let notificationContentBuilder = NotificationContentBuilder(messageEventStringBuilder: RoomMessageEventStringBuilder(attributedStringBuilder: AttributedStringBuilder(permalinkBaseURL: .homeDirectory,
-                                                                                                                                                                                  mentionBuilder: PlainMentionBuilder())))
-    private lazy var keychainController = KeychainController(service: .sessions,
-                                                             accessGroup: InfoPlistReader.main.keychainAccessGroupIdentifier)
    private var handler: ((UNNotificationContent) -> Void)?
    private var modifiedContent: UNMutableNotificationContent?
-    private var userSession: NSEUserSession?

    override func didReceive(_ request: UNNotificationRequest,
                             withContentHandler contentHandler: @escaping (UNNotificationContent) -> Void) {
@@ -75,8 +98,13 @@ class NotificationServiceExtension: UNNotificationServiceExtension {
        MXLog.info("\(tag) run with roomId: \(roomId), eventId: \(eventId)")

        do {
-            let userSession = try NSEUserSession(credentials: credentials, clientSessionDelegate: keychainController)
-            self.userSession = userSession
+            let userSession: NSEUserSession
+            if let existingSession = userSessions[credentials.userID] {
+                userSession = existingSession
+            } else {
+                userSession = try NSEUserSession(credentials: credentials, clientSessionDelegate: keychainController)
+                userSessions[credentials.userID] = userSession
+            }
            
            guard let itemProxy = await userSession.notificationItemProxy(roomID: roomId, eventID: eventId) else {
                MXLog.info("\(tag) no notification for the event, discard")
--- a/NSE/Sources/Other/NSEUserSession.swift
+++ b/NSE/Sources/Other/NSEUserSession.swift
@@ -24,6 +24,7 @@ final class NSEUserSession {
    private(set) lazy var mediaProvider: MediaProviderProtocol = MediaProvider(mediaLoader: MediaLoader(client: baseClient),
                                                                               imageCache: .onlyOnDisk,
                                                                               backgroundTaskService: nil)
+    private let delegateHandle: TaskHandle?

    init(credentials: KeychainCredentials, clientSessionDelegate: ClientSessionDelegate) throws {
        userID = credentials.userID
@@ -35,7 +36,7 @@ final class NSEUserSession {
                                           sessionDelegate: clientSessionDelegate)
            .build()
        
-        baseClient.setDelegate(delegate: ClientDelegateWrapper())
+        delegateHandle = baseClient.setDelegate(delegate: ClientDelegateWrapper())
        try baseClient.restoreSession(session: credentials.restorationToken.session)
        
        notificationClient = try baseClient
@@ -62,6 +63,10 @@ final class NSEUserSession {
            }
        }
    }
+    
+    deinit {
+        delegateHandle?.cancel()
+    }
 }

 private class ClientDelegateWrapper: ClientDelegate {
--- a/changelog.d/1923.bugfix
+++ b/changelog.d/1923.bugfix
@@ -0,0 +1 @@
+Fixed a memory leak that made the NSE crash.
--- a/project.yml
+++ b/project.yml
@@ -45,7 +45,7 @@ packages:
  # Element/Matrix dependencies
  MatrixRustSDK:
    url: https://github.com/matrix-org/matrix-rust-components-swift
-    exactVersion: 0.0.2-october23
+    exactVersion: 0.0.1-november23
    # path: ../matrix-rust-sdk
  Compound:
    url: https://github.com/vector-im/compound-ios
				`@@ -0,0 +1 @@`
				`Fixed a memory leak that made the NSE crash.`