From cb365d0a5872f6c59fc31e87265a1166c2e8f15c Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 25 Sep 2017 12:38:35 -0400 Subject: [PATCH 1/3] Address deadlocks in profile manager. // FREEBIE --- Signal/src/Profiles/OWSProfileManager.h | 6 +- Signal/src/Profiles/OWSProfileManager.m | 141 ++++++++++++++++-------- 2 files changed, 96 insertions(+), 51 deletions(-) diff --git a/Signal/src/Profiles/OWSProfileManager.h b/Signal/src/Profiles/OWSProfileManager.h index a6f8d42a5..b2eac1bcb 100644 --- a/Signal/src/Profiles/OWSProfileManager.h +++ b/Signal/src/Profiles/OWSProfileManager.h @@ -16,9 +16,9 @@ extern NSString *const kNSNotificationKey_ProfileGroupId; extern const NSUInteger kOWSProfileManager_NameDataLength; extern const NSUInteger kOWSProfileManager_MaxAvatarDiameter; -@class TSThread; @class OWSAES256Key; @class OWSMessageSender; +@class TSThread; // This class can be safely accessed and used from any thread. @interface OWSProfileManager : NSObject @@ -63,10 +63,6 @@ extern const NSUInteger kOWSProfileManager_MaxAvatarDiameter; - (void)addThreadToProfileWhitelist:(TSThread *)thread; -- (BOOL)isThreadInProfileWhitelist:(TSThread *)thread; - -- (BOOL)isUserInProfileWhitelist:(NSString *)recipientId; - - (void)setContactRecipientIds:(NSArray *)contactRecipientIds; #pragma mark - Other User's Profiles diff --git a/Signal/src/Profiles/OWSProfileManager.m b/Signal/src/Profiles/OWSProfileManager.m index 9c46b8aa1..083d88585 100644 --- a/Signal/src/Profiles/OWSProfileManager.m +++ b/Signal/src/Profiles/OWSProfileManager.m @@ -26,20 +26,20 @@ NS_ASSUME_NONNULL_BEGIN // UserProfile properties may be read from any thread, but should // only be mutated when synchronized on the profile manager. -@interface UserProfile : TSYapDatabaseObject +@interface UserProfile : TSYapDatabaseObject @property (atomic, readonly) NSString *recipientId; @property (atomic, nullable) OWSAES256Key *profileKey; -@property (nonatomic, nullable) NSString *profileName; -@property (nonatomic, nullable) NSString *avatarUrlPath; +@property (atomic, nullable) NSString *profileName; +@property (atomic, nullable) NSString *avatarUrlPath; // This filename is relative to OWSProfileManager.profileAvatarsDirPath. -@property (nonatomic, nullable) NSString *avatarFileName; +@property (atomic, nullable) NSString *avatarFileName; // This should reflect when either: // // * The last successful update finished. // * The current in-flight update began. -@property (nonatomic, nullable) NSDate *lastUpdateDate; +@property (atomic, nullable) NSDate *lastUpdateDate; - (instancetype)init NS_UNAVAILABLE; @@ -49,6 +49,8 @@ NS_ASSUME_NONNULL_BEGIN @implementation UserProfile +@synthesize profileName = _profileName; + - (instancetype)initWithRecipientId:(NSString *)recipientId { self = [super initWithUniqueId:recipientId]; @@ -63,23 +65,67 @@ NS_ASSUME_NONNULL_BEGIN return self; } +- (nullable NSString *)profileName +{ + @synchronized(self) + { + return _profileName; + } +} + - (void)setProfileName:(nullable NSString *)profileName { - _profileName = [profileName ows_stripped]; + @synchronized(self) + { + _profileName = [profileName ows_stripped]; + } +} + +#pragma mark - NSCopying + +- (id)copyWithZone:(nullable NSZone *)zone +{ + UserProfile *copy = [[UserProfile alloc] initWithRecipientId:self.recipientId]; + copy.profileKey = self.profileKey; + copy.profileName = self.profileName; + copy.avatarUrlPath = self.avatarUrlPath; + copy.avatarFileName = self.avatarFileName; + copy.lastUpdateDate = self.lastUpdateDate; + return copy; } #pragma mark - NSObject -- (BOOL)isEqual:(UserProfile *)other +- (BOOL)areEqualStrings:(NSString *)left other:(NSString *)right { - return ([other isKindOfClass:[UserProfile class]] && [self.recipientId isEqualToString:other.recipientId] && - [self.profileName isEqualToString:other.profileName] && [self.avatarUrlPath isEqualToString:other.avatarUrlPath] && - [self.avatarFileName isEqualToString:other.avatarFileName]); + if (!left && !right) { + return YES; + } + if (!left || !right) { + return NO; + } + return [left isEqualToString:right]; } -- (NSUInteger)hash +- (BOOL)areEqualKeys:(OWSAES256Key *)left other:(OWSAES256Key *)right { - return self.recipientId.hash ^ self.profileName.hash ^ self.avatarUrlPath.hash ^ self.avatarFileName.hash; + if (!left && !right) { + return YES; + } + if (!left || !right) { + return NO; + } + return [left.keyData isEqual:right.keyData]; +} + +- (BOOL)isEqual:(UserProfile *)other +{ + // Don't bother comparing lastUpdateDate property. + return ([other isKindOfClass:[UserProfile class]] && [self areEqualStrings:self.recipientId other:other.recipientId] + && [self areEqualStrings:self.profileName other:other.profileName] && + [self areEqualKeys:self.profileKey other:other.profileKey] && + [self areEqualStrings:self.avatarUrlPath other:other.avatarUrlPath] && + [self areEqualStrings:self.avatarFileName other:other.avatarFileName]); } @end @@ -234,42 +280,48 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; { OWSAssert(userProfile); + // Make a copy to use inside the transaction. + // To avoid deadlock, we want to avoid creating a new transaction while sync'd on self. + UserProfile *userProfileCopy; @synchronized(self) { - // Make sure to save on the local db connection for consistency. - [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { - [userProfile saveWithTransaction:transaction]; - }]; + userProfileCopy = [userProfile copy]; + // Other threads may modify this profile's properties + OWSAssert([userProfile isEqual:userProfileCopy]); + } - BOOL isLocalUserProfile = userProfile == self.localUserProfile; + // Make sure to save on the local db connection for consistency. + [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + [userProfileCopy saveWithTransaction:transaction]; + }]; - dispatch_async(dispatch_get_main_queue(), ^{ - if (isLocalUserProfile) { - [MultiDeviceProfileKeyUpdateJob runWithProfileKey:userProfile.profileKey - identityManager:self.identityManager - messageSender:self.messageSender - profileManager:self]; + BOOL isLocalUserProfile = userProfile == self.localUserProfile; - [[NSNotificationCenter defaultCenter] - postNotificationNameAsync:kNSNotificationName_LocalProfileDidChange - object:nil - userInfo:nil]; - } else { - [[NSNotificationCenter defaultCenter] - postNotificationNameAsync:kNSNotificationName_OtherUsersProfileWillChange - object:nil - userInfo:@{ - kNSNotificationKey_ProfileRecipientId : userProfile.recipientId, - }]; - [[NSNotificationCenter defaultCenter] - postNotificationNameAsync:kNSNotificationName_OtherUsersProfileDidChange - object:nil - userInfo:@{ - kNSNotificationKey_ProfileRecipientId : userProfile.recipientId, - }]; - } - }); - } + dispatch_async(dispatch_get_main_queue(), ^{ + if (isLocalUserProfile) { + [MultiDeviceProfileKeyUpdateJob runWithProfileKey:userProfile.profileKey + identityManager:self.identityManager + messageSender:self.messageSender + profileManager:self]; + + [[NSNotificationCenter defaultCenter] postNotificationNameAsync:kNSNotificationName_LocalProfileDidChange + object:nil + userInfo:nil]; + } else { + [[NSNotificationCenter defaultCenter] + postNotificationNameAsync:kNSNotificationName_OtherUsersProfileWillChange + object:nil + userInfo:@{ + kNSNotificationKey_ProfileRecipientId : userProfile.recipientId, + }]; + [[NSNotificationCenter defaultCenter] + postNotificationNameAsync:kNSNotificationName_OtherUsersProfileDidChange + object:nil + userInfo:@{ + kNSNotificationKey_ProfileRecipientId : userProfile.recipientId, + }]; + } + }); } - (void)ensureLocalProfileCached @@ -295,9 +347,6 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; DDLogInfo(@"%@ Building local profile.", self.tag); _localUserProfile = [[UserProfile alloc] initWithRecipientId:kLocalProfileUniqueId]; _localUserProfile.profileKey = [OWSAES256Key generateRandomKey]; - [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) { - [_localUserProfile saveWithTransaction:transaction]; - }]; // Sync local profile to any linked device dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ @synchronized(self) From 57b5ccdc3f4fc92d299791629c82caacaf17fbf7 Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 25 Sep 2017 14:28:24 -0400 Subject: [PATCH 2/3] Address deadlocks in profile manager. // FREEBIE --- Signal/src/Profiles/OWSProfileManager.m | 71 ++++++++++--------------- 1 file changed, 29 insertions(+), 42 deletions(-) diff --git a/Signal/src/Profiles/OWSProfileManager.m b/Signal/src/Profiles/OWSProfileManager.m index 083d88585..b6f347579 100644 --- a/Signal/src/Profiles/OWSProfileManager.m +++ b/Signal/src/Profiles/OWSProfileManager.m @@ -813,23 +813,7 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; { OWSAssert(recipientId.length > 0); - dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - @synchronized(self) - { - if ([self isUserInProfileWhitelist:recipientId]) { - return; - } - - self.userProfileWhitelistCache[recipientId] = @(YES); - [self.dbConnection setBool:YES forKey:recipientId inCollection:kOWSProfileManager_UserWhitelistCollection]; - } - - [[NSNotificationCenter defaultCenter] postNotificationNameAsync:kNSNotificationName_ProfileWhitelistDidChange - object:nil - userInfo:@{ - kNSNotificationKey_ProfileRecipientId : recipientId, - }]; - }); + [self addUsersToProfileWhitelist:@[ recipientId ]]; } - (void)addUsersToProfileWhitelist:(NSArray *)recipientIds @@ -851,24 +835,27 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; return; } - [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { - for (NSString *recipientId in recipientIds) { - [transaction setObject:@(YES) - forKey:recipientId - inCollection:kOWSProfileManager_UserWhitelistCollection]; - self.userProfileWhitelistCache[recipientId] = @(YES); - } - }]; + for (NSString *recipientId in recipientIds) { + self.userProfileWhitelistCache[recipientId] = @(YES); + } } - for (NSString *recipientId in newRecipientIds) { - [[NSNotificationCenter defaultCenter] - postNotificationNameAsync:kNSNotificationName_ProfileWhitelistDidChange - object:nil - userInfo:@{ - kNSNotificationKey_ProfileRecipientId : recipientId, - }]; + [self.dbConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + for (NSString *recipientId in recipientIds) { + [transaction setObject:@(YES) + forKey:recipientId + inCollection:kOWSProfileManager_UserWhitelistCollection]; } + }]; + + for (NSString *recipientId in newRecipientIds) { + [[NSNotificationCenter defaultCenter] + postNotificationNameAsync:kNSNotificationName_ProfileWhitelistDidChange + object:nil + userInfo:@{ + kNSNotificationKey_ProfileRecipientId : recipientId, + }]; + } }); } @@ -895,24 +882,24 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; OWSAssert(groupId.length > 0); dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ + NSString *groupIdKey = [groupId hexadecimalString]; + @synchronized(self) { - NSString *groupIdKey = [groupId hexadecimalString]; - if ([self isGroupIdInProfileWhitelist:groupId]) { return; } - [self.dbConnection setBool:YES forKey:groupIdKey inCollection:kOWSProfileManager_GroupWhitelistCollection]; self.groupProfileWhitelistCache[groupIdKey] = @(YES); - - [[NSNotificationCenter defaultCenter] - postNotificationNameAsync:kNSNotificationName_ProfileWhitelistDidChange - object:nil - userInfo:@{ - kNSNotificationKey_ProfileGroupId : groupId, - }]; } + + [self.dbConnection setBool:YES forKey:groupIdKey inCollection:kOWSProfileManager_GroupWhitelistCollection]; + + [[NSNotificationCenter defaultCenter] postNotificationNameAsync:kNSNotificationName_ProfileWhitelistDidChange + object:nil + userInfo:@{ + kNSNotificationKey_ProfileGroupId : groupId, + }]; }); } From 9dcc7e1ea0c5e1456d5c226e613560e0236c3c5c Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Tue, 26 Sep 2017 09:46:47 -0400 Subject: [PATCH 3/3] Respond to CR. // FREEBIE --- Signal/src/Profiles/OWSProfileManager.m | 51 +------------------------ 1 file changed, 2 insertions(+), 49 deletions(-) diff --git a/Signal/src/Profiles/OWSProfileManager.m b/Signal/src/Profiles/OWSProfileManager.m index b6f347579..3724ad1c6 100644 --- a/Signal/src/Profiles/OWSProfileManager.m +++ b/Signal/src/Profiles/OWSProfileManager.m @@ -26,7 +26,7 @@ NS_ASSUME_NONNULL_BEGIN // UserProfile properties may be read from any thread, but should // only be mutated when synchronized on the profile manager. -@interface UserProfile : TSYapDatabaseObject +@interface UserProfile : TSYapDatabaseObject @property (atomic, readonly) NSString *recipientId; @property (atomic, nullable) OWSAES256Key *profileKey; @@ -81,53 +81,6 @@ NS_ASSUME_NONNULL_BEGIN } } -#pragma mark - NSCopying - -- (id)copyWithZone:(nullable NSZone *)zone -{ - UserProfile *copy = [[UserProfile alloc] initWithRecipientId:self.recipientId]; - copy.profileKey = self.profileKey; - copy.profileName = self.profileName; - copy.avatarUrlPath = self.avatarUrlPath; - copy.avatarFileName = self.avatarFileName; - copy.lastUpdateDate = self.lastUpdateDate; - return copy; -} - -#pragma mark - NSObject - -- (BOOL)areEqualStrings:(NSString *)left other:(NSString *)right -{ - if (!left && !right) { - return YES; - } - if (!left || !right) { - return NO; - } - return [left isEqualToString:right]; -} - -- (BOOL)areEqualKeys:(OWSAES256Key *)left other:(OWSAES256Key *)right -{ - if (!left && !right) { - return YES; - } - if (!left || !right) { - return NO; - } - return [left.keyData isEqual:right.keyData]; -} - -- (BOOL)isEqual:(UserProfile *)other -{ - // Don't bother comparing lastUpdateDate property. - return ([other isKindOfClass:[UserProfile class]] && [self areEqualStrings:self.recipientId other:other.recipientId] - && [self areEqualStrings:self.profileName other:other.profileName] && - [self areEqualKeys:self.profileKey other:other.profileKey] && - [self areEqualStrings:self.avatarUrlPath other:other.avatarUrlPath] && - [self areEqualStrings:self.avatarFileName other:other.avatarFileName]); -} - @end #pragma mark - @@ -347,7 +300,7 @@ const NSUInteger kOWSProfileManager_MaxAvatarDiameter = 640; DDLogInfo(@"%@ Building local profile.", self.tag); _localUserProfile = [[UserProfile alloc] initWithRecipientId:kLocalProfileUniqueId]; _localUserProfile.profileKey = [OWSAES256Key generateRandomKey]; - // Sync local profile to any linked device + dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ @synchronized(self) {