From 16d4256e99b5ba3e2119cf99c9483fb328deb63e Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Sun, 24 Sep 2017 23:51:58 -0400 Subject: [PATCH] Address deadlocks in profile manager. // FREEBIE --- Signal/src/Profiles/OWSProfileManager.m | 53 ++++++++++--------- .../src/ViewControllers/HomeViewController.m | 1 - Signal/src/util/ThreadUtil.m | 33 ++++++------ .../Messages/Interactions/TSOutgoingMessage.m | 1 - .../src/Messages/OWSProfileKeyMessage.m | 5 +- 5 files changed, 46 insertions(+), 47 deletions(-) diff --git a/Signal/src/Profiles/OWSProfileManager.m b/Signal/src/Profiles/OWSProfileManager.m index f523a400c..47356f458 100644 --- a/Signal/src/Profiles/OWSProfileManager.m +++ b/Signal/src/Profiles/OWSProfileManager.m @@ -307,10 +307,7 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; _localUserProfile.profileKey = [OWSAES256Key generateRandomKey]; dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - @synchronized(self) - { - [self saveUserProfile:_localUserProfile]; - } + [self saveUserProfile:_localUserProfile]; }); } } @@ -534,21 +531,23 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; // TODO: Revisit this so that failed profile updates don't leave // the profile avatar blank, etc. void (^clearLocalAvatar)() = ^{ - @synchronized(self) - { - UserProfile *userProfile = self.localUserProfile; - OWSAssert(userProfile); + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + @synchronized(self) + { + UserProfile *userProfile = self.localUserProfile; + OWSAssert(userProfile); - // TODO remote avatarUrlPath changes as result of fetching form - - // we should probably invalidate it at that point, and refresh again when - // uploading file completes. - userProfile.avatarUrlPath = nil; - userProfile.avatarFileName = nil; + // TODO remote avatarUrlPath changes as result of fetching form - + // we should probably invalidate it at that point, and refresh again when + // uploading file completes. + userProfile.avatarUrlPath = nil; + userProfile.avatarFileName = nil; - [self saveUserProfile:userProfile]; + [self saveUserProfile:userProfile]; - self.localCachedAvatarImage = nil; - } + self.localCachedAvatarImage = nil; + } + }); }; dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ @@ -754,17 +753,19 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; - (void)regenerateLocalProfile { - @synchronized(self) - { - _localUserProfile = nil; - DDLogWarn(@"%@ Removing local user profile", self.tag); - [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) { - [transaction removeObjectForKey:kLocalProfileUniqueId inCollection:[UserProfile collection]]; - }]; + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + @synchronized(self) + { + _localUserProfile = nil; + DDLogWarn(@"%@ Removing local user profile", self.tag); + [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) { + [transaction removeObjectForKey:kLocalProfileUniqueId inCollection:[UserProfile collection]]; + }]; - // rebuild localUserProfile - OWSAssert(self.localUserProfile); - } + // rebuild localUserProfile + OWSAssert(self.localUserProfile); + } + }); } - (void)addUserToProfileWhitelist:(NSString *)recipientId diff --git a/Signal/src/ViewControllers/HomeViewController.m b/Signal/src/ViewControllers/HomeViewController.m index 1387712b7..df2975616 100644 --- a/Signal/src/ViewControllers/HomeViewController.m +++ b/Signal/src/ViewControllers/HomeViewController.m @@ -10,7 +10,6 @@ #import "NewContactThreadViewController.h" #import "OWSContactsManager.h" #import "OWSNavigationController.h" -#import "OWSProfileManager.h" #import "ProfileViewController.h" #import "PushManager.h" #import "Signal-Swift.h" diff --git a/Signal/src/util/ThreadUtil.m b/Signal/src/util/ThreadUtil.m index 0df0b3ee9..374c7fec9 100644 --- a/Signal/src/util/ThreadUtil.m +++ b/Signal/src/util/ThreadUtil.m @@ -149,6 +149,19 @@ NS_ASSUME_NONNULL_BEGIN NSString *localNumber = [TSAccountManager localNumber]; OWSAssert(localNumber.length > 0); + // Many OWSProfileManager methods aren't safe to call from inside a database + // transaction, so do this work now. + OWSProfileManager *profileManager = OWSProfileManager.sharedManager; + BOOL hasLocalProfile = [profileManager hasLocalProfile]; + BOOL isThreadInProfileWhitelist = [profileManager isThreadInProfileWhitelist:thread]; + BOOL hasUnwhitelistedMember = NO; + for (NSString *recipientId in thread.recipientIdentifiers) { + if (![profileManager isUserInProfileWhitelist:recipientId]) { + hasUnwhitelistedMember = YES; + break; + } + } + ThreadDynamicInteractions *result = [ThreadDynamicInteractions new]; [dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { @@ -407,24 +420,14 @@ NS_ASSUME_NONNULL_BEGIN shouldHaveBlockOffer = NO; } - if (![OWSProfileManager.sharedManager hasLocalProfile] || - [OWSProfileManager.sharedManager isThreadInProfileWhitelist:thread]) { + if (!hasLocalProfile || isThreadInProfileWhitelist) { // Don't show offer if thread is local user hasn't configured their profile. // Don't show offer if thread is already in profile whitelist. shouldHaveAddToProfileWhitelistOffer = NO; - } else if (thread.isGroupThread) { - BOOL hasUnwhitelistedMember = NO; - for (NSString *recipientId in thread.recipientIdentifiers) { - if (![OWSProfileManager.sharedManager isUserInProfileWhitelist:recipientId]) { - hasUnwhitelistedMember = YES; - break; - } - } - if (!hasUnwhitelistedMember) { - // Don't show offer in group thread if all members are already individually - // whitelisted. - shouldHaveAddToProfileWhitelistOffer = NO; - } + } else if (thread.isGroupThread && !hasUnwhitelistedMember) { + // Don't show offer in group thread if all members are already individually + // whitelisted. + shouldHaveAddToProfileWhitelistOffer = NO; } BOOL shouldHaveContactOffers diff --git a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m index 272d864d4..8b686304a 100644 --- a/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m +++ b/SignalServiceKit/src/Messages/Interactions/TSOutgoingMessage.m @@ -6,7 +6,6 @@ #import "NSDate+OWS.h" #import "OWSOutgoingSyncMessage.h" #import "OWSSignalServiceProtos.pb.h" -#import "ProfileManagerProtocol.h" #import "ProtoBuf+OWS.h" #import "SignalRecipient.h" #import "TSAttachmentStream.h" diff --git a/SignalServiceKit/src/Messages/OWSProfileKeyMessage.m b/SignalServiceKit/src/Messages/OWSProfileKeyMessage.m index 06af08c8c..73254cbd3 100644 --- a/SignalServiceKit/src/Messages/OWSProfileKeyMessage.m +++ b/SignalServiceKit/src/Messages/OWSProfileKeyMessage.m @@ -36,10 +36,7 @@ NS_ASSUME_NONNULL_BEGIN // Once we've shared our profile key with a user (perhaps due to being // a member of a whitelisted group), make sure they're whitelisted. id profileManager = [TextSecureKitEnv sharedEnv].profileManager; - // FIXME PERF avoid this dispatch. It's going to happen for *each* recipient in a group message. - dispatch_async(dispatch_get_main_queue(), ^{ - [profileManager addUserToProfileWhitelist:recipientId]; - }); + [profileManager addUserToProfileWhitelist:recipientId]; } return [builder build];