From bfb03c0db4bdb50951095e98d20d48c0b54780e3 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 21 Sep 2017 11:55:25 -0400 Subject: [PATCH] Fix message processing edge cases. // FREEBIE --- .../src/ViewControllers/DebugUI/DebugUIMisc.m | 29 ++++++++++ Signal/src/environment/NotificationsManager.m | 1 + .../src/Devices/OWSRecordTranscriptJob.m | 2 +- .../src/Messages/Interactions/TSInteraction.m | 2 + .../TSInvalidIdentityKeySendingErrorMessage.m | 3 +- .../src/Messages/OWSBatchMessageProcessor.m | 17 +++++- .../src/Messages/OWSDisappearingMessagesJob.m | 6 +- .../src/Messages/OWSIdentityManager.h | 1 + .../src/Messages/OWSIdentityManager.m | 1 - .../src/Messages/OWSMessageDecrypter.m | 10 ++-- .../src/Messages/OWSMessageManager.m | 12 ++-- .../src/Messages/OWSMessageSender.h | 4 -- .../src/Messages/OWSMessageSender.m | 58 ++++++++++--------- 13 files changed, 98 insertions(+), 48 deletions(-) diff --git a/Signal/src/ViewControllers/DebugUI/DebugUIMisc.m b/Signal/src/ViewControllers/DebugUI/DebugUIMisc.m index ec66c4a93..30284e0b0 100644 --- a/Signal/src/ViewControllers/DebugUI/DebugUIMisc.m +++ b/Signal/src/ViewControllers/DebugUI/DebugUIMisc.m @@ -61,6 +61,13 @@ NS_ASSUME_NONNULL_BEGIN actionBlock:^{ [DebugUIMisc clearHasDismissedOffers]; }]]; + if ([thread isKindOfClass:[TSGroupThread class]]) { + TSGroupThread *groupThread = (TSGroupThread *)thread; + [items addObject:[OWSTableItem itemWithTitle:@"Hallucinate twin group" + actionBlock:^{ + [DebugUIMisc hallucinateTwinGroup:groupThread]; + }]]; + } return [OWSTableSection sectionWithTitle:self.name items:items]; } @@ -115,6 +122,28 @@ NS_ASSUME_NONNULL_BEGIN }]; } +// Creates a new group (by cloning the current group) without informing the, +// other members. This can be used to test "group info requests", etc. ++ (void)hallucinateTwinGroup:(TSGroupThread *)groupThread +{ + __block TSGroupThread *thread; + [[TSStorageManager sharedManager].dbReadWriteConnection + readWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) { + TSGroupModel *groupModel = + [[TSGroupModel alloc] initWithTitle:[groupThread.groupModel.groupName stringByAppendingString:@" Copy"] + memberIds:[groupThread.groupModel.groupMemberIds mutableCopy] + image:groupThread.groupModel.groupImage + groupId:[SecurityUtils generateRandomBytes:16]]; + thread = [TSGroupThread getOrCreateThreadWithGroupModel:groupModel transaction:transaction]; + }]; + OWSAssert(thread); + + dispatch_async(dispatch_get_main_queue(), ^{ + [Environment presentConversationForThread:thread]; + + }); +} + @end NS_ASSUME_NONNULL_END diff --git a/Signal/src/environment/NotificationsManager.m b/Signal/src/environment/NotificationsManager.m index 87f6f6fa1..c321b9d1f 100644 --- a/Signal/src/environment/NotificationsManager.m +++ b/Signal/src/environment/NotificationsManager.m @@ -206,6 +206,7 @@ NSString *const kNotificationsManagerNewMesssageSoundName = @"NewMessage.aifc"; #pragma mark - Signal Messages - (void)notifyUserForErrorMessage:(TSErrorMessage *)message inThread:(TSThread *)thread { + OWSAssert([NSThread isMainThread]); OWSAssert(message); OWSAssert(thread); diff --git a/SignalServiceKit/src/Devices/OWSRecordTranscriptJob.m b/SignalServiceKit/src/Devices/OWSRecordTranscriptJob.m index 129f595b9..821e9efe6 100644 --- a/SignalServiceKit/src/Devices/OWSRecordTranscriptJob.m +++ b/SignalServiceKit/src/Devices/OWSRecordTranscriptJob.m @@ -102,7 +102,7 @@ NS_ASSUME_NONNULL_BEGIN // If there is an attachment + text, render the text here, as Signal-iOS renders two messages. if (attachmentsProcessor.hasSupportedAttachments && transcript.body && ![transcript.body isEqualToString:@""]) { - // render text *after* the attachment + // We want the text to appear after the attachment. uint64_t textMessageTimestamp = transcript.timestamp + 1; TSOutgoingMessage *textMessage = [[TSOutgoingMessage alloc] initWithTimestamp:textMessageTimestamp inThread:thread diff --git a/SignalServiceKit/src/Messages/Interactions/TSInteraction.m b/SignalServiceKit/src/Messages/Interactions/TSInteraction.m index 60b3b8ac1..f7bb4d15a 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSInteraction.m +++ b/SignalServiceKit/src/Messages/Interactions/TSInteraction.m @@ -14,6 +14,8 @@ NS_ASSUME_NONNULL_BEGIN + (instancetype)interactionForTimestamp:(uint64_t)timestamp withTransaction:(YapDatabaseReadWriteTransaction *)transaction { + OWSAssert(timestamp > 0); + __block int counter = 0; __block TSInteraction *interaction; diff --git a/SignalServiceKit/src/Messages/InvalidKeyMessages/TSInvalidIdentityKeySendingErrorMessage.m b/SignalServiceKit/src/Messages/InvalidKeyMessages/TSInvalidIdentityKeySendingErrorMessage.m index 072705a11..64ca55d40 100644 --- a/SignalServiceKit/src/Messages/InvalidKeyMessages/TSInvalidIdentityKeySendingErrorMessage.m +++ b/SignalServiceKit/src/Messages/InvalidKeyMessages/TSInvalidIdentityKeySendingErrorMessage.m @@ -30,7 +30,8 @@ NSString *TSInvalidRecipientKey = @"TSInvalidRecipientKey"; forRecipient:(NSString *)recipientId preKeyBundle:(PreKeyBundle *)preKeyBundle { - self = [super initWithTimestamp:message.timestamp + // We want the error message to appear after the message. + self = [super initWithTimestamp:message.timestamp + 1 inThread:thread failedMessageType:TSErrorMessageWrongTrustedIdentityKey recipientId:recipientId]; diff --git a/SignalServiceKit/src/Messages/OWSBatchMessageProcessor.m b/SignalServiceKit/src/Messages/OWSBatchMessageProcessor.m index 0f67fa140..032b014a0 100644 --- a/SignalServiceKit/src/Messages/OWSBatchMessageProcessor.m +++ b/SignalServiceKit/src/Messages/OWSBatchMessageProcessor.m @@ -133,8 +133,9 @@ NSString *const OWSMessageContentJobFinderExtensionGroup = @"OWSBatchMessageProc - (void)addJobWithEnvelopeData:(NSData *)envelopeData plaintextData:(NSData *_Nullable)plaintextData { [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) { - [[[OWSMessageContentJob alloc] initWithEnvelopeData:envelopeData plaintextData:plaintextData] - saveWithTransaction:transaction]; + OWSMessageContentJob *job = + [[OWSMessageContentJob alloc] initWithEnvelopeData:envelopeData plaintextData:plaintextData]; + [job saveWithTransaction:transaction]; }]; } @@ -205,6 +206,18 @@ NSString *const OWSMessageContentJobFinderExtensionGroup = @"OWSBatchMessageProc [database registerExtension:[self databaseExtension] withName:OWSMessageContentJobFinderExtensionName]; } +#pragma mark Logging + ++ (NSString *)tag +{ + return [NSString stringWithFormat:@"[%@]", self.class]; +} + +- (NSString *)tag +{ + return self.class.tag; +} + @end #pragma mark - Queue Processing diff --git a/SignalServiceKit/src/Messages/OWSDisappearingMessagesJob.m b/SignalServiceKit/src/Messages/OWSDisappearingMessagesJob.m index 6f57812cf..3d6d6f04b 100644 --- a/SignalServiceKit/src/Messages/OWSDisappearingMessagesJob.m +++ b/SignalServiceKit/src/Messages/OWSDisappearingMessagesJob.m @@ -266,12 +266,14 @@ NS_ASSUME_NONNULL_BEGIN TSIncomingMessage *incomingMessage = (TSIncomingMessage *)message; NSString *contactName = [contactsManager displayNameForPhoneIdentifier:incomingMessage.messageAuthorId]; - [[[OWSDisappearingConfigurationUpdateInfoMessage alloc] initWithTimestamp:message.timestamp + // We want the info message to appear after the message. + [[[OWSDisappearingConfigurationUpdateInfoMessage alloc] initWithTimestamp:message.timestamp + 1 thread:message.thread configuration:disappearingMessagesConfiguration createdByRemoteName:contactName] save]; } else { - [[[OWSDisappearingConfigurationUpdateInfoMessage alloc] initWithTimestamp:message.timestamp + // We want the info message to appear after the message. + [[[OWSDisappearingConfigurationUpdateInfoMessage alloc] initWithTimestamp:message.timestamp + 1 thread:message.thread configuration:disappearingMessagesConfiguration] save]; diff --git a/SignalServiceKit/src/Messages/OWSIdentityManager.h b/SignalServiceKit/src/Messages/OWSIdentityManager.h index 531d00d04..cd634b1b4 100644 --- a/SignalServiceKit/src/Messages/OWSIdentityManager.h +++ b/SignalServiceKit/src/Messages/OWSIdentityManager.h @@ -46,6 +46,7 @@ extern const NSUInteger kIdentityKeyLength; */ - (nullable OWSRecipientIdentity *)untrustedIdentityForSendingToRecipientId:(NSString *)recipientId; +// This method can be called from any thread. - (void)processIncomingSyncMessage:(OWSSignalServiceProtosVerified *)verified; @end diff --git a/SignalServiceKit/src/Messages/OWSIdentityManager.m b/SignalServiceKit/src/Messages/OWSIdentityManager.m index 834cca6ff..dc0844e33 100644 --- a/SignalServiceKit/src/Messages/OWSIdentityManager.m +++ b/SignalServiceKit/src/Messages/OWSIdentityManager.m @@ -561,7 +561,6 @@ NSString *const kNSNotificationName_IdentityStateDidChange = @"kNSNotificationNa - (void)processIncomingSyncMessage:(OWSSignalServiceProtosVerified *)verified { - NSString *recipientId = verified.destination; if (recipientId.length < 1) { OWSFail(@"Verification state sync message missing recipientId."); diff --git a/SignalServiceKit/src/Messages/OWSMessageDecrypter.m b/SignalServiceKit/src/Messages/OWSMessageDecrypter.m index 2d8f1b651..6ed8ab086 100644 --- a/SignalServiceKit/src/Messages/OWSMessageDecrypter.m +++ b/SignalServiceKit/src/Messages/OWSMessageDecrypter.m @@ -247,7 +247,7 @@ NS_ASSUME_NONNULL_BEGIN NSData *plaintextData = [[cipher decrypt:cipherMessage] removePadding]; successBlock(plaintextData); } @catch (NSException *exception) { - dispatch_async(dispatch_get_main_queue(), ^{ + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ [self processException:exception envelope:envelope]; NSString *errorDescription = [NSString stringWithFormat:@"Exception while decrypting %@: %@", cipherTypeName, exception.description]; @@ -260,8 +260,6 @@ NS_ASSUME_NONNULL_BEGIN - (void)processException:(NSException *)exception envelope:(OWSSignalServiceProtosEnvelope *)envelope { - OWSAssert([NSThread isMainThread]); - DDLogError(@"%@ Got exception: %@ of type: %@ with reason: %@", self.tag, exception.description, @@ -295,6 +293,7 @@ NS_ASSUME_NONNULL_BEGIN errorMessage = [TSErrorMessage corruptedMessageWithEnvelope:envelope withTransaction:transaction]; } + OWSAssert(errorMessage); [errorMessage saveWithTransaction:transaction]; }]; @@ -306,7 +305,10 @@ NS_ASSUME_NONNULL_BEGIN - (void)notifyForErrorMessage:(TSErrorMessage *)errorMessage withEnvelope:(OWSSignalServiceProtosEnvelope *)envelope { TSThread *contactThread = [TSContactThread getOrCreateThreadWithContactId:envelope.source]; - [[TextSecureKitEnv sharedEnv].notificationsManager notifyUserForErrorMessage:errorMessage inThread:contactThread]; + dispatch_async(dispatch_get_main_queue(), ^{ + [[TextSecureKitEnv sharedEnv].notificationsManager notifyUserForErrorMessage:errorMessage + inThread:contactThread]; + }); } #pragma mark - Logging diff --git a/SignalServiceKit/src/Messages/OWSMessageManager.m b/SignalServiceKit/src/Messages/OWSMessageManager.m index eb1e51912..eae549d34 100644 --- a/SignalServiceKit/src/Messages/OWSMessageManager.m +++ b/SignalServiceKit/src/Messages/OWSMessageManager.m @@ -191,7 +191,9 @@ NS_ASSUME_NONNULL_BEGIN TSOutgoingMessage *outgoingMessage = (TSOutgoingMessage *)interaction; [outgoingMessage updateWithWasDeliveredWithTransaction:transaction]; } else { - OWSFail(@"%@ Unexpected message with timestamp: %llu", self.tag, envelope.timestamp); + // Desktop currently sends delivery receipts for "unpersisted" messages + // like group updates, so these errors are expected to a certain extent. + DDLogError(@"%@ Unexpected message with timestamp: %llu", self.tag, envelope.timestamp); } } @@ -292,7 +294,6 @@ NS_ASSUME_NONNULL_BEGIN OWSSyncGroupsRequestMessage *syncGroupsRequestMessage = [[OWSSyncGroupsRequestMessage alloc] initWithThread:thread groupId:groupId]; [self.messageSender sendMessage:syncGroupsRequestMessage - transaction:transaction success:^{ DDLogWarn(@"%@ Successfully sent Request Group Info message.", self.tag); } @@ -645,7 +646,6 @@ NS_ASSUME_NONNULL_BEGIN - (void)sendGroupUpdateForThread:(TSGroupThread *)gThread message:(TSOutgoingMessage *)message { - OWSAssert([NSThread isMainThread]); OWSAssert(gThread); OWSAssert(gThread.groupModel); OWSAssert(message); @@ -715,9 +715,7 @@ NS_ASSUME_NONNULL_BEGIN // Only send this group update to the requester. [message updateWithSingleGroupRecipient:envelope.source transaction:transaction]; - dispatch_async(dispatch_get_main_queue(), ^{ - [self sendGroupUpdateForThread:gThread message:message]; - }); + [self sendGroupUpdateForThread:gThread message:message]; } - (TSIncomingMessage *_Nullable)handleReceivedEnvelope:(OWSSignalServiceProtosEnvelope *)envelope @@ -855,7 +853,7 @@ NS_ASSUME_NONNULL_BEGIN // Other clients allow attachments to be sent along with body, we want the text displayed as a separate // message if ([attachmentIds count] > 0 && body != nil && body.length > 0) { - // We want the text to be displayed under the attachment + // We want the text to be displayed under the attachment. uint64_t textMessageTimestamp = timestamp + 1; TSIncomingMessage *textMessage = [[TSIncomingMessage alloc] initWithTimestamp:textMessageTimestamp inThread:thread diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.h b/SignalServiceKit/src/Messages/OWSMessageSender.h index 43a2b5047..0061b7bef 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.h +++ b/SignalServiceKit/src/Messages/OWSMessageSender.h @@ -69,10 +69,6 @@ NS_SWIFT_NAME(MessageSender) - (void)sendMessage:(TSOutgoingMessage *)message success:(void (^)())successHandler failure:(void (^)(NSError *error))failureHandler; -- (void)sendMessage:(TSOutgoingMessage *)message - transaction:(YapDatabaseReadWriteTransaction *_Nullable)transaction - success:(void (^)())successHandler - failure:(void (^)(NSError *error))failureHandler; /** * Takes care of allocating and uploading the attachment, then sends the message. diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index 1695fcfd6..c7e117710 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -32,6 +32,7 @@ #import "TSStorageManager+sessionStore.h" #import "TSStorageManager.h" #import "TSThread.h" +#import "Threading.h" #import #import #import @@ -424,33 +425,36 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; - (void)sendMessage:(TSOutgoingMessage *)message success:(void (^)())successHandler failure:(void (^)(NSError *error))failureHandler -{ - [self sendMessage:message transaction:nil success:successHandler failure:failureHandler]; -} - -- (void)sendMessage:(TSOutgoingMessage *)message - transaction:(YapDatabaseReadWriteTransaction *_Nullable)transaction - success:(void (^)())successHandler - failure:(void (^)(NSError *error))failureHandler { OWSAssert(message); - if (transaction) { - [message updateWithMessageState:TSOutgoingMessageStateAttemptingOut transaction:transaction]; - } else { + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + // This method will use a read/write transaction. This transaction + // will block until any open read/write transactions are complete. + // + // That's key - we don't want to send any messages in response + // to an incoming message until processing of that batch of messages + // is complete. [message updateWithMessageState:TSOutgoingMessageStateAttemptingOut]; - } - OWSSendMessageOperation *sendMessageOperation = [[OWSSendMessageOperation alloc] initWithMessage:message - messageSender:self - success:successHandler - failure:failureHandler]; - // We call `startBackgroundTask` here to prevent our app from suspending while being backgrounded - // until the operation is completed - at which point the OWSSendMessageOperation ends it's background task. - [sendMessageOperation startBackgroundTask]; + OWSSendMessageOperation *sendMessageOperation = + [[OWSSendMessageOperation alloc] initWithMessage:message + messageSender:self + success:successHandler + failure:failureHandler]; - NSOperationQueue *sendingQueue = [self sendingQueueForMessage:message]; - [sendingQueue addOperation:sendMessageOperation]; + // startBackgroundTask must be called on the main thread. + dispatch_async(dispatch_get_main_queue(), ^{ + // We call `startBackgroundTask` here to prevent our app from suspending while being backgrounded + // until the operation is completed - at which point the OWSSendMessageOperation ends it's background task. + [sendMessageOperation startBackgroundTask]; + + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + NSOperationQueue *sendingQueue = [self sendingQueueForMessage:message]; + [sendingQueue addOperation:sendMessageOperation]; + }); + }); + }); } - (void)attemptToSendMessage:(TSOutgoingMessage *)message @@ -556,9 +560,7 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; } [message save]; - dispatch_async(dispatch_get_main_queue(), ^{ - [self sendMessage:message success:successHandler failure:failureHandler]; - }); + [self sendMessage:message success:successHandler failure:failureHandler]; }); } @@ -1127,6 +1129,8 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; TSContactThread *cThread = [TSContactThread getOrCreateThreadWithContactId:contactId transaction:transaction]; [cThread saveWithTransaction:transaction]; + + // We want the incoming message to appear after the outgoing message. TSIncomingMessage *incomingMessage = [[TSIncomingMessage alloc] initWithTimestamp:(outgoingMessage.timestamp + 1) inThread:cThread @@ -1324,12 +1328,14 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; // TODO: Why is this necessary? [message save]; } else if (message.groupMetaMessage == TSGroupMessageQuit) { - [[[TSInfoMessage alloc] initWithTimestamp:message.timestamp + // We want the info message to appear after the message. + [[[TSInfoMessage alloc] initWithTimestamp:message.timestamp + 1 inThread:thread messageType:TSInfoMessageTypeGroupQuit customMessage:message.customMessage] save]; } else { - [[[TSInfoMessage alloc] initWithTimestamp:message.timestamp + // We want the info message to appear after the message. + [[[TSInfoMessage alloc] initWithTimestamp:message.timestamp + 1 inThread:thread messageType:TSInfoMessageTypeGroupUpdate customMessage:message.customMessage] save];