From 74cdfbbae6c5e76748ed6017768d6dccbbeb2802 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 27 Aug 2024 09:56:20 +1000 Subject: [PATCH 1/2] Reworked the SignalAttachmentItem hash function --- .../AttachmentItemCollection.swift | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/SignalUtilitiesKit/Media Viewing & Editing/Attachment Approval/AttachmentItemCollection.swift b/SignalUtilitiesKit/Media Viewing & Editing/Attachment Approval/AttachmentItemCollection.swift index 98188f870..f52bd25f8 100644 --- a/SignalUtilitiesKit/Media Viewing & Editing/Attachment Approval/AttachmentItemCollection.swift +++ b/SignalUtilitiesKit/Media Viewing & Editing/Attachment Approval/AttachmentItemCollection.swift @@ -30,6 +30,7 @@ class SignalAttachmentItem: Hashable { case noThumbnail } + let uniqueIdentifier: UUID let attachment: SignalAttachment // This might be nil if the attachment is not a valid image. @@ -66,7 +67,20 @@ class SignalAttachmentItem: Hashable { // MARK: Hashable func hash(into hasher: inout Hasher) { - attachment.hash(into: &hasher) + /// There was a crash in `AttachmentApprovalViewController` when trying to generate the hash + /// value to store in a dictionary, this crash persisted even after refactoring `DataSource` into Swift and + /// using custom `hash(into:)` functions on everything in order to exclude values which might have + /// been unsafe. + /// + /// Since the crash is still occurring the most likely culprit is now that one of the values used to generate the + /// hash was mutated after the value was stored (as `SignalAttachment` is a class and it was previously + /// used for generating the hash) - in order to avoid this we now generate a `uniqueIdentifier` when + /// initialising this type and use _only_ that for the hash (this `SignalAttachmentItem` is only used for + /// the `AttachmentApprovalViewController` and based on it's usage we shouldn't run into issues + /// with this hash not being deterministic + /// + /// If the crash still occurs it's likely a red herring and there is some other, larger, issue that is causing it + uniqueIdentifier.hash(into: &hasher) } // MARK: Equatable From 375f66f45af6bc6fbe0fa6e4da3b21fc3b5c3003 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 27 Aug 2024 11:48:38 +1000 Subject: [PATCH 2/2] Reworked to just use a unique id rather than hacky hash overrides --- .../Attachments/SignalAttachment.swift | 18 +-------------- SessionUtilitiesKit/Media/DataSource.swift | 15 +----------- .../AttachmentApprovalViewController.swift | 6 ++--- .../AttachmentItemCollection.swift | 23 ++----------------- 4 files changed, 7 insertions(+), 55 deletions(-) diff --git a/SessionMessagingKit/Sending & Receiving/Attachments/SignalAttachment.swift b/SessionMessagingKit/Sending & Receiving/Attachments/SignalAttachment.swift index 35b004501..11224310a 100644 --- a/SessionMessagingKit/Sending & Receiving/Attachments/SignalAttachment.swift +++ b/SessionMessagingKit/Sending & Receiving/Attachments/SignalAttachment.swift @@ -104,7 +104,7 @@ public enum TSImageQuality: UInt { // [SignalAttachment hasError] will be true for non-valid attachments. // // TODO: Perhaps do conversion off the main thread? -public class SignalAttachment: Equatable, Hashable { +public class SignalAttachment: Equatable { // MARK: Properties @@ -1131,20 +1131,4 @@ public class SignalAttachment: Equatable, Hashable { lhs.isVoiceMessage == rhs.isVoiceMessage ) } - - // MARK: - Hashable - - public func hash(into hasher: inout Hasher) { - dataUTI.hash(into: &hasher) - captionText.hash(into: &hasher) - linkPreviewDraft.hash(into: &hasher) - isConvertibleToTextMessage.hash(into: &hasher) - isConvertibleToContactShare.hash(into: &hasher) - isVoiceMessage.hash(into: &hasher) - dataSource.hash(into: &hasher) - cachedImage?.size.width.hash(into: &hasher) - cachedImage?.size.height.hash(into: &hasher) - cachedVideoPreview?.size.width.hash(into: &hasher) - cachedVideoPreview?.size.height.hash(into: &hasher) - } } diff --git a/SessionUtilitiesKit/Media/DataSource.swift b/SessionUtilitiesKit/Media/DataSource.swift index 2db8110e8..d14195f00 100644 --- a/SessionUtilitiesKit/Media/DataSource.swift +++ b/SessionUtilitiesKit/Media/DataSource.swift @@ -4,7 +4,7 @@ import Foundation // MARK: - DataSource -public protocol DataSource: Equatable, Hashable { +public protocol DataSource: Equatable { var data: Data { get } var dataUrl: URL? { get } @@ -140,13 +140,6 @@ public class DataSourceValue: DataSource { try data.write(to: URL(fileURLWithPath: path), options: .atomic) } - public func hash(into hasher: inout Hasher) { - data.hash(into: &hasher) - sourceFilename.hash(into: &hasher) - fileExtension.hash(into: &hasher) - shouldDeleteOnDeinit.hash(into: &hasher) - } - public static func == (lhs: DataSourceValue, rhs: DataSourceValue) -> Bool { return ( lhs.data == rhs.data && @@ -234,12 +227,6 @@ public class DataSourcePath: DataSource { try FileManager.default.copyItem(atPath: filePath, toPath: path) } - public func hash(into hasher: inout Hasher) { - filePath.hash(into: &hasher) - sourceFilename.hash(into: &hasher) - shouldDeleteOnDeinit.hash(into: &hasher) - } - public static func == (lhs: DataSourcePath, rhs: DataSourcePath) -> Bool { return ( lhs.filePath == rhs.filePath && diff --git a/SignalUtilitiesKit/Media Viewing & Editing/Attachment Approval/AttachmentApprovalViewController.swift b/SignalUtilitiesKit/Media Viewing & Editing/Attachment Approval/AttachmentApprovalViewController.swift index 9206e27fa..cbc153883 100644 --- a/SignalUtilitiesKit/Media Viewing & Editing/Attachment Approval/AttachmentApprovalViewController.swift +++ b/SignalUtilitiesKit/Media Viewing & Editing/Attachment Approval/AttachmentApprovalViewController.swift @@ -95,7 +95,7 @@ public class AttachmentApprovalViewController: UIPageViewController, UIPageViewC set { setCurrentItem(newValue, direction: .forward, animated: false) } } - private var cachedPages: [SignalAttachmentItem: AttachmentPrepViewController] = [:] + private var cachedPages: [UUID: AttachmentPrepViewController] = [:] public var shouldHideControls: Bool { guard let pageViewController: AttachmentPrepViewController = pageViewControllers?.first else { @@ -477,7 +477,7 @@ public class AttachmentApprovalViewController: UIPageViewController, UIPageViewC } private func buildPage(item: SignalAttachmentItem) -> AttachmentPrepViewController? { - if let cachedPage = cachedPages[item] { + if let cachedPage = cachedPages[item.uniqueIdentifier] { Log.debug("[AttachmentApprovalViewController] cache hit.") return cachedPage } @@ -485,7 +485,7 @@ public class AttachmentApprovalViewController: UIPageViewController, UIPageViewC Log.debug("[AttachmentApprovalViewController] cache miss.") let viewController = AttachmentPrepViewController(attachmentItem: item, using: dependencies) viewController.prepDelegate = self - cachedPages[item] = viewController + cachedPages[item.uniqueIdentifier] = viewController return viewController } diff --git a/SignalUtilitiesKit/Media Viewing & Editing/Attachment Approval/AttachmentItemCollection.swift b/SignalUtilitiesKit/Media Viewing & Editing/Attachment Approval/AttachmentItemCollection.swift index f52bd25f8..225669dfc 100644 --- a/SignalUtilitiesKit/Media Viewing & Editing/Attachment Approval/AttachmentItemCollection.swift +++ b/SignalUtilitiesKit/Media Viewing & Editing/Attachment Approval/AttachmentItemCollection.swift @@ -24,13 +24,13 @@ class AddMoreRailItem: GalleryRailItem { } } -class SignalAttachmentItem: Hashable { +class SignalAttachmentItem: Equatable { enum SignalAttachmentItemError: Error { case noThumbnail } - let uniqueIdentifier: UUID + let uniqueIdentifier: UUID = UUID() let attachment: SignalAttachment // This might be nil if the attachment is not a valid image. @@ -64,25 +64,6 @@ class SignalAttachmentItem: Hashable { return attachment.staticThumbnail() } - // MARK: Hashable - - func hash(into hasher: inout Hasher) { - /// There was a crash in `AttachmentApprovalViewController` when trying to generate the hash - /// value to store in a dictionary, this crash persisted even after refactoring `DataSource` into Swift and - /// using custom `hash(into:)` functions on everything in order to exclude values which might have - /// been unsafe. - /// - /// Since the crash is still occurring the most likely culprit is now that one of the values used to generate the - /// hash was mutated after the value was stored (as `SignalAttachment` is a class and it was previously - /// used for generating the hash) - in order to avoid this we now generate a `uniqueIdentifier` when - /// initialising this type and use _only_ that for the hash (this `SignalAttachmentItem` is only used for - /// the `AttachmentApprovalViewController` and based on it's usage we shouldn't run into issues - /// with this hash not being deterministic - /// - /// If the crash still occurs it's likely a red herring and there is some other, larger, issue that is causing it - uniqueIdentifier.hash(into: &hasher) - } - // MARK: Equatable static func == (lhs: SignalAttachmentItem, rhs: SignalAttachmentItem) -> Bool {