Merge pull request #177 from loki-project/protocol-bug-fixes

Various Session Protocol Bug Fixes

Signal/src/Loki/Components/ConversationTitleView.swift

@@ -3,6 +3,7 @@
 final class ConversationTitleView : UIView {
     private let thread: TSThread
     private var currentStatus: Status? { didSet { updateSubtitleForCurrentStatus() } }
+    private var handledMessageTimestamps: Set<NSNumber> = []
     
     // MARK: Types
     private enum Status : Int {
@@ -112,6 +113,7 @@ final class ConversationTitleView : UIView {
     @objc private func handleMessageSentNotification(_ notification: Notification) {
         guard let timestamp = notification.object as? NSNumber else { return }
         setStatusIfNeeded(to: .messageSent, forMessageWithTimestamp: timestamp)
+        handledMessageTimestamps.insert(timestamp)
         DispatchQueue.main.asyncAfter(deadline: .now() + 1) {
             self.clearStatusIfNeededForMessageWithTimestamp(timestamp)
         }
@@ -123,12 +125,21 @@ final class ConversationTitleView : UIView {
     }
     
     private func setStatusIfNeeded(to status: Status, forMessageWithTimestamp timestamp: NSNumber) {
+        guard !handledMessageTimestamps.contains(timestamp) else { return }
         var uncheckedTargetInteraction: TSInteraction? = nil
         thread.enumerateInteractions { interaction in
             guard interaction.timestamp == timestamp.uint64Value else { return }
             uncheckedTargetInteraction = interaction
         }
-        guard let targetInteraction = uncheckedTargetInteraction, targetInteraction.interactionType() == .outgoingMessage, status.rawValue > (currentStatus?.rawValue ?? 0) else { return }
+        guard let targetInteraction = uncheckedTargetInteraction, targetInteraction.interactionType() == .outgoingMessage,
+            status.rawValue > (currentStatus?.rawValue ?? 0), let hexEncodedPublicKey = targetInteraction.thread.contactIdentifier() else { return }
+        var masterHexEncodedPublicKey: String!
+        let storage = OWSPrimaryStorage.shared()
+        storage.dbReadConnection.read { transaction in
+            masterHexEncodedPublicKey = storage.getMasterHexEncodedPublicKey(for: hexEncodedPublicKey, in: transaction) ?? hexEncodedPublicKey
+        }
+        let isSlaveDevice = masterHexEncodedPublicKey != hexEncodedPublicKey
+        guard !isSlaveDevice else { return }
         currentStatus = status
     }
     

Signal/src/ViewControllers/ConversationView/ConversationViewController.m

@@ -220,6 +220,10 @@ typedef enum : NSUInteger {
 @property (nonatomic) NSMutableArray<LKMention *> *mentions;
 @property (nonatomic) NSString *oldText;
 
+// Status bar updating
+/// Used to avoid duplicate status bar updates.
+@property (nonatomic) NSMutableSet<NSNumber *> *handledMessageTimestamps;
+
 @end
 
 #pragma mark -
@@ -1237,9 +1241,11 @@ typedef enum : NSUInteger {
 }
 
 - (void)restoreSession {
-    [OWSPrimaryStorage.sharedManager.dbReadWriteConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) {
+    dispatch_async(dispatch_get_main_queue(), ^{
-        [LKSessionManagementProtocol sending_startSessionResetInThread:self.thread using:transaction];
+        [OWSPrimaryStorage.sharedManager.dbReadWriteConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) {
-    }];
+            [LKSessionManagementProtocol sending_startSessionResetInThread:self.thread using:transaction];
+        }];
+    });
 }
 
 - (void)noLongerVerifiedBannerViewWasTapped:(UIGestureRecognizer *)sender
@@ -5394,6 +5400,7 @@ typedef enum : NSUInteger {
 {
     NSNumber *timestamp = (NSNumber *)notification.object;
     [self setProgressIfNeededTo:1.0f forMessageWithTimestamp:timestamp];
+    [self.handledMessageTimestamps addObject:timestamp];
     dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(1 * NSEC_PER_SEC)), dispatch_get_main_queue(), ^(void) {
         [self hideProgressIndicatorViewForMessageWithTimestamp:timestamp];
     });
@@ -5407,6 +5414,11 @@ typedef enum : NSUInteger {
 
 - (void)setProgressIfNeededTo:(float)progress forMessageWithTimestamp:(NSNumber *)timestamp
 {
+    if ([self.handledMessageTimestamps contains:^BOOL(NSNumber *t) {
+        return [t isEqual:timestamp];
+    }]) {
+        return;
+    }
     __block TSInteraction *targetInteraction;
     [self.thread enumerateInteractionsUsingBlock:^(TSInteraction *interaction) {
         if (interaction.timestamp == timestamp.unsignedLongLongValue) {
@@ -5414,6 +5426,14 @@ typedef enum : NSUInteger {
         }
     }];
     if (targetInteraction == nil || targetInteraction.interactionType != OWSInteractionType_OutgoingMessage) { return; }
+    NSString *hexEncodedPublicKey = targetInteraction.thread.contactIdentifier;
+    if (hexEncodedPublicKey == nil) { return; }
+    __block NSString *masterHexEncodedPublicKey;
+    [OWSPrimaryStorage.sharedManager.dbReadConnection readWithBlock:^(YapDatabaseReadTransaction *transaction) {
+        masterHexEncodedPublicKey = [LKDatabaseUtilities getMasterHexEncodedPublicKeyFor:hexEncodedPublicKey in:transaction] ?: hexEncodedPublicKey;
+    }];
+    BOOL isSlaveDevice = ![masterHexEncodedPublicKey isEqual:hexEncodedPublicKey];
+    if (isSlaveDevice) { return; }
     dispatch_async(dispatch_get_main_queue(), ^{
         if (progress <= self.progressIndicatorView.progress) { return; }
         self.progressIndicatorView.alpha = 1;

SignalServiceKit/src/Loki/API/LokiAPI.swift

@@ -2,7 +2,7 @@ import PromiseKit
 
 // TODO: A lot of the API relies on things happening serially and state being maintained correctly (i.e. without
 // race conditions). To this end we should just have one high quality serial queue and do everything on there, except
-// for things that explicitly *can* be done in parallel and don't modify state, any which should then happen
+// for things that explicitly *can* be done in parallel and don't modify state, which should then happen
 // on a global queue.
 
 @objc(LKAPI)

SignalServiceKit/src/Loki/API/LokiDotNetAPI.swift

@@ -40,6 +40,7 @@ public class LokiDotNetAPI : NSObject {
         } else {
             return requestNewAuthToken(for: server).then(on: LokiAPI.workQueue) { submitAuthToken($0, for: server) }.then(on: LokiAPI.workQueue) { token -> Promise<String> in
                 let (promise, seal) = Promise<String>.pending()
+                // Dispatch async on the main queue to avoid nested write transactions
                 DispatchQueue.main.async {
                     storage.dbReadWriteConnection.readWrite { transaction in
                         setAuthToken(for: server, to: token, in: transaction)

SignalServiceKit/src/Loki/API/LokiFileServerAPI.swift

@@ -86,6 +86,7 @@ public final class LokiFileServerAPI : LokiDotNetAPI {
                 })
             }.then(on: LokiAPI.workQueue) { deviceLinks -> Promise<Set<DeviceLink>> in
                 let (promise, seal) = Promise<Set<DeviceLink>>.pending()
+                // Dispatch async on the main queue to avoid nested write transactions
                 DispatchQueue.main.async {
                     storage.dbReadWriteConnection.readWrite { transaction in
                         storage.setDeviceLinks(deviceLinks, in: transaction)
@@ -126,6 +127,7 @@ public final class LokiFileServerAPI : LokiDotNetAPI {
         deviceLinks.insert(deviceLink)
         return setDeviceLinks(deviceLinks).then(on: LokiAPI.workQueue) { _ -> Promise<Void> in
             let (promise, seal) = Promise<Void>.pending()
+            // Dispatch async on the main queue to avoid nested write transactions
             DispatchQueue.main.async {
                 storage.dbReadWriteConnection.readWrite { transaction in
                     storage.addDeviceLink(deviceLink, in: transaction)
@@ -145,6 +147,7 @@ public final class LokiFileServerAPI : LokiDotNetAPI {
         deviceLinks.remove(deviceLink)
         return setDeviceLinks(deviceLinks).then(on: LokiAPI.workQueue) { _ -> Promise<Void> in
             let (promise, seal) = Promise<Void>.pending()
+            // Dispatch async on the main queue to avoid nested write transactions
             DispatchQueue.main.async {
                 storage.dbReadWriteConnection.readWrite { transaction in
                     storage.removeDeviceLink(deviceLink, in: transaction)

SignalServiceKit/src/Loki/Protocol/Mentions/MentionsManager.swift

@@ -9,7 +9,7 @@ public final class MentionsManager : NSObject {
         set { LokiAPI.stateQueue.sync { _userHexEncodedPublicKeyCache = newValue } }
     }
 
-    // TODO: I don't think this stateQueue stuff actually helps avoid race conditions
+    // TODO: I don't think stateQueue actually helps avoid race conditions
 
     internal static var storage: OWSPrimaryStorage { OWSPrimaryStorage.shared() }
 

SignalServiceKit/src/Loki/Protocol/Message Types/LKSessionRestoreMessage.h

@@ -2,7 +2,7 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
-/// TODO: This is just a friend request message with a flag set. Not sure if it needs to be its own type.
+// TODO: This is just a friend request message with a flag set. Not sure if it needs to be its own type.
 
 NS_SWIFT_NAME(SessionRestoreMessage)
 @interface LKSessionRestoreMessage : LKFriendRequestMessage

SignalServiceKit/src/Loki/Protocol/Message Types/LKUnlinkDeviceMessage.h

@@ -2,7 +2,7 @@
 
 NS_ASSUME_NONNULL_BEGIN
 
-/// TODO: This is just an ephemeral message with a flag set. Not sure if it needs to be its own type.
+// TODO: This is just an ephemeral message with a flag set. Not sure if it needs to be its own type.
 
 NS_SWIFT_NAME(UnlinkDeviceMessage)
 @interface LKUnlinkDeviceMessage : TSOutgoingMessage

SignalServiceKit/src/Loki/Protocol/Multi Device/MultiDeviceProtocol.swift

@@ -19,7 +19,7 @@ public final class MultiDeviceProtocol : NSObject {
         set { LokiAPI.stateQueue.sync { _lastDeviceLinkUpdate = newValue } }
     }
 
-    // TODO: I don't think this stateQueue stuff actually helps avoid race conditions
+    // TODO: I don't think stateQueue actually helps avoid race conditions
 
     internal static var storage: OWSPrimaryStorage { OWSPrimaryStorage.shared() }
 
@@ -46,7 +46,7 @@ public final class MultiDeviceProtocol : NSObject {
         storage.dbReadConnection.read { transaction in
             recipient = SignalRecipient.getOrBuildUnsavedRecipient(forRecipientId: destination.hexEncodedPublicKey, transaction: transaction)
         }
-        // TODO: Apparently it's okay that the thread, sender certificate, etc. don't get changed?
+        // TODO: Why is it okay that the thread, sender certificate, etc. don't get changed?
         return OWSMessageSend(message: messageSend.message, thread: messageSend.thread, recipient: recipient,
             senderCertificate: messageSend.senderCertificate, udAccess: messageSend.udAccess, localNumber: messageSend.localNumber, success: {
             seal.fulfill(messageSend.message.thread as! TSContactThread)
@@ -176,10 +176,11 @@ public final class MultiDeviceProtocol : NSObject {
         let udManager = SSKEnvironment.shared.udManager
         let senderCertificate = udManager.getSenderCertificate()
         let (promise, seal) = Promise<OWSMessageSend>.pending()
+        // Dispatch async on the main queue to avoid nested write transactions
         DispatchQueue.main.async {
             var recipientUDAccess: OWSUDAccess?
             if let senderCertificate = senderCertificate {
-                recipientUDAccess = udManager.udAccess(forRecipientId: hexEncodedPublicKey, requireSyncAccess: true)
+                recipientUDAccess = udManager.udAccess(forRecipientId: hexEncodedPublicKey, requireSyncAccess: true) // Starts a new write transaction internally
             }
             let messageSend = OWSMessageSend(message: message, thread: thread, recipient: recipient, senderCertificate: senderCertificate,
                 udAccess: recipientUDAccess, localNumber: getUserHexEncodedPublicKey(), success: {

SignalServiceKit/src/Loki/Protocol/Session Management/SessionManagementProtocol.swift

@@ -80,7 +80,7 @@ public final class SessionManagementProtocol : NSObject {
     }
 
     // MARK: - Sending
-    // TODO: Confusing that we have this but also the receiving version
+    // TODO: Confusing that we have this but also a receiving version
     @objc(sending_startSessionResetInThread:using:)
     public static func sending_startSessionReset(in thread: TSThread, using transaction: YapDatabaseReadWriteTransaction) {
         guard let thread = thread as? TSContactThread else {
@@ -91,8 +91,7 @@ public final class SessionManagementProtocol : NSObject {
         let devices = thread.sessionRestoreDevices // TODO: Rename this
         for device in devices {
             guard device.count != 0 else { continue }
-            let sessionResetMessageSend = getSessionResetMessageSend(for: device, in: transaction)
+            getSessionResetMessageSend(for: device, in: transaction).done(on: OWSDispatch.sendingQueue()) { sessionResetMessageSend in
-            OWSDispatch.sendingQueue().async {
                 messageSender.sendMessage(sessionResetMessageSend)
             }
         }
@@ -112,7 +111,11 @@ public final class SessionManagementProtocol : NSObject {
     }
 
     @objc(getSessionResetMessageSendForHexEncodedPublicKey:in:)
-    public static func getSessionResetMessageSend(for hexEncodedPublicKey: String, in transaction: YapDatabaseReadWriteTransaction) -> OWSMessageSend {
+    public static func objc_getSessionResetMessageSend(for hexEncodedPublicKey: String, in transaction: YapDatabaseReadWriteTransaction) -> AnyPromise {
+        return AnyPromise.from(getSessionResetMessageSend(for: hexEncodedPublicKey, in: transaction))
+    }
+
+    public static func getSessionResetMessageSend(for hexEncodedPublicKey: String, in transaction: YapDatabaseReadWriteTransaction) -> Promise<OWSMessageSend> {
         let thread = TSContactThread.getOrCreateThread(withContactId: hexEncodedPublicKey, transaction: transaction)
         let masterHexEncodedPublicKey = storage.getMasterHexEncodedPublicKey(for: hexEncodedPublicKey, in: transaction)
         let isSlaveDeviceThread = masterHexEncodedPublicKey != hexEncodedPublicKey
@@ -122,16 +125,22 @@ public final class SessionManagementProtocol : NSObject {
         let recipient = SignalRecipient.getOrBuildUnsavedRecipient(forRecipientId: hexEncodedPublicKey, transaction: transaction)
         let udManager = SSKEnvironment.shared.udManager
         let senderCertificate = udManager.getSenderCertificate()
-        var recipientUDAccess: OWSUDAccess?
+        let (promise, seal) = Promise<OWSMessageSend>.pending()
-        if let senderCertificate = senderCertificate {
+        // Dispatch async on the main queue to avoid nested write transactions
-            recipientUDAccess = udManager.udAccess(forRecipientId: hexEncodedPublicKey, requireSyncAccess: true)
+        DispatchQueue.main.async {
-        }
+            var recipientUDAccess: OWSUDAccess?
-        return OWSMessageSend(message: message, thread: thread, recipient: recipient, senderCertificate: senderCertificate,
+            if let senderCertificate = senderCertificate {
-            udAccess: recipientUDAccess, localNumber: getUserHexEncodedPublicKey(), success: {
+                recipientUDAccess = udManager.udAccess(forRecipientId: hexEncodedPublicKey, requireSyncAccess: true) // Starts a new write transaction internally
+            }
+            let messageSend = OWSMessageSend(message: message, thread: thread, recipient: recipient, senderCertificate: senderCertificate,
+                udAccess: recipientUDAccess, localNumber: getUserHexEncodedPublicKey(), success: {
 
-        }, failure: { error in
+            }, failure: { error in
 
-        })
+            })
+            seal.fulfill(messageSend)
+        }
+        return promise
     }
 
     // MARK: - Receiving
@@ -155,14 +164,13 @@ public final class SessionManagementProtocol : NSObject {
         return dataMessage.flags & UInt32(sessionRestoreFlag.rawValue) != 0
     }
 
-    // TODO: Is this only ever used for closed groups?
     @objc(isSessionRequestMessage:)
     public static func isSessionRequestMessage(_ dataMessage: SSKProtoDataMessage) -> Bool {
         let sessionRequestFlag = SSKProtoDataMessage.SSKProtoDataMessageFlags.sessionRequest
         return dataMessage.flags & UInt32(sessionRequestFlag.rawValue) != 0
     }
 
-    // TODO: This seriously needs some explanation of when we expect pre key bundles to be attached
+    // TODO: This needs an explanation of when we expect pre key bundles to be attached
     @objc(handlePreKeyBundleMessageIfNeeded:wrappedIn:using:)
     public static func handlePreKeyBundleMessageIfNeeded(_ protoContent: SSKProtoContent, wrappedIn envelope: SSKProtoEnvelope, using transaction: YapDatabaseReadWriteTransaction) {
         // The envelope source is set during UD decryption
@@ -177,7 +185,7 @@ public final class SessionManagementProtocol : NSObject {
         // If we received a friend request (i.e. also a new pre key bundle), but we were already friends with the other user, reset the session.
         // The envelope type is set during UD decryption.
         if envelope.type == .friendRequest,
-            let thread = TSContactThread.getWithContactId(hexEncodedPublicKey, transaction: transaction), // TODO: Maybe this should be getOrCreate?
+            let thread = TSContactThread.getWithContactId(hexEncodedPublicKey, transaction: transaction), // TODO: Should this be getOrCreate?
             thread.isContactFriend {
             receiving_startSessionReset(in: thread, using: transaction)
             // Notify our other devices that we've started a session reset
@@ -186,7 +194,7 @@ public final class SessionManagementProtocol : NSObject {
         }
     }
 
-    // TODO: Confusing that we have this but also the sending version
+    // TODO: Confusing that we have this but also a sending version
     @objc(receiving_startSessionResetInThread:using:)
     public static func receiving_startSessionReset(in thread: TSContactThread, using transaction: YapDatabaseReadWriteTransaction) {
         let hexEncodedPublicKey = thread.contactIdentifier()

SignalServiceKit/src/Loki/Protocol/SessionProtocol.swift

@@ -48,7 +48,7 @@ public final class SessionProtocol : NSObject {
                 if let openGroup = LokiDatabaseUtilities.getPublicChat(for: thread.uniqueId!, in: transaction) {
                     result = [ openGroup.server ] // Aim the message at the open group server
                 } else {
-                    // TODO: Handle
+                    // TODO: Handle?
                 }
             }
         } else {
@@ -111,7 +111,8 @@ public final class SessionProtocol : NSObject {
         return !isMessageNoteToSelf(thread) && !thread.isGroupThread()
     }
 
-    // TODO: Not sure how these two relate. EDIT: I think the one below is used to block delivery receipts. That means that
+    // TODO: Not sure how these two relate
+    // EDIT: I think the one below is used to block delivery receipts. That means that
     // right now we do send delivery receipts in note to self, but not read receipts. Other than that their behavior should
     // be identical. Should we just not send any kind of receipt in note to self?
 
@@ -139,8 +140,9 @@ public final class SessionProtocol : NSObject {
     // MARK: - Decryption
     @objc(shouldSkipMessageDecryptResult:)
     public static func shouldSkipMessageDecryptResult(_ result: OWSMessageDecryptResult) -> Bool {
-        // Called from OWSMessageReceiver to prevent messages from even being added to the processing queue for some reason
+        // Called from OWSMessageReceiver to prevent messages from even being added to the processing queue
-        return result.source == getUserHexEncodedPublicKey() // NOTE: This doesn't take into account multi device
+        // TODO: Why is this function needed at all?
+        return result.source == getUserHexEncodedPublicKey() // This intentionally doesn't take into account multi device
     }
 
     // MARK: Profile Updating
@@ -167,7 +169,7 @@ public final class SessionProtocol : NSObject {
             return
         }
         let profileManager = SSKEnvironment.shared.profileManager
-        // This dispatches async on the main queue internally, where it starts a new write transaction. Apparently that's an okay thing to do in this case?
+        // This dispatches async on the main queue internally where it starts a new write transaction
         profileManager.setProfileKeyData(profileKey, forRecipientId: hexEncodedPublicKey, avatarURL: profilePictureURL)
     }
 

SignalServiceKit/src/Loki/Protocol/Sync Messages/SyncMessagesProtocol.swift

@@ -44,7 +44,7 @@ public final class SyncMessagesProtocol : NSObject {
             guard thread.isContactFriend && thread.shouldThreadBeVisible && !thread.isForceHidden else { return }
             friends.append(SignalAccount(recipientId: hexEncodedPublicKey))
         }
-        friends.append(SignalAccount(recipientId: getUserHexEncodedPublicKey())) // TODO: We sure about this?
+        friends.append(SignalAccount(recipientId: getUserHexEncodedPublicKey())) // TODO: Are we sure about this?
         let syncManager = SSKEnvironment.shared.syncManager
         let promises = friends.chunked(by: 3).map { friends -> Promise<Void> in // TODO: Does this always fit?
             return Promise(syncManager.syncContacts(for: friends)).map { _ in }