Merge branch 'mkirk/replace-cache-for-migration' into hotfix/2.19.3

Signal/src/ViewControllers/NewContactThreadViewController.m

@@ -147,9 +147,13 @@ NS_ASSUME_NONNULL_BEGIN
 {
     OWSAssert([NSThread isMainThread]);
 
-    [self.contactsViewHelper.contactsManager fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotify];
+    [self.contactsViewHelper.contactsManager
-
+        userRequestedSystemContactsRefreshWithCompletion:^(NSError *_Nullable error) {
-    [refreshControl endRefreshing];
+            if (error) {
+                DDLogError(@"%@ refreshing contacts failed with error: %@", self.logTag, error);
+            }
+            [refreshControl endRefreshing];
+        }];
 }
 
 - (void)showContactsPermissionReminder:(BOOL)isVisible

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.
+// but not prompt for contact access. Also, it will always notify delegates, even if
-- (void)fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotify;
+// contacts haven't changed, and will clear out any stale cached SignalAccounts
+- (void)userRequestedSystemContactsRefreshWithCompletion:(void (^)(NSError *_Nullable error))completionHandler;
 
 #pragma mark - Util
 

Signal/src/contact/OWSContactsManager.m

@@ -63,17 +63,32 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification
 - (void)loadSignalAccountsFromCache
 {
     __block NSMutableArray<SignalAccount *> *signalAccounts;
-    [self.dbReadConnection readWithBlock:^(YapDatabaseReadTransaction * _Nonnull transaction) {
+    [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);
+
+        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<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;
         }
 
@@ -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<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) {
@@ -193,17 +210,16 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification
 
             [self.avatarCache removeAllImages];
 
-            [self intersectContacts];
+            [self intersectContactsWithCompletion:^(NSError *_Nullable error) {
-
+                [self buildSignalAccountsAndClearStaleCache:shouldClearStaleCache];
-            [self buildSignalAccounts];
+            }];
         });
     });
 }
 
-- (void)buildSignalAccounts
+- (void)buildSignalAccountsAndClearStaleCache:(BOOL)shouldClearStaleCache;
 {
-    dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{
+    dispatch_async(self.serialQueue, ^{
-        NSMutableDictionary<NSString *, SignalAccount *> *signalAccountMap = [NSMutableDictionary new];
         NSMutableArray<SignalAccount *> *signalAccounts = [NSMutableArray new];
         NSArray<Contact *> *contacts = self.allContacts;
 
@@ -218,9 +234,15 @@ 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:)]) {
+                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<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]];
+            DDLogInfo(@"%@ Saving %lu SignalAccounts", self.logTag, (unsigned long)accountsToSave.count);
-            NSMutableSet<NSString *> *orphanedKeys = [NSMutableSet setWithArray:allKeys];
+            for (SignalAccount *signalAccount in accountsToSave) {
-
+                DDLogVerbose(@"%@ Saving SignalAccount: %@", self.logTag, signalAccount);
-            DDLogInfo(@"%@ Saving %lu SignalAccounts", self.logTag, signalAccounts.count);
-            for (SignalAccount *signalAccount in signalAccounts) {
-                // TODO only save the ones that changed
-                [orphanedKeys removeObject:signalAccount.uniqueId];
                 [signalAccount saveWithTransaction:transaction];
             }
-            
+
-            if (orphanedKeys.count > 0) {
+            if (shouldClearStaleCache) {
-                DDLogInfo(@"%@ Removing %lu orphaned SignalAccounts", self.logTag, (unsigned long)orphanedKeys.count);
+                DDLogInfo(@"%@ Removing %lu old SignalAccounts.", self.logTag, (unsigned long)oldSignalAccounts.count);
-                [transaction removeObjectsForKeys:orphanedKeys.allObjects inCollection:[SignalAccount collection]];
+                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<SignalAccount *> *)signalAccounts
 {
     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;
@@ -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;

Signal/src/contact/SystemContactsFetcher.swift

@@ -185,11 +185,11 @@ class AddressBookContactStoreAdaptee: ContactStoreAdaptee {
             }
         }
 
-        return Contact(contactWithFirstName: firstName,
+        return Contact(firstName: firstName,
-                       andLastName: lastName,
+                       lastName: lastName,
-                       andUserTextPhoneNumbers: phoneNumbers,
+                       userTextPhoneNumbers: phoneNumbers,
-                       andImage: addressBookRecord.image,
+                       imageData: addressBookRecord.imageData,
-                       andContactID: addressBookRecord.recordId)
+                       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<T>(_ 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 {
+                } else if isUserRequested {
-                    Logger.info("\(self.TAG) ignoring debounce.")
+                    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)
             }
         }

SignalServiceKit/src/Contacts/Contact.h

@@ -44,11 +44,11 @@ NS_ASSUME_NONNULL_BEGIN
 
 #if TARGET_OS_IOS
 
-- (instancetype)initWithContactWithFirstName:(nullable NSString *)firstName
+- (instancetype)initWithFirstName:(nullable NSString *)firstName
-                                 andLastName:(nullable NSString *)lastName
+                         lastName:(nullable NSString *)lastName
-                     andUserTextPhoneNumbers:(NSArray<NSString *> *)phoneNumbers
+             userTextPhoneNumbers:(NSArray<NSString *> *)phoneNumbers
-                                    andImage:(nullable UIImage *)image
+                        imageData:(nullable NSData *)imageData
-                                andContactID:(ABRecordID)record;
+                        contactID:(ABRecordID)record;
 
 - (instancetype)initWithSystemContact:(CNContact *)contact;
 

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
+- (instancetype)initWithFirstName:(nullable NSString *)firstName
-                                 andLastName:(nullable NSString *)lastName
+                         lastName:(nullable NSString *)lastName
-                     andUserTextPhoneNumbers:(NSArray *)phoneNumbers
+             userTextPhoneNumbers:(NSArray<NSString *> *)phoneNumbers
-                                    andImage:(nullable UIImage *)image
+                        imageData:(nullable NSData *)imageData
-                                andContactID:(ABRecordID)record
+                        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

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