From b58a3f23cd2d47cba0cfbf23635b8380c93e0c52 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Mon, 27 May 2024 17:47:16 +1000 Subject: [PATCH] Fixed a couple of bugs in the ConversationVC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Fixed a bug where a disappearing voice message wouldn't stop playing • Fixed a bug where the conversation screen was never freed from memory • Fixed an issue with an earlier change where the openGroupManager cache would access itself while modifying itself causing a crash --- LibSession-Util | 2 +- .../ConversationVC+Interaction.swift | 2 +- Session/Conversations/ConversationVC.swift | 8 ++-- .../Conversations/ConversationViewModel.swift | 44 ++++++++++++++----- .../Open Groups/OpenGroupManager.swift | 19 ++++---- .../Sending & Receiving/Pollers/Poller.swift | 2 +- SessionUtilitiesKit/JobRunner/JobRunner.swift | 5 +-- 7 files changed, 54 insertions(+), 28 deletions(-) diff --git a/LibSession-Util b/LibSession-Util index d113e77c7..b66e54b25 160000 --- a/LibSession-Util +++ b/LibSession-Util @@ -1 +1 @@ -Subproject commit d113e77c7df369eaa0fcc5dbeb3e1e249efeade9 +Subproject commit b66e54b25805a3edbf5c09fafa2c486b18766383 diff --git a/Session/Conversations/ConversationVC+Interaction.swift b/Session/Conversations/ConversationVC+Interaction.swift index d150e28da..068b9acb5 100644 --- a/Session/Conversations/ConversationVC+Interaction.swift +++ b/Session/Conversations/ConversationVC+Interaction.swift @@ -34,7 +34,7 @@ extension ConversationVC: openSettingsFromTitleView() } - @objc func openSettingsFromTitleView() { + @objc func openSettingsFromTitleView() { switch self.titleView.currentLabelType { case .userCount: if self.viewModel.threadData.threadVariant == .group || self.viewModel.threadData.threadVariant == .legacyGroup { diff --git a/Session/Conversations/ConversationVC.swift b/Session/Conversations/ConversationVC.swift index 89fff7475..353e85cdc 100644 --- a/Session/Conversations/ConversationVC.swift +++ b/Session/Conversations/ConversationVC.swift @@ -214,7 +214,9 @@ final class ConversationVC: BaseVC, LibSessionRespondingViewController, Conversa messageLabelAccessibilityLabel: "Outdated client banner text", height: 40 ) - let result: InfoBanner = InfoBanner(info: info, dismiss: self.removeOutdatedClientBanner) + let result: InfoBanner = InfoBanner(info: info, dismiss: { [weak self] in + self?.removeOutdatedClientBanner() + }) result.accessibilityLabel = "Outdated client banner" result.isAccessibilityElement = true @@ -1532,8 +1534,8 @@ final class ConversationVC: BaseVC, LibSessionRespondingViewController, Conversa cell.update( with: cellViewModel, mediaCache: mediaCache, - playbackInfo: viewModel.playbackInfo(for: cellViewModel) { updatedInfo, error in - DispatchQueue.main.async { [weak self] in + playbackInfo: viewModel.playbackInfo(for: cellViewModel) { [weak self] updatedInfo, error in + DispatchQueue.main.async { guard error == nil else { let modal: ConfirmationModal = ConfirmationModal( targetView: self?.view, diff --git a/Session/Conversations/ConversationViewModel.swift b/Session/Conversations/ConversationViewModel.swift index c84169093..ac6497d2f 100644 --- a/Session/Conversations/ConversationViewModel.swift +++ b/Session/Conversations/ConversationViewModel.swift @@ -202,7 +202,7 @@ public class ConversationViewModel: OWSAudioPlayerDelegate { deinit { // Stop any audio playing when leaving the screen - audioPlayer.wrappedValue?.stop() + stopAudio() } // MARK: - Thread Data @@ -442,6 +442,16 @@ public class ConversationViewModel: OWSAudioPlayerDelegate { .filter { !$0.cellType.isPostProcessed } // Remove headers and other .sorted { lhs, rhs -> Bool in lhs.timestampMs < rhs.timestampMs } + // If we are currently playing a voice message we should make sure it hasn't been deleted and, if + // it has, we should stop playing the audio (this is mostly to catch the case where a message is + // deleted due to a disappearing messages setting) + if + let audioPlayingInteractionId: Int64 = currentPlayingInteraction.wrappedValue, + !sortedData.contains(where: { $0.id == audioPlayingInteractionId }) + { + self.stopAudio() + } + // We load messages from newest to oldest so having a pageOffset larger than zero means // there are newer pages to load return [ @@ -957,6 +967,12 @@ public class ConversationViewModel: OWSAudioPlayerDelegate { } public func playOrPauseAudio(for viewModel: MessageViewModel) { + /// Ensure the `OWSAudioPlayer` logic is run on the main thread as it calls `MainAppContext.ensureSleepBlocking` + /// must run on the main thread (also there is no guarantee that `AVAudioPlayer` is thread safe so better safe than sorry) + guard Thread.isMainThread else { + return DispatchQueue.main.sync { [weak self] in self?.playOrPauseAudio(for: viewModel) } + } + guard let attachment: Attachment = viewModel.attachments?.first, let originalFilePath: String = attachment.originalFilePath, @@ -1010,6 +1026,12 @@ public class ConversationViewModel: OWSAudioPlayerDelegate { } public func speedUpAudio(for viewModel: MessageViewModel) { + /// Ensure the `OWSAudioPlayer` logic is run on the main thread as it calls `MainAppContext.ensureSleepBlocking` + /// must run on the main thread (also there is no guarantee that `AVAudioPlayer` is thread safe so better safe than sorry) + guard Thread.isMainThread else { + return DispatchQueue.main.sync { [weak self] in self?.speedUpAudio(for: viewModel) } + } + // If we aren't playing the specified item then just start playing it guard viewModel.id == currentPlayingInteraction.wrappedValue else { playOrPauseAudio(for: viewModel) @@ -1027,12 +1049,18 @@ public class ConversationViewModel: OWSAudioPlayerDelegate { } public func stopAudioIfNeeded(for viewModel: MessageViewModel) { - guard viewModel.id == currentPlayingInteraction.wrappedValue else { return } + guard viewModel.id == currentPlayingInteraction.wrappedValue else { return } - audioPlayer.wrappedValue?.stop() + stopAudio() } public func stopAudio() { + /// Ensure the `OWSAudioPlayer` logic is run on the main thread as it calls `MainAppContext.ensureSleepBlocking` + /// must run on the main thread (also there is no guarantee that `AVAudioPlayer` is thread safe so better safe than sorry) + guard Thread.isMainThread else { + return DispatchQueue.main.sync { [weak self] in self?.stopAudio() } + } + audioPlayer.wrappedValue?.stop() currentPlayingInteraction.mutate { $0 = nil } @@ -1088,13 +1116,7 @@ public class ConversationViewModel: OWSAudioPlayerDelegate { updatedPlaybackInfo?.updateCallback(updatedPlaybackInfo, nil) // Clear out the currently playing record - currentPlayingInteraction.mutate { $0 = nil } - audioPlayer.mutate { - // Note: We clear the delegate and explicitly set to nil here as when the OWSAudioPlayer - // gets deallocated it triggers state changes which cause UI bugs when auto-playing - $0?.delegate = nil - $0 = nil - } + stopAudio() // If the next interaction is another voice message then autoplay it guard @@ -1121,7 +1143,7 @@ public class ConversationViewModel: OWSAudioPlayerDelegate { playbackRate: 1 ) - currentPlayingInteraction.mutate { $0 = nil } + stopAudio() playbackInfo.mutate { $0[interactionId] = updatedPlaybackInfo } updatedPlaybackInfo?.updateCallback(updatedPlaybackInfo, AttachmentError.invalidData) } diff --git a/SessionMessagingKit/Open Groups/OpenGroupManager.swift b/SessionMessagingKit/Open Groups/OpenGroupManager.swift index 5f50758d6..333e5b69b 100644 --- a/SessionMessagingKit/Open Groups/OpenGroupManager.swift +++ b/SessionMessagingKit/Open Groups/OpenGroupManager.swift @@ -39,16 +39,17 @@ public final class OpenGroupManager { .defaulting(to: []) // Update the cache state and re-create all of the pollers - dependencies.caches.mutate(cache: .openGroupManager) { cache in + let pollers: [OpenGroupAPI.PollerType] = dependencies.caches.mutate(cache: .openGroupManager) { cache in cache.isPolling = true - servers - .map { server -> OpenGroupAPI.PollerType in cache.getOrCreatePoller(for: server.lowercased()) } - .forEach { poller in - // Now that the pollers have been created actually start them - poller.stop() // Should never be an issue - poller.startIfNeeded(using: dependencies) - } + return servers.map { server -> OpenGroupAPI.PollerType in cache.getOrCreatePoller(for: server.lowercased()) } + } + + // Need to do this outside of the mutate as the poller will attempt to read from the + // 'openGroupManager' cache which is not allowed + pollers.forEach { poller in + poller.stop() // Should never be an issue + poller.startIfNeeded(using: dependencies) } } } @@ -297,6 +298,8 @@ public final class OpenGroupManager { $0.getOrCreatePoller(for: server.lowercased()) } + // Need to do this outside of the mutate as the poller will attempt to read from the + // 'openGroupManager' cache which is not allowed poller.stop() poller.startIfNeeded(using: dependencies) } diff --git a/SessionMessagingKit/Sending & Receiving/Pollers/Poller.swift b/SessionMessagingKit/Sending & Receiving/Pollers/Poller.swift index caec2d94e..aa44e2a94 100644 --- a/SessionMessagingKit/Sending & Receiving/Pollers/Poller.swift +++ b/SessionMessagingKit/Sending & Receiving/Pollers/Poller.swift @@ -76,7 +76,7 @@ public class Poller { // on startup let drainBehaviour: Atomic = Atomic(pollDrainBehaviour) - Threading.pollerQueue.async { [weak self] in + pollerQueue.async { [weak self] in guard self?.isPolling.wrappedValue[publicKey] != true else { return } // Might be a race condition that the setUpPolling finishes too soon, diff --git a/SessionUtilitiesKit/JobRunner/JobRunner.swift b/SessionUtilitiesKit/JobRunner/JobRunner.swift index de04c1da0..d91006046 100644 --- a/SessionUtilitiesKit/JobRunner/JobRunner.swift +++ b/SessionUtilitiesKit/JobRunner/JobRunner.swift @@ -1492,14 +1492,13 @@ public final class JobQueue: Hashable { // If the next job isn't scheduled in the future then just restart the JobRunner immediately let secondsUntilNextJob: TimeInterval = (nextJobTimestamp - dependencies.dateNow.timeIntervalSince1970) - let ceilSecondsUntilNextJob: Int = Int(ceil(Swift.abs(secondsUntilNextJob))) guard secondsUntilNextJob > 0 else { // Only log that the queue is getting restarted if this queue had actually been about to stop if executionType != .concurrent || currentlyRunningJobIds.wrappedValue.isEmpty { let timingString: String = (nextJobTimestamp == 0 ? "that should be in the queue" : - "scheduled \(ceilSecondsUntilNextJob) second\(ceilSecondsUntilNextJob == 1 ? "" : "s") ago" + "scheduled \(.seconds(secondsUntilNextJob), unit: .s) ago" ) SNLog("[JobRunner] Restarting \(queueContext) immediately for job \(timingString)") } @@ -1517,7 +1516,7 @@ public final class JobQueue: Hashable { guard executionType == .concurrent || currentlyRunningJobIds.wrappedValue.isEmpty else { return } // Setup a trigger - SNLog("[JobRunner] Stopping \(queueContext) until next job in \(ceilSecondsUntilNextJob) second\(ceilSecondsUntilNextJob == 1 ? "" : "s")") + SNLog("[JobRunner] Stopping \(queueContext) until next job in \(.seconds(secondsUntilNextJob), unit: .s)") nextTrigger.mutate { trigger in trigger?.invalidate() // Need to invalidate the old trigger to prevent a memory leak trigger = Trigger.create(queue: self, timestamp: nextJobTimestamp, using: dependencies)