From c3dca21a69c6d4d71cc34012e5ec921f11d53d78 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 28 Sep 2017 21:12:02 -0400 Subject: [PATCH] More thread safety fixes. // FREEBIE --- .../Attachments/OWSAttachmentsProcessor.m | 20 +++-- .../src/Messages/Interactions/TSInteraction.h | 2 + .../src/Messages/Interactions/TSInteraction.m | 5 ++ .../src/Messages/OWSDisappearingMessagesJob.m | 83 ++++++++++--------- .../src/Messages/OWSMessageSender.m | 4 +- .../src/Messages/OWSReadReceiptManager.m | 2 +- 6 files changed, 65 insertions(+), 51 deletions(-) diff --git a/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m b/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m index 13cbc5eaf..c47cf6b43 100644 --- a/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m +++ b/SignalServiceKit/src/Messages/Attachments/OWSAttachmentsProcessor.m @@ -363,16 +363,20 @@ static const CGFloat kAttachmentDownloadProgressTheta = 0.001f; hasCheckedContentLength = YES; } success:^(NSURLSessionDataTask *_Nonnull task, id _Nullable responseObject) { - if (![responseObject isKindOfClass:[NSData class]]) { - DDLogError(@"%@ Failed retrieval of attachment. Response had unexpected format.", self.tag); - NSError *error = OWSErrorMakeUnableToProcessServerResponseError(); - return failureHandler(task, error); - } - successHandler((NSData *)responseObject); + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + if (![responseObject isKindOfClass:[NSData class]]) { + DDLogError(@"%@ Failed retrieval of attachment. Response had unexpected format.", self.tag); + NSError *error = OWSErrorMakeUnableToProcessServerResponseError(); + return failureHandler(task, error); + } + successHandler((NSData *)responseObject); + }); } failure:^(NSURLSessionDataTask *_Nullable task, NSError *_Nonnull error) { - DDLogError(@"Failed to retrieve attachment with error: %@", error.description); - return failureHandler(task, error); + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + DDLogError(@"Failed to retrieve attachment with error: %@", error.description); + return failureHandler(task, error); + }); }]; } diff --git a/SignalServiceKit/src/Messages/Interactions/TSInteraction.h b/SignalServiceKit/src/Messages/Interactions/TSInteraction.h index c9bb9ebe7..db49219e3 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSInteraction.h +++ b/SignalServiceKit/src/Messages/Interactions/TSInteraction.h @@ -18,6 +18,8 @@ NS_ASSUME_NONNULL_BEGIN - (NSString *)description; +- (TSThread *)threadWithTransaction:(YapDatabaseReadWriteTransaction *)transaction; + /** * When an interaction is updated, it often affects the UI for it's containing thread. Touching it's thread will notify * any observers so they can redraw any related UI. diff --git a/SignalServiceKit/src/Messages/Interactions/TSInteraction.m b/SignalServiceKit/src/Messages/Interactions/TSInteraction.m index 07d7a10d3..18f4a26c8 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSInteraction.m +++ b/SignalServiceKit/src/Messages/Interactions/TSInteraction.m @@ -77,6 +77,11 @@ NS_ASSUME_NONNULL_BEGIN return [TSThread fetchObjectWithUniqueID:self.uniqueThreadId]; } +- (TSThread *)threadWithTransaction:(YapDatabaseReadWriteTransaction *)transaction +{ + return [TSThread fetchObjectWithUniqueID:self.uniqueThreadId transaction:transaction]; +} + - (void)touchThreadWithTransaction:(YapDatabaseReadWriteTransaction *)transaction { TSThread *thread = [TSThread fetchObjectWithUniqueID:self.uniqueThreadId transaction:transaction]; diff --git a/SignalServiceKit/src/Messages/OWSDisappearingMessagesJob.m b/SignalServiceKit/src/Messages/OWSDisappearingMessagesJob.m index 2f8c1d1b9..e7c83dc02 100644 --- a/SignalServiceKit/src/Messages/OWSDisappearingMessagesJob.m +++ b/SignalServiceKit/src/Messages/OWSDisappearingMessagesJob.m @@ -224,59 +224,62 @@ NS_ASSUME_NONNULL_BEGIN + (void)becomeConsistentWithConfigurationForMessage:(TSMessage *)message contactsManager:(id)contactsManager { - dispatch_async(self.serialQueue, ^{ [[self sharedJob] becomeConsistentWithConfigurationForMessage:message contactsManager:contactsManager]; - }); } - (void)becomeConsistentWithConfigurationForMessage:(TSMessage *)message contactsManager:(id)contactsManager { - // Become eventually consistent in the case that the remote changed their settings at the same time. - // Also in case remote doesn't support expiring messages - OWSDisappearingMessagesConfiguration *disappearingMessagesConfiguration = - [OWSDisappearingMessagesConfiguration fetchOrCreateDefaultWithThreadId:message.uniqueThreadId]; - - BOOL changed = NO; - if (message.expiresInSeconds == 0) { - if (disappearingMessagesConfiguration.isEnabled) { + OWSAssert(message); + OWSAssert(contactsManager); + + dispatch_async(OWSDisappearingMessagesJob.serialQueue, ^{ + // Become eventually consistent in the case that the remote changed their settings at the same time. + // Also in case remote doesn't support expiring messages + OWSDisappearingMessagesConfiguration *disappearingMessagesConfiguration = + [OWSDisappearingMessagesConfiguration fetchOrCreateDefaultWithThreadId:message.uniqueThreadId]; + + BOOL changed = NO; + if (message.expiresInSeconds == 0) { + if (disappearingMessagesConfiguration.isEnabled) { + changed = YES; + DDLogWarn(@"%@ Received remote message which had no expiration set, disabling our expiration to become " + @"consistent.", + self.tag); + disappearingMessagesConfiguration.enabled = NO; + [disappearingMessagesConfiguration save]; + } + } else if (message.expiresInSeconds != disappearingMessagesConfiguration.durationSeconds) { changed = YES; - DDLogWarn(@"%@ Received remote message which had no expiration set, disabling our expiration to become " + DDLogInfo(@"%@ Received remote message with different expiration set, updating our expiration to become " @"consistent.", self.tag); - disappearingMessagesConfiguration.enabled = NO; + disappearingMessagesConfiguration.enabled = YES; + disappearingMessagesConfiguration.durationSeconds = message.expiresInSeconds; [disappearingMessagesConfiguration save]; } - } else if (message.expiresInSeconds != disappearingMessagesConfiguration.durationSeconds) { - changed = YES; - DDLogInfo( - @"%@ Received remote message with different expiration set, updating our expiration to become consistent.", - self.tag); - disappearingMessagesConfiguration.enabled = YES; - disappearingMessagesConfiguration.durationSeconds = message.expiresInSeconds; - [disappearingMessagesConfiguration save]; - } - if (!changed) { - return; - } + if (!changed) { + return; + } - if ([message isKindOfClass:[TSIncomingMessage class]]) { - TSIncomingMessage *incomingMessage = (TSIncomingMessage *)message; - NSString *contactName = [contactsManager displayNameForPhoneIdentifier:incomingMessage.messageAuthorId]; - - // We want the info message to appear _before_ the message. - [[[OWSDisappearingConfigurationUpdateInfoMessage alloc] initWithTimestamp:message.timestamp - 1 - thread:message.thread - configuration:disappearingMessagesConfiguration - createdByRemoteName:contactName] save]; - } else { - // We want the info message to appear _before_ the message. - [[[OWSDisappearingConfigurationUpdateInfoMessage alloc] initWithTimestamp:message.timestamp - 1 - thread:message.thread - configuration:disappearingMessagesConfiguration] - save]; - } + if ([message isKindOfClass:[TSIncomingMessage class]]) { + TSIncomingMessage *incomingMessage = (TSIncomingMessage *)message; + NSString *contactName = [contactsManager displayNameForPhoneIdentifier:incomingMessage.messageAuthorId]; + + // We want the info message to appear _before_ the message. + [[[OWSDisappearingConfigurationUpdateInfoMessage alloc] initWithTimestamp:message.timestamp - 1 + thread:message.thread + configuration:disappearingMessagesConfiguration + createdByRemoteName:contactName] save]; + } else { + // We want the info message to appear _before_ the message. + [[[OWSDisappearingConfigurationUpdateInfoMessage alloc] initWithTimestamp:message.timestamp - 1 + thread:message.thread + configuration:disappearingMessagesConfiguration] + save]; + } + }); } - (void)startIfNecessary diff --git a/SignalServiceKit/src/Messages/OWSMessageSender.m b/SignalServiceKit/src/Messages/OWSMessageSender.m index 2d99619a6..1cd2d0d15 100644 --- a/SignalServiceKit/src/Messages/OWSMessageSender.m +++ b/SignalServiceKit/src/Messages/OWSMessageSender.m @@ -600,9 +600,9 @@ NSString *const OWSMessageSenderRateLimitedException = @"RateLimitedException"; success:(void (^)())successHandler failure:(RetryableFailureHandler)failureHandler { - TSThread *thread = message.thread; - dispatch_async([OWSDispatch sendingQueue], ^{ + TSThread *thread = message.thread; + if ([thread isKindOfClass:[TSGroupThread class]]) { TSGroupThread *gThread = (TSGroupThread *)thread; diff --git a/SignalServiceKit/src/Messages/OWSReadReceiptManager.m b/SignalServiceKit/src/Messages/OWSReadReceiptManager.m index 5a424de0b..8b7f6a660 100644 --- a/SignalServiceKit/src/Messages/OWSReadReceiptManager.m +++ b/SignalServiceKit/src/Messages/OWSReadReceiptManager.m @@ -470,7 +470,7 @@ NSString *const OWSReadReceiptManagerAreReadReceiptsEnabled = @"areReadReceiptsE // Use timestampForSorting which reflects local sort order, rather than timestamp // which reflect sender time. [self markAsReadBeforeTimestamp:message.timestampForSorting - thread:message.thread + thread:[message threadWithTransaction:transaction] wasLocal:NO transaction:transaction]; }