Only clear cache when user pulls-to-refresh

// FREEBIE

Signal/src/ViewControllers/NewContactThreadViewController.m

@@ -148,7 +148,7 @@ NS_ASSUME_NONNULL_BEGIN
     OWSAssert([NSThread isMainThread]);
 
     [self.contactsViewHelper.contactsManager
-        fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotifyWithCompletion:^(NSError *_Nullable error) {
+        userRequestedSystemContactsRefreshWithCompletion:^(NSError *_Nullable error) {
             if (error) {
                 DDLogError(@"%@ refreshing contacts failed with error: %@", self.logTag, error);
             }

Signal/src/contact/OWSContactsManager.h

@@ -54,9 +54,9 @@ extern NSString *const OWSContactsManagerSignalAccountsDidChangeNotification;
 - (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)fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotifyWithCompletion:
-    (void (^)(NSError *_Nullable error))completionHandler;
+// 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 SignalAccount's
+- (void)userRequestedSystemContactsRefreshWithCompletion:(void (^)(NSError *_Nullable error))completionHandler;
 
 #pragma mark - Util
 

Signal/src/contact/OWSContactsManager.m

@@ -77,6 +77,17 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification
     [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.
@@ -95,10 +106,9 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification
     [self.systemContactsFetcher fetchOnceIfAlreadyAuthorized];
 }
 
-- (void)fetchSystemContactsIfAlreadyAuthorizedAndAlwaysNotifyWithCompletion:
-    (void (^)(NSError *_Nullable error))completionHandler
+- (void)userRequestedSystemContactsRefreshWithCompletion:(void (^)(NSError *_Nullable error))completionHandler
 {
-    [self.systemContactsFetcher fetchIfAlreadyAuthorizedAndAlwaysNotifyWithCompletion:completionHandler];
+    [self.systemContactsFetcher userRequestedRefreshWithCompletion:completionHandler];
 }
 
 - (BOOL)isSystemContactsAuthorized
@@ -120,8 +130,9 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification
 
 - (void)systemContactsFetcher:(SystemContactsFetcher *)systemsContactsFetcher
               updatedContacts:(NSArray<Contact *> *)contacts
+                userRequested:(BOOL)userRequested
 {
-    [self updateWithContacts:contacts];
+    [self updateWithContacts:contacts clearStaleCache:userRequested];
 }
 
 #pragma mark - Intersection
@@ -179,10 +190,9 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification
     [self.avatarCache removeAllImagesForKey:recipientId];
 }
 
-- (void)updateWithContacts:(NSArray<Contact *> *)contacts
+- (void)updateWithContacts:(NSArray<Contact *> *)contacts clearStaleCache:(BOOL)clearStaleCache
 {
-    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,22 +210,15 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification
             [self.avatarCache removeAllImages];
 
             [self intersectContactsWithCompletion:^(NSError *_Nullable error) {
-                [self buildSignalAccounts];
+                [self buildSignalAccountsAndClearStaleCache:clearStaleCache];
             }];
         });
     });
 }
 
-- (void)buildSignalAccounts
+- (void)buildSignalAccountsAndClearStaleCache:(BOOL)clearStaleCache;
 {
-    // Ensure we're not running concurrently since one invocation could affect the other.
-    static dispatch_queue_t _serialQueue;
-    static dispatch_once_t onceToken;
-    dispatch_once(&onceToken, ^{
-        _serialQueue = dispatch_queue_create("org.whispersystems.contacts.buildSignalAccount", DISPATCH_QUEUE_SERIAL);
-    });
-
-    dispatch_async(_serialQueue, ^{
+    dispatch_async(self.serialQueue, ^{
         NSMutableArray<SignalAccount *> *signalAccounts = [NSMutableArray new];
         NSArray<Contact *> *contacts = self.allContacts;
 
@@ -291,16 +294,43 @@ NSString *const OWSContactsManagerSignalAccountsDidChangeNotification
 
         // Update cached SignalAccounts on disk
         [self.dbWriteConnection readWriteWithBlock:^(YapDatabaseReadWriteTransaction *_Nonnull transaction) {
-            DDLogInfo(@"%@ Saving %lu new SignalAccounts", self.logTag, (unsigned long)accountsToSave.count);
+            DDLogInfo(@"%@ Saving %lu SignalAccounts", self.logTag, (unsigned long)accountsToSave.count);
             for (SignalAccount *signalAccount in accountsToSave) {
-                DDLogVerbose(@"%@ Adding new SignalAccount: %@", self.logTag, signalAccount);
+                DDLogVerbose(@"%@ Saving SignalAccount: %@", self.logTag, signalAccount);
                 [signalAccount saveWithTransaction:transaction];
             }
 
-            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];
+            if (clearStaleCache) {
+                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:^NSComparisonResult(SignalAccount *left, SignalAccount *right) {
+                        NSString *leftName = [self comparableNameForSignalAccount:left];
+                        NSString *rightName = [self comparableNameForSignalAccount:right];
+
+                        return [leftName compare:rightName];
+                    }];
+                }
             }
         }];
 

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<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], userRequested: 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, userRequested: false)
             }
         }
     }
@@ -431,19 +431,20 @@ class SystemContactsFetcher: NSObject {
             return
         }
 
-        updateContacts(completion: nil, alwaysNotify: false)
+        updateContacts(completion: nil, userRequested: false)
     }
 
-    public func fetchIfAlreadyAuthorizedAndAlwaysNotify(completion: ((Error?) -> Void)?) {
+    public func userRequestedRefresh(completion: @escaping (Error?) -> Void) {
         AssertIsOnMainThread()
         guard authorizationStatus == .authorized else {
+            owsFail("should have already requested contact access")
             return
         }
 
-        updateContacts(completion: completion, alwaysNotify: true)
+        updateContacts(completion: completion, userRequested: true)
     }
 
-    private func updateContacts(completion completionParam: ((Error?) -> Void)?, alwaysNotify: Bool = false) {
+    private func updateContacts(completion completionParam: ((Error?) -> Void)?, userRequested: 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 userRequested {
+                    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, userRequested: userRequested)
                 completion(nil)
             }
         }

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;
 

SignalServiceKit/src/Contacts/Contact.m

@@ -27,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) {
@@ -47,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];
 
@@ -128,27 +129,24 @@ NS_ASSUME_NONNULL_BEGIN
     _emails = [emailAddresses copy];
 
     if (contact.thumbnailImageData) {
-        _image = [UIImage imageWithData:contact.thumbnailImageData];
+        _imageData = contact.thumbnailImageData;
     }
 
     return self;
 }
 
-- (instancetype)initWithCoder:(NSCoder *)coder
+- (nullable UIImage *)image
 {
-    self = [super initWithCoder:coder];
-    if (_imageData) {
-        _image = [UIImage imageWithData:_imageData];
+    if (_image) {
+        return _image;
     }
-    return self;
-}
 
-- (void)encodeWithCoder:(NSCoder *)coder
-{
-    if (_image) {
-        _imageData = UIImagePNGRepresentation(_image);
+    if (!self.imageData) {
+        return nil;
     }
-    [super encodeWithCoder:coder];
+
+    _image = [UIImage imageWithData:self.imageData];
+    return _image;
 }
 
 - (NSString *)trimName:(NSString *)name