From 4a1c53f6241e1fc5ebd35e87bdde957eec3ecfa7 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Thu, 16 Jun 2016 13:54:33 -0700 Subject: [PATCH] Access contacts atomically to avoid corruption (PR #1223) 4% of our crashes are from accessing latestContactsById with an invalid address. This was partially addressed by assigning a value on init, but it's still happening. By converting the ivar to an atomic property we can avoid any funny business that would happen when accessing this var while it's being updated in a different thread. Also, make sure we're copying to defensively strip mutability. Also: * bumped release target * removed unused code * dealloc at the top per style. --- Signal/Signal-Info.plist | 4 +- Signal/src/contact/ContactsManager.h | 13 +----- Signal/src/contact/ContactsManager.m | 68 ++++++++++------------------ 3 files changed, 27 insertions(+), 58 deletions(-) diff --git a/Signal/Signal-Info.plist b/Signal/Signal-Info.plist index a0614c797..6987d4d10 100644 --- a/Signal/Signal-Info.plist +++ b/Signal/Signal-Info.plist @@ -21,7 +21,7 @@ CFBundlePackageType APPL CFBundleShortVersionString - 2.3.2 + 2.3.3 CFBundleSignature ???? CFBundleURLTypes @@ -38,7 +38,7 @@ CFBundleVersion - 2.3.2.1 + 2.3.3.0 ITSAppUsesNonExemptEncryption LOGS_EMAIL diff --git a/Signal/src/contact/ContactsManager.h b/Signal/src/contact/ContactsManager.h index 87f489e2b..b877297e3 100644 --- a/Signal/src/contact/ContactsManager.h +++ b/Signal/src/contact/ContactsManager.h @@ -21,18 +21,7 @@ typedef void (^ABAccessRequestCompletionBlock)(BOOL hasAccess); typedef void (^ABReloadRequestCompletionBlock)(NSArray *contacts); -@interface ContactsManager : NSObject { - @private - TOCFuture *futureAddressBook; - @private - ObservableValueController *observableContactsController; - @private - TOCCancelTokenSource *life; - @private - NSDictionary *latestContactsById; - @private - NSDictionary *latestWhisperUsersById; -} +@interface ContactsManager : NSObject @property CNContactStore *contactStore; diff --git a/Signal/src/contact/ContactsManager.m b/Signal/src/contact/ContactsManager.m index 7b17bb007..af1b957b9 100644 --- a/Signal/src/contact/ContactsManager.m +++ b/Signal/src/contact/ContactsManager.m @@ -7,20 +7,28 @@ typedef BOOL (^ContactSearchBlock)(id, NSUInteger, BOOL *); -@interface ContactsManager () { - id addressBookReference; -} +@interface ContactsManager () + +@property id addressBookReference; +@property TOCFuture *futureAddressBook; +@property ObservableValueController *observableContactsController; +@property TOCCancelTokenSource *life; +@property(atomic, copy) NSDictionary *latestContactsById; @end @implementation ContactsManager +- (void)dealloc { + [_life cancel]; +} + - (id)init { self = [super init]; if (self) { - life = [TOCCancelTokenSource new]; - observableContactsController = [ObservableValueController observableValueControllerWithInitialValue:nil]; - latestContactsById = @{}; + _life = [TOCCancelTokenSource new]; + _observableContactsController = [ObservableValueController observableValueControllerWithInitialValue:nil]; + _latestContactsById = @{}; } return self; } @@ -39,20 +47,16 @@ typedef BOOL (^ContactSearchBlock)(id, NSUInteger, BOOL *); [self setupAddressBook]; - [observableContactsController watchLatestValueOnArbitraryThread:^(NSArray *latestContacts) { + [self.observableContactsController watchLatestValueOnArbitraryThread:^(NSArray *latestContacts) { @synchronized(self) { [self setupLatestContacts:latestContacts]; } } - untilCancelled:life.token]; -} - -- (void)dealloc { - [life cancel]; + untilCancelled:_life.token]; } - (void)verifyABPermission { - if (!addressBookReference) { + if (!self.addressBookReference) { [self setupAddressBook]; } } @@ -73,7 +77,7 @@ void onAddressBookChanged(ABAddressBookRef notifyAddressBook, CFDictionaryRef in - (void)setupAddressBook { dispatch_async(ADDRESSBOOK_QUEUE, ^{ [[ContactsManager asyncGetAddressBook] thenDo:^(id addressBook) { - addressBookReference = addressBook; + self.addressBookReference = addressBook; ABAddressBookRef cfAddressBook = (__bridge ABAddressBookRef)addressBook; ABAddressBookRegisterExternalChangeCallback(cfAddressBook, onAddressBookChanged, (__bridge void *)self); dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), ^{ @@ -106,12 +110,12 @@ void onAddressBookChanged(ABAddressBookRef notifyAddressBook, CFDictionaryRef in [ContactsManager blockingContactDialog]; } }); - [observableContactsController updateValue:[self getContactsFromAddressBook:addressBookRef]]; + [self.observableContactsController updateValue:[self getContactsFromAddressBook:addressBookRef]]; } - (void)setupLatestContacts:(NSArray *)contacts { if (contacts) { - latestContactsById = [ContactsManager keyContactsById:contacts]; + self.latestContactsById = [ContactsManager keyContactsById:contacts]; } } @@ -174,16 +178,10 @@ void onAddressBookChanged(ABAddressBookRef notifyAddressBook, CFDictionaryRef in } } -- (void)setupLatestRedPhoneUsers:(NSArray *)users { - if (users) { - latestWhisperUsersById = [ContactsManager keyContactsById:users]; - } -} - #pragma mark - Observables - (ObservableValue *)getObservableContacts { - return observableContactsController; + return self.observableContactsController; } #pragma mark - Address Book utils @@ -251,7 +249,7 @@ void onAddressBookChanged(ABAddressBookRef notifyAddressBook, CFDictionaryRef in } - (NSArray *)latestContactsWithSearchString:(NSString *)searchString { - return [latestContactsById.allValues filter:^int(Contact *contact) { + return [self.latestContactsById.allValues filter:^int(Contact *contact) { return searchString.length == 0 || [ContactsManager name:contact.fullName matchesQuery:searchString]; }]; } @@ -395,17 +393,11 @@ void onAddressBookChanged(ABAddressBookRef notifyAddressBook, CFDictionaryRef in }]; } -- (Contact *)latestContactWithRecordId:(ABRecordID)recordId { - @synchronized(self) { - return latestContactsById[@(recordId)]; - } -} - - (NSArray *)allContacts { NSMutableArray *allContacts = [NSMutableArray array]; - for (NSString *key in latestContactsById.allKeys) { - Contact *contact = [latestContactsById objectForKey:key]; + for (NSString *key in self.latestContactsById.allKeys) { + Contact *contact = [self.latestContactsById objectForKey:key]; if ([contact isKindOfClass:[Contact class]]) { [allContacts addObject:contact]; @@ -445,18 +437,6 @@ void onAddressBookChanged(ABAddressBookRef notifyAddressBook, CFDictionaryRef in return [searchString rangeOfString:queryString options:searchOpts].location != NSNotFound; } -- (NSArray *)contactsForContactIds:(NSArray *)contactIds { - NSMutableArray *contacts = [NSMutableArray array]; - for (NSNumber *favouriteId in contactIds) { - Contact *contact = [self latestContactWithRecordId:favouriteId.intValue]; - - if (contact) { - [contacts addObject:contact]; - } - } - return [contacts copy]; -} - #pragma mark - Whisper User Management - (NSArray *)getSignalUsersFromContactsArray:(NSArray *)contacts {