From f4d164b2e798fc80fdcadd1716aa30fc6e59896b Mon Sep 17 00:00:00 2001 From: Mauro <34335419+Velin92@users.noreply.github.com> Date: Fri, 3 Nov 2023 21:30:58 +0100 Subject: [PATCH] Fix NSE Leak (#2022) --- ElementX.xcodeproj/project.pbxproj | 2 +- .../xcshareddata/swiftpm/Package.resolved | 4 +- .../Mocks/Generated/GeneratedMocks.swift | 2 +- .../Mocks/Generated/SDKGeneratedMocks.swift | 11 +++-- .../Sources/Services/Client/ClientProxy.swift | 17 +++++-- .../ElementCall/ElementCallWidgetDriver.swift | 3 +- .../Media/Provider/MediaFileHandleProxy.swift | 6 ++- .../NotificationServiceExtension.swift | 44 +++++++++++++++---- NSE/Sources/Other/NSEUserSession.swift | 7 ++- changelog.d/1923.bugfix | 1 + project.yml | 2 +- 11 files changed, 77 insertions(+), 22 deletions(-) create mode 100644 changelog.d/1923.bugfix diff --git a/ElementX.xcodeproj/project.pbxproj b/ElementX.xcodeproj/project.pbxproj index c0ca1e59a..952e3c76c 100644 --- 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" */ = { diff --git a/ElementX.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved b/ElementX.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved index 86b56a8e4..aaeb4e22a 100644 --- 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" } }, { diff --git a/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift b/ElementX/Sources/Mocks/Generated/GeneratedMocks.swift index 1a49de306..2f17a11a3 100644 --- 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 diff --git a/ElementX/Sources/Mocks/Generated/SDKGeneratedMocks.swift b/ElementX/Sources/Mocks/Generated/SDKGeneratedMocks.swift index 2a3aebbd6..d9dcafe3d 100644 --- 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 diff --git a/ElementX/Sources/Services/Client/ClientProxy.swift b/ElementX/Sources/Services/Client/ClientProxy.swift index 6eca06902..64524e98f 100644 --- 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() @@ -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)") diff --git a/ElementX/Sources/Services/ElementCall/ElementCallWidgetDriver.swift b/ElementX/Sources/Services/ElementCall/ElementCallWidgetDriver.swift index 60cbf9b21..9fa463f69 100644 --- 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) } diff --git a/ElementX/Sources/Services/Media/Provider/MediaFileHandleProxy.swift b/ElementX/Sources/Services/Media/Provider/MediaFileHandleProxy.swift index 592641370..cf7f798ee 100644 --- 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() } diff --git a/NSE/Sources/NotificationServiceExtension.swift b/NSE/Sources/NotificationServiceExtension.swift index d6f3a8a61..92ef306fe 100644 --- 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") diff --git a/NSE/Sources/Other/NSEUserSession.swift b/NSE/Sources/Other/NSEUserSession.swift index 77161df90..d5d935562 100644 --- 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 { diff --git a/changelog.d/1923.bugfix b/changelog.d/1923.bugfix new file mode 100644 index 000000000..c58bd172e --- /dev/null +++ b/changelog.d/1923.bugfix @@ -0,0 +1 @@ +Fixed a memory leak that made the NSE crash. \ No newline at end of file diff --git a/project.yml b/project.yml index 851323eb2..43c619834 100644 --- 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