diff --git a/Signal/src/ViewControllers/NewContactThreadViewController.m b/Signal/src/ViewControllers/NewContactThreadViewController.m index 3b825598f..3f63e48d4 100644 --- a/Signal/src/ViewControllers/NewContactThreadViewController.m +++ b/Signal/src/ViewControllers/NewContactThreadViewController.m @@ -149,9 +149,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/SignalMessaging/contacts/OWSContactsManager.h b/SignalMessaging/contacts/OWSContactsManager.h index ba6d9fad1..53c3c34b5 100644 --- a/SignalMessaging/contacts/OWSContactsManager.h +++ b/SignalMessaging/contacts/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/SignalMessaging/contacts/OWSContactsManager.m b/SignalMessaging/contacts/OWSContactsManager.m index 5b88f6609..cd327c0aa 100644 --- a/SignalMessaging/contacts/OWSContactsManager.m +++ b/SignalMessaging/contacts/OWSContactsManager.m @@ -69,18 +69,31 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification { __block NSMutableArray<SignalAccount *> *signalAccounts; [self.dbReadConnection readWithBlock:^(YapDatabaseReadTransaction *_Nonnull transaction) { - signalAccounts = [[NSMutableArray alloc] - initWithCapacity:[SignalAccount numberOfKeysInCollectionWithTransaction:transaction]]; + NSUInteger signalAccountCount = [SignalAccount numberOfKeysInCollectionWithTransaction:transaction]; + DDLogInfo(@"%@ loading %lu signal accounts from cache.", self.logTag, (unsigned long)signalAccountCount); - [SignalAccount enumerateCollectionObjectsWithTransaction:transaction - usingBlock:^(SignalAccount *signalAccount, BOOL *_Nonnull stop) { - [signalAccounts addObject:signalAccount]; - }]; + 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. @@ -99,9 +112,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 @@ -123,27 +136,30 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification - (void)systemContactsFetcher:(SystemContactsFetcher *)systemsContactsFetcher updatedContacts:(NSArray<Contact *> *)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; } @@ -154,7 +170,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 @@ -180,10 +196,9 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification [self.avatarCache removeAllImagesForKey:recipientId]; } -- (void)updateWithContacts:(NSArray<Contact *> *)contacts +- (void)updateWithContacts:(NSArray<Contact *> *)contacts shouldClearStaleCache:(BOOL)shouldClearStaleCache { - dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ - + dispatch_async(self.serialQueue, ^{ NSMutableDictionary<NSString *, Contact *> *allContactsMap = [NSMutableDictionary new]; for (Contact *contact in contacts) { for (PhoneNumber *phoneNumber in contact.parsedPhoneNumbers) { @@ -200,17 +215,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<NSString *, SignalAccount *> *signalAccountMap = [NSMutableDictionary new]; + dispatch_async(self.serialQueue, ^{ NSMutableArray<SignalAccount *> *signalAccounts = [NSMutableArray new]; NSArray<Contact *> *contacts = self.allContacts; @@ -225,10 +239,16 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification } }]; + NSMutableSet<NSString *> *seenRecipientIds = [NSMutableSet new]; for (Contact *contact in contacts) { NSArray<SignalRecipient *> *signalRecipients = contactIdToSignalRecipientsMap[contact.uniqueId]; - for (SignalRecipient *signalRecipient in - [signalRecipients sortedArrayUsingSelector:@selector((compare:))]) { + 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) { @@ -236,33 +256,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<NSString *, SignalAccount *> *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<NSString *> *allKeys = [transaction allKeysInCollection:[SignalAccount collection]]; - NSMutableSet<NSString *> *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]; }); @@ -273,6 +341,11 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification { AssertIsOnMainThread(); + if ([signalAccounts isEqual:self.signalAccounts]) { + DDLogDebug(@"%@ SignalAccounts unchanged.", self.logTag); + return; + } + NSMutableDictionary<NSString *, SignalAccount *> *signalAccountMap = [NSMutableDictionary new]; for (SignalAccount *signalAccount in signalAccounts) { signalAccountMap[signalAccount.recipientId] = signalAccount; @@ -630,6 +703,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/SignalMessaging/contacts/SystemContactsFetcher.swift b/SignalMessaging/contacts/SystemContactsFetcher.swift index a9d945dc9..0ed13a0d3 100644 --- a/SignalMessaging/contacts/SystemContactsFetcher.swift +++ b/SignalMessaging/contacts/SystemContactsFetcher.swift @@ -186,11 +186,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) } } @@ -244,14 +244,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<T>(_ propertyName: ABPropertyID) -> T? { @@ -317,7 +317,7 @@ class ContactStoreAdapter: ContactStoreAdaptee { } @objc public protocol SystemContactsFetcherDelegate: class { - func systemContactsFetcher(_ systemContactsFetcher: SystemContactsFetcher, updatedContacts contacts: [Contact]) + func systemContactsFetcher(_ systemContactsFetcher: SystemContactsFetcher, updatedContacts contacts: [Contact], isUserRequested: Bool) } @objc @@ -370,7 +370,7 @@ public 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) } } } @@ -444,20 +444,21 @@ public class SystemContactsFetcher: NSObject { return } - updateContacts(completion: nil, alwaysNotify: false) + updateContacts(completion: nil, isUserRequested: false) } @objc - 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. @@ -497,8 +498,8 @@ public 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 { @@ -530,7 +531,7 @@ public 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 2a1c2e32d..449f7f0c0 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<NSString *> *)phoneNumbers - andImage:(nullable UIImage *)image - andContactID:(ABRecordID)record; +- (instancetype)initWithFirstName:(nullable NSString *)firstName + lastName:(nullable NSString *)lastName + userTextPhoneNumbers:(NSArray<NSString *> *)phoneNumbers + imageData:(nullable NSData *)imageData + contactID:(ABRecordID)record; - (instancetype)initWithSystemContact:(CNContact *)contact NS_AVAILABLE_IOS(9_0); 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<NSString *, NSString *> *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<NSString *> *)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<PhoneNumber *> *)parsedPhoneNumbersFromUserTextPhoneNumbers:(NSArray<NSString *> *)userTextPhoneNumbers diff --git a/SignalServiceKit/src/Contacts/PhoneNumber.m b/SignalServiceKit/src/Contacts/PhoneNumber.m index 98059a918..ce78fdd19 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