From 74cdfbbae6c5e76748ed6017768d6dccbbeb2802 Mon Sep 17 00:00:00 2001 From: Morgan Pretty Date: Tue, 27 Aug 2024 09:56:20 +1000 Subject: [PATCH] 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