diff --git a/Signal/src/ViewControllers/NewContactThreadViewController.m b/Signal/src/ViewControllers/NewContactThreadViewController.m index 8f0f759c0..acd419397 100644 --- a/Signal/src/ViewControllers/NewContactThreadViewController.m +++ b/Signal/src/ViewControllers/NewContactThreadViewController.m @@ -147,9 +147,13 @@ NS_ASSUME_NONNULL_BEGIN { OWSAssert([NSThread isMainThread]); - [self.contactsViewHelper.contactsManager fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotify]; - - [refreshControl endRefreshing]; + [self.contactsViewHelper.contactsManager + userRequestedSystemContactsRefreshWithCompletion:^(NSError *_Nullable error) { + if (error) { + DDLogError(@"%@ refreshing contacts failed with error: %@", self.logTag, error); + } + [refreshControl endRefreshing]; + }]; } - (void)showContactsPermissionReminder:(BOOL)isVisible diff --git a/Signal/src/contact/OWSContactsManager.h b/Signal/src/contact/OWSContactsManager.h index e26cf05a4..bdd8487d7 100644 --- a/Signal/src/contact/OWSContactsManager.h +++ b/Signal/src/contact/OWSContactsManager.h @@ -52,9 +52,11 @@ extern NSString *const OWSContactsManagerSignalAccountsDidChangeNotification; // Ensure's the app has the latest contacts, but won't prompt the user for contact // access if they haven't granted it. - (void)fetchSystemContactsOnceIfAlreadyAuthorized; + // This variant will fetch system contacts if contact access has already been granted, -// but not prompt for contact access. Also, it will always fire a notification. -- (void)fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotify; +// but not prompt for contact access. Also, it will always notify delegates, even if +// contacts haven't changed, and will clear out any stale cached SignalAccounts +- (void)userRequestedSystemContactsRefreshWithCompletion:(void (^)(NSError *_Nullable error))completionHandler; #pragma mark - Util diff --git a/Signal/src/contact/OWSContactsManager.m b/Signal/src/contact/OWSContactsManager.m index 49cdc9230..90c351a68 100644 --- a/Signal/src/contact/OWSContactsManager.m +++ b/Signal/src/contact/OWSContactsManager.m @@ -63,17 +63,32 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification - (void)loadSignalAccountsFromCache { __block NSMutableArray *signalAccounts; - [self.dbReadConnection readWithBlock:^(YapDatabaseReadTransaction * _Nonnull transaction) { - signalAccounts = [[NSMutableArray alloc] initWithCapacity:[SignalAccount numberOfKeysInCollectionWithTransaction:transaction]]; - + [self.dbReadConnection readWithBlock:^(YapDatabaseReadTransaction *_Nonnull transaction) { + NSUInteger signalAccountCount = [SignalAccount numberOfKeysInCollectionWithTransaction:transaction]; + DDLogInfo(@"%@ loading %lu signal accounts from cache.", self.logTag, (unsigned long)signalAccountCount); + + signalAccounts = [[NSMutableArray alloc] initWithCapacity:signalAccountCount]; + [SignalAccount enumerateCollectionObjectsWithTransaction:transaction usingBlock:^(SignalAccount *signalAccount, BOOL * _Nonnull stop) { [signalAccounts addObject:signalAccount]; }]; }]; - + + [signalAccounts sortUsingComparator:self.signalAccountComparator]; [self updateSignalAccounts:signalAccounts]; } +- (dispatch_queue_t)serialQueue +{ + static dispatch_queue_t _serialQueue; + static dispatch_once_t onceToken; + dispatch_once(&onceToken, ^{ + _serialQueue = dispatch_queue_create("org.whispersystems.contacts.buildSignalAccount", DISPATCH_QUEUE_SERIAL); + }); + + return _serialQueue; +} + #pragma mark - System Contact Fetching // Request contacts access if you haven't asked recently. @@ -92,9 +107,9 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification [self.systemContactsFetcher fetchOnceIfAlreadyAuthorized]; } -- (void)fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotify +- (void)userRequestedSystemContactsRefreshWithCompletion:(void (^)(NSError *_Nullable error))completionHandler { - [self.systemContactsFetcher fetchIfAlreadyAuthorizedAndAlwaysNotify]; + [self.systemContactsFetcher userRequestedRefreshWithCompletion:completionHandler]; } - (BOOL)isSystemContactsAuthorized @@ -116,27 +131,30 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification - (void)systemContactsFetcher:(SystemContactsFetcher *)systemsContactsFetcher updatedContacts:(NSArray *)contacts + isUserRequested:(BOOL)isUserRequested { - [self updateWithContacts:contacts]; + [self updateWithContacts:contacts shouldClearStaleCache:isUserRequested]; } #pragma mark - Intersection -- (void)intersectContacts +- (void)intersectContactsWithCompletion:(void (^)(NSError *_Nullable error))completionBlock { - [self intersectContactsWithRetryDelay:1]; + [self intersectContactsWithRetryDelay:1 completion:completionBlock]; } - (void)intersectContactsWithRetryDelay:(double)retryDelaySeconds + completion:(void (^)(NSError *_Nullable error))completionBlock { void (^success)(void) = ^{ DDLogInfo(@"%@ Successfully intersected contacts.", self.logTag); - [self buildSignalAccounts]; + completionBlock(nil); }; void (^failure)(NSError *error) = ^(NSError *error) { if ([error.domain isEqualToString:OWSSignalServiceKitErrorDomain] && error.code == OWSErrorCodeContactsUpdaterRateLimit) { DDLogError(@"Contact intersection hit rate limit with error: %@", error); + completionBlock(error); return; } @@ -147,7 +165,7 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification // TODO: Abort if another contact intersection succeeds in the meantime. dispatch_after( dispatch_time(DISPATCH_TIME_NOW, (int64_t)(retryDelaySeconds * NSEC_PER_SEC)), dispatch_get_main_queue(), ^{ - [self intersectContactsWithRetryDelay:retryDelaySeconds * 2]; + [self intersectContactsWithRetryDelay:retryDelaySeconds * 2 completion:completionBlock]; }); }; [[ContactsUpdater sharedUpdater] updateSignalContactIntersectionWithABContacts:self.allContacts @@ -173,10 +191,9 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification [self.avatarCache removeAllImagesForKey:recipientId]; } -- (void)updateWithContacts:(NSArray *)contacts +- (void)updateWithContacts:(NSArray *)contacts shouldClearStaleCache:(BOOL)shouldClearStaleCache { - dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - + dispatch_async(self.serialQueue, ^{ NSMutableDictionary *allContactsMap = [NSMutableDictionary new]; for (Contact *contact in contacts) { for (PhoneNumber *phoneNumber in contact.parsedPhoneNumbers) { @@ -193,17 +210,16 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification [self.avatarCache removeAllImages]; - [self intersectContacts]; - - [self buildSignalAccounts]; + [self intersectContactsWithCompletion:^(NSError *_Nullable error) { + [self buildSignalAccountsAndClearStaleCache:shouldClearStaleCache]; + }]; }); }); } -- (void)buildSignalAccounts +- (void)buildSignalAccountsAndClearStaleCache:(BOOL)shouldClearStaleCache; { - dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - NSMutableDictionary *signalAccountMap = [NSMutableDictionary new]; + dispatch_async(self.serialQueue, ^{ NSMutableArray *signalAccounts = [NSMutableArray new]; NSArray *contacts = self.allContacts; @@ -218,9 +234,15 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification } }]; + NSMutableSet *seenRecipientIds = [NSMutableSet new]; for (Contact *contact in contacts) { NSArray *signalRecipients = contactIdToSignalRecipientsMap[contact.uniqueId]; for (SignalRecipient *signalRecipient in [signalRecipients sortedArrayUsingSelector:@selector(compare:)]) { + if ([seenRecipientIds containsObject:signalRecipient.recipientId]) { + DDLogDebug(@"Ignoring duplicate contact: %@, %@", signalRecipient.recipientId, contact.fullName); + continue; + } + [seenRecipientIds addObject:signalRecipient.recipientId]; SignalAccount *signalAccount = [[SignalAccount alloc] initWithSignalRecipient:signalRecipient]; signalAccount.contact = contact; if (signalRecipients.count > 1) { @@ -228,33 +250,81 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification signalAccount.multipleAccountLabelText = [[self class] accountLabelForContact:contact recipientId:signalRecipient.recipientId]; } - if (signalAccountMap[signalAccount.recipientId]) { - DDLogDebug(@"Ignoring duplicate contact: %@, %@", signalAccount.recipientId, contact.fullName); - continue; - } [signalAccounts addObject:signalAccount]; } } + NSMutableDictionary *oldSignalAccounts = [NSMutableDictionary new]; + [self.dbReadConnection readWithBlock:^(YapDatabaseReadTransaction *_Nonnull transaction) { + [SignalAccount + enumerateCollectionObjectsWithTransaction:transaction + usingBlock:^(id _Nonnull object, BOOL *_Nonnull stop) { + OWSAssert([object isKindOfClass:[SignalAccount class]]); + SignalAccount *oldSignalAccount = (SignalAccount *)object; + + oldSignalAccounts[oldSignalAccount.uniqueId] = oldSignalAccount; + }]; + }]; + + NSMutableArray *accountsToSave = [NSMutableArray new]; + for (SignalAccount *signalAccount in signalAccounts) { + SignalAccount *_Nullable oldSignalAccount = oldSignalAccounts[signalAccount.uniqueId]; + + // keep track of which accounts are still relevant, so we can clean up orphans + [oldSignalAccounts removeObjectForKey:signalAccount.uniqueId]; + + if (oldSignalAccount == nil) { + // new Signal Account + [accountsToSave addObject:signalAccount]; + continue; + } + + if ([oldSignalAccount isEqual:signalAccount]) { + // Same value, no need to save. + continue; + } + + // value changed, save account + [accountsToSave addObject:signalAccount]; + } + // Update cached SignalAccounts on disk [self.dbWriteConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) { - NSArray *allKeys = [transaction allKeysInCollection:[SignalAccount collection]]; - NSMutableSet *orphanedKeys = [NSMutableSet setWithArray:allKeys]; - - DDLogInfo(@"%@ Saving %lu SignalAccounts", self.logTag, signalAccounts.count); - for (SignalAccount *signalAccount in signalAccounts) { - // TODO only save the ones that changed - [orphanedKeys removeObject:signalAccount.uniqueId]; + DDLogInfo(@"%@ Saving %lu SignalAccounts", self.logTag, (unsigned long)accountsToSave.count); + for (SignalAccount *signalAccount in accountsToSave) { + DDLogVerbose(@"%@ Saving SignalAccount: %@", self.logTag, signalAccount); [signalAccount saveWithTransaction:transaction]; } - - if (orphanedKeys.count > 0) { - DDLogInfo(@"%@ Removing %lu orphaned SignalAccounts", self.logTag, (unsigned long)orphanedKeys.count); - [transaction removeObjectsForKeys:orphanedKeys.allObjects inCollection:[SignalAccount collection]]; + + if (shouldClearStaleCache) { + DDLogInfo(@"%@ Removing %lu old SignalAccounts.", self.logTag, (unsigned long)oldSignalAccounts.count); + for (SignalAccount *signalAccount in oldSignalAccounts.allValues) { + DDLogVerbose(@"%@ Removing old SignalAccount: %@", self.logTag, signalAccount); + [signalAccount removeWithTransaction:transaction]; + } + } else { + // In theory we want to remove SignalAccounts if the user deletes the corresponding system contact. + // However, as of iOS11.2 CNContactStore occasionally gives us only a subset of the system contacts. + // Because of that, it's not safe to clear orphaned accounts. + // Because we still want to give users a way to clear their stale accounts, if they pull-to-refresh + // their contacts we'll clear the cached ones. + // RADAR: https://bugreport.apple.com/web/?problemID=36082946 + if (oldSignalAccounts.allValues.count > 0) { + DDLogWarn(@"%@ NOT Removing %lu old SignalAccounts.", + self.logTag, + (unsigned long)oldSignalAccounts.count); + for (SignalAccount *signalAccount in oldSignalAccounts.allValues) { + DDLogVerbose( + @"%@ Ensuring old SignalAccount is not inadvertently lost: %@", self.logTag, signalAccount); + [signalAccounts addObject:signalAccount]; + } + + // re-sort signal accounts since we've appended some orphans + [signalAccounts sortUsingComparator:self.signalAccountComparator]; + } } }]; - dispatch_async(dispatch_get_main_queue(), ^{ [self updateSignalAccounts:signalAccounts]; }); @@ -264,7 +334,11 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification - (void)updateSignalAccounts:(NSArray *)signalAccounts { AssertIsOnMainThread(); - + if ([signalAccounts isEqual:self.signalAccounts]) { + DDLogDebug(@"%@ SignalAccounts unchanged.", self.logTag); + return; + } + NSMutableDictionary *signalAccountMap = [NSMutableDictionary new]; for (SignalAccount *signalAccount in signalAccounts) { signalAccountMap[signalAccount.recipientId] = signalAccount; @@ -620,6 +694,16 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification return image; } +- (NSComparisonResult (^)(SignalAccount *left, SignalAccount *right))signalAccountComparator +{ + return ^NSComparisonResult(SignalAccount *left, SignalAccount *right) { + NSString *leftName = [self comparableNameForSignalAccount:left]; + NSString *rightName = [self comparableNameForSignalAccount:right]; + + return [leftName compare:rightName]; + }; +} + - (NSString *)comparableNameForSignalAccount:(SignalAccount *)signalAccount { NSString *_Nullable name; diff --git a/Signal/src/contact/SystemContactsFetcher.swift b/Signal/src/contact/SystemContactsFetcher.swift index 75ab49e14..a70d8b838 100644 --- a/Signal/src/contact/SystemContactsFetcher.swift +++ b/Signal/src/contact/SystemContactsFetcher.swift @@ -185,11 +185,11 @@ class AddressBookContactStoreAdaptee: ContactStoreAdaptee { } } - return Contact(contactWithFirstName: firstName, - andLastName: lastName, - andUserTextPhoneNumbers: phoneNumbers, - andImage: addressBookRecord.image, - andContactID: addressBookRecord.recordId) + return Contact(firstName: firstName, + lastName: lastName, + userTextPhoneNumbers: phoneNumbers, + imageData: addressBookRecord.imageData, + contactID: addressBookRecord.recordId) } } @@ -243,14 +243,14 @@ struct OWSABRecord { } } - var image: UIImage? { + var imageData: Data? { guard ABPersonHasImageData(abRecord) else { return nil } guard let data = ABPersonCopyImageData(abRecord)?.takeRetainedValue() else { return nil } - return UIImage(data: data as Data) + return data as Data } private func extractProperty(_ propertyName: ABPropertyID) -> T? { @@ -316,7 +316,7 @@ class ContactStoreAdapter: ContactStoreAdaptee { } @objc protocol SystemContactsFetcherDelegate: class { - func systemContactsFetcher(_ systemContactsFetcher: SystemContactsFetcher, updatedContacts contacts: [Contact]) + func systemContactsFetcher(_ systemContactsFetcher: SystemContactsFetcher, updatedContacts contacts: [Contact], isUserRequested: Bool) } @objc @@ -361,7 +361,7 @@ class SystemContactsFetcher: NSObject { hasSetupObservation = true self.contactStoreAdapter.startObservingChanges { [weak self] in DispatchQueue.main.async { - self?.updateContacts(completion: nil, alwaysNotify: false) + self?.updateContacts(completion: nil, isUserRequested: false) } } } @@ -431,19 +431,20 @@ class SystemContactsFetcher: NSObject { return } - updateContacts(completion: nil, alwaysNotify: false) + updateContacts(completion: nil, isUserRequested: false) } - public func fetchIfAlreadyAuthorizedAndAlwaysNotify() { + public func userRequestedRefresh(completion: @escaping (Error?) -> Void) { AssertIsOnMainThread() guard authorizationStatus == .authorized else { + owsFail("should have already requested contact access") return } - updateContacts(completion: nil, alwaysNotify: true) + updateContacts(completion: completion, isUserRequested: true) } - private func updateContacts(completion completionParam: ((Error?) -> Void)?, alwaysNotify: Bool = false) { + private func updateContacts(completion completionParam: ((Error?) -> Void)?, isUserRequested: Bool = false) { AssertIsOnMainThread() // Ensure completion is invoked on main thread. @@ -483,8 +484,8 @@ class SystemContactsFetcher: NSObject { if self.lastContactUpdateHash != contactsHash { Logger.info("\(self.TAG) contact hash changed. new contactsHash: \(contactsHash)") shouldNotifyDelegate = true - } else if alwaysNotify { - Logger.info("\(self.TAG) ignoring debounce.") + } else if isUserRequested { + Logger.info("\(self.TAG) ignoring debounce due to user request") shouldNotifyDelegate = true } else { @@ -516,7 +517,7 @@ class SystemContactsFetcher: NSObject { self.lastDelegateNotificationDate = Date() self.lastContactUpdateHash = contactsHash - self.delegate?.systemContactsFetcher(self, updatedContacts: contacts) + self.delegate?.systemContactsFetcher(self, updatedContacts: contacts, isUserRequested: isUserRequested) completion(nil) } } diff --git a/SignalServiceKit/src/Contacts/Contact.h b/SignalServiceKit/src/Contacts/Contact.h index 94fbf42b4..8c7729aec 100644 --- a/SignalServiceKit/src/Contacts/Contact.h +++ b/SignalServiceKit/src/Contacts/Contact.h @@ -44,11 +44,11 @@ NS_ASSUME_NONNULL_BEGIN #if TARGET_OS_IOS -- (instancetype)initWithContactWithFirstName:(nullable NSString *)firstName - andLastName:(nullable NSString *)lastName - andUserTextPhoneNumbers:(NSArray *)phoneNumbers - andImage:(nullable UIImage *)image - andContactID:(ABRecordID)record; +- (instancetype)initWithFirstName:(nullable NSString *)firstName + lastName:(nullable NSString *)lastName + userTextPhoneNumbers:(NSArray *)phoneNumbers + imageData:(nullable NSData *)imageData + contactID:(ABRecordID)record; - (instancetype)initWithSystemContact:(CNContact *)contact; diff --git a/SignalServiceKit/src/Contacts/Contact.m b/SignalServiceKit/src/Contacts/Contact.m index e7f5e9837..c0c291c16 100644 --- a/SignalServiceKit/src/Contacts/Contact.m +++ b/SignalServiceKit/src/Contacts/Contact.m @@ -16,6 +16,7 @@ NS_ASSUME_NONNULL_BEGIN @interface Contact () @property (readonly, nonatomic) NSMutableDictionary *phoneNumberNameMap; +@property (readonly, nonatomic) NSData *imageData; @end @@ -26,13 +27,14 @@ NS_ASSUME_NONNULL_BEGIN @synthesize fullName = _fullName; @synthesize comparableNameFirstLast = _comparableNameFirstLast; @synthesize comparableNameLastFirst = _comparableNameLastFirst; +@synthesize image = _image; #if TARGET_OS_IOS -- (instancetype)initWithContactWithFirstName:(nullable NSString *)firstName - andLastName:(nullable NSString *)lastName - andUserTextPhoneNumbers:(NSArray *)phoneNumbers - andImage:(nullable UIImage *)image - andContactID:(ABRecordID)record +- (instancetype)initWithFirstName:(nullable NSString *)firstName + lastName:(nullable NSString *)lastName + userTextPhoneNumbers:(NSArray *)phoneNumbers + imageData:(nullable NSData *)imageData + contactID:(ABRecordID)record { self = [super init]; if (!self) { @@ -46,7 +48,7 @@ NS_ASSUME_NONNULL_BEGIN _userTextPhoneNumbers = phoneNumbers; _phoneNumberNameMap = [NSMutableDictionary new]; _parsedPhoneNumbers = [self parsedPhoneNumbersFromUserTextPhoneNumbers:phoneNumbers phoneNumberNameMap:@{}]; - _image = image; + _imageData = imageData; // Not using emails for old AB style contacts. _emails = [NSMutableArray new]; @@ -127,12 +129,26 @@ NS_ASSUME_NONNULL_BEGIN _emails = [emailAddresses copy]; if (contact.thumbnailImageData) { - _image = [UIImage imageWithData:contact.thumbnailImageData]; + _imageData = contact.thumbnailImageData; } return self; } +- (nullable UIImage *)image +{ + if (_image) { + return _image; + } + + if (!self.imageData) { + return nil; + } + + _image = [UIImage imageWithData:self.imageData]; + return _image; +} + - (NSString *)trimName:(NSString *)name { return [name stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]]; @@ -143,6 +159,15 @@ NS_ASSUME_NONNULL_BEGIN return [NSString stringWithFormat:@"ABRecordId:%d", recordId]; } ++ (MTLPropertyStorage)storageBehaviorForPropertyWithKey:(NSString *)propertyKey +{ + if ([propertyKey isEqualToString:@"cnContact"] || [propertyKey isEqualToString:@"image"]) { + return MTLPropertyStorageTransitory; + } else { + return [super storageBehaviorForPropertyWithKey:propertyKey]; + } +} + #endif // TARGET_OS_IOS - (NSArray *)parsedPhoneNumbersFromUserTextPhoneNumbers:(NSArray *)userTextPhoneNumbers diff --git a/SignalServiceKit/src/Contacts/PhoneNumber.m b/SignalServiceKit/src/Contacts/PhoneNumber.m index b26b6848a..dab6785a6 100644 --- a/SignalServiceKit/src/Contacts/PhoneNumber.m +++ b/SignalServiceKit/src/Contacts/PhoneNumber.m @@ -377,4 +377,14 @@ static NSString *const RPDefaultsKeyPhoneNumberCanonical = @"RPDefaultsKeyPhoneN return [self.toE164 compare:other.toE164]; } +- (BOOL)isEqual:(id)other +{ + if (![other isMemberOfClass:[self class]]) { + return NO; + } + PhoneNumber *otherPhoneNumber = (PhoneNumber *)other; + + return [self.phoneNumber isEqual:otherPhoneNumber.phoneNumber]; +} + @end