From 1260e7459d65e6ce13c31a7fecb6724bee65820b Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Thu, 20 Dec 2018 16:45:50 -0500 Subject: [PATCH 1/2] Add asserts around attachment crash. --- .../Messages/Attachments/TSAttachmentPointer.m | 17 +++++++++++++++++ .../Messages/Attachments/TSAttachmentStream.m | 7 ++++--- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/SignalServiceKit/src/Messages/Attachments/TSAttachmentPointer.m b/SignalServiceKit/src/Messages/Attachments/TSAttachmentPointer.m index 2d9927309..bcf7aef20 100644 --- a/SignalServiceKit/src/Messages/Attachments/TSAttachmentPointer.m +++ b/SignalServiceKit/src/Messages/Attachments/TSAttachmentPointer.m @@ -8,6 +8,7 @@ #import #import #import +#import NS_ASSUME_NONNULL_BEGIN @@ -209,6 +210,22 @@ NS_ASSUME_NONNULL_BEGIN }]; } +- (void)saveWithTransaction:(YapDatabaseReadWriteTransaction *)transaction +{ +#ifdef DEBUG + if (self.uniqueId.length > 0) { + id _Nullable oldObject = [transaction objectForKey:self.uniqueId inCollection:TSAttachment.collection]; + if ([oldObject isKindOfClass:[TSAttachmentStream class]]) { + OWSFailDebug(@"We should never overwrite a TSAttachmentStream with a TSAttachmentPointer."); + } + } else { + OWSFailDebug(@"Missing uniqueId."); + } +#endif + + [super saveWithTransaction:transaction]; +} + @end NS_ASSUME_NONNULL_END diff --git a/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m b/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m index e3b393dad..41cbf3047 100644 --- a/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m +++ b/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m @@ -524,10 +524,11 @@ typedef void (^OWSLoadedThumbnailSuccess)(OWSLoadedThumbnail *loadedThumbnail); self.cachedImageHeight = @(imageSize.height); [self.dbReadWriteConnection asyncReadWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { - - NSString *collection = [[self class] collection]; + NSString *collection = [TSAttachmentStream collection]; TSAttachmentStream *latestInstance = [transaction objectForKey:self.uniqueId inCollection:collection]; - if (latestInstance) { + if (![latestInstance isKindOfClass:[TSAttachmentStream class]]) { + OWSFailDebug(@"Attachment has unexpected type: %@", latestInstance.class); + } else if (latestInstance) { latestInstance.cachedImageWidth = @(imageSize.width); latestInstance.cachedImageHeight = @(imageSize.height); [latestInstance saveWithTransaction:transaction]; From dc6dadad432d53c43635d94a926d0bb2442b99ca Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Fri, 21 Dec 2018 12:44:51 -0500 Subject: [PATCH 2/2] Respond to CR. --- .../Messages/Attachments/TSAttachmentStream.m | 76 +++++++++---------- .../src/Storage/TSYapDatabaseObject.h | 2 +- .../src/Storage/TSYapDatabaseObject.m | 2 +- 3 files changed, 39 insertions(+), 41 deletions(-) diff --git a/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m b/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m index 41cbf3047..92700a3cc 100644 --- a/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m +++ b/SignalServiceKit/src/Messages/Attachments/TSAttachmentStream.m @@ -368,11 +368,8 @@ typedef void (^OWSLoadedThumbnailSuccess)(OWSLoadedThumbnail *loadedThumbnail); } if (didUpdateCache) { - [self.dbReadWriteConnection asyncReadWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { - [self applyChangeToSelfAndLatestCopy:transaction - changeBlock:^(TSAttachmentStream *attachmentStream) { - attachmentStream.isValidImageCached = @(result); - }]; + [self applyChangeAsyncToLatestCopyWithChangeBlock:^(TSAttachmentStream *latestInstance) { + latestInstance.isValidImageCached = @(result); }]; } @@ -395,11 +392,8 @@ typedef void (^OWSLoadedThumbnailSuccess)(OWSLoadedThumbnail *loadedThumbnail); } if (didUpdateCache) { - [self.dbReadWriteConnection asyncReadWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { - [self applyChangeToSelfAndLatestCopy:transaction - changeBlock:^(TSAttachmentStream *attachmentStream) { - attachmentStream.isValidVideoCached = @(result); - }]; + [self applyChangeAsyncToLatestCopyWithChangeBlock:^(TSAttachmentStream *latestInstance) { + latestInstance.isValidVideoCached = @(result); }]; } @@ -523,29 +517,43 @@ typedef void (^OWSLoadedThumbnailSuccess)(OWSLoadedThumbnail *loadedThumbnail); self.cachedImageWidth = @(imageSize.width); self.cachedImageHeight = @(imageSize.height); - [self.dbReadWriteConnection asyncReadWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { - NSString *collection = [TSAttachmentStream collection]; - TSAttachmentStream *latestInstance = [transaction objectForKey:self.uniqueId inCollection:collection]; - if (![latestInstance isKindOfClass:[TSAttachmentStream class]]) { - OWSFailDebug(@"Attachment has unexpected type: %@", latestInstance.class); - } else if (latestInstance) { - latestInstance.cachedImageWidth = @(imageSize.width); - latestInstance.cachedImageHeight = @(imageSize.height); - [latestInstance saveWithTransaction:transaction]; - } else { - // This message has not yet been saved or has been deleted; do nothing. - // This isn't an error per se, but these race conditions should be - // _very_ rare. - // - // An exception is incoming group avatar updates which we don't ever save. - OWSLogWarn(@"Attachment not yet saved."); - } + [self applyChangeAsyncToLatestCopyWithChangeBlock:^(TSAttachmentStream *latestInstance) { + latestInstance.cachedImageWidth = @(imageSize.width); + latestInstance.cachedImageHeight = @(imageSize.height); }]; return imageSize; } } +#pragma mark - Update With... + +- (void)applyChangeAsyncToLatestCopyWithChangeBlock:(void (^)(TSAttachmentStream *))changeBlock +{ + OWSAssertDebug(changeBlock); + + [self.dbReadWriteConnection asyncReadWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { + NSString *collection = [TSAttachmentStream collection]; + TSAttachmentStream *latestInstance = [transaction objectForKey:self.uniqueId inCollection:collection]; + if (!latestInstance) { + // This attachment has either not yet been saved or has been deleted; do nothing. + // This isn't an error per se, but these race conditions should be + // _very_ rare. + // + // An exception is incoming group avatar updates which we don't ever save. + OWSLogWarn(@"Attachment not yet saved."); + } else if (![latestInstance isKindOfClass:[TSAttachmentStream class]]) { + OWSFailDebug(@"Attachment has unexpected type: %@", latestInstance.class); + } else { + changeBlock(latestInstance); + + [latestInstance saveWithTransaction:transaction]; + } + }]; +} + +#pragma mark - + - (CGFloat)calculateAudioDurationSeconds { OWSAssertIsOnMainThread(); @@ -577,18 +585,8 @@ typedef void (^OWSLoadedThumbnailSuccess)(OWSLoadedThumbnail *loadedThumbnail); CGFloat audioDurationSeconds = [self calculateAudioDurationSeconds]; self.cachedAudioDurationSeconds = @(audioDurationSeconds); - [self.dbReadWriteConnection asyncReadWriteWithBlock:^(YapDatabaseReadWriteTransaction *transaction) { - NSString *collection = [[self class] collection]; - TSAttachmentStream *latestInstance = [transaction objectForKey:self.uniqueId inCollection:collection]; - if (latestInstance) { - latestInstance.cachedAudioDurationSeconds = @(audioDurationSeconds); - [latestInstance saveWithTransaction:transaction]; - } else { - // This message has not yet been saved or has been deleted; do nothing. - // This isn't an error per se, but these race conditions should be - // _very_ rare. - OWSFailDebug(@"Attachment not yet saved."); - } + [self applyChangeAsyncToLatestCopyWithChangeBlock:^(TSAttachmentStream *latestInstance) { + latestInstance.cachedAudioDurationSeconds = @(audioDurationSeconds); }]; return audioDurationSeconds; diff --git a/SignalServiceKit/src/Storage/TSYapDatabaseObject.h b/SignalServiceKit/src/Storage/TSYapDatabaseObject.h index 62864f7ca..cb95b2ee4 100644 --- a/SignalServiceKit/src/Storage/TSYapDatabaseObject.h +++ b/SignalServiceKit/src/Storage/TSYapDatabaseObject.h @@ -132,7 +132,7 @@ NS_ASSUME_NONNULL_BEGIN - (void)removeWithTransaction:(YapDatabaseReadWriteTransaction *)transaction; - (void)remove; -#pragma mark Update With... +#pragma mark - Update With... // This method is used by "updateWith..." methods. // diff --git a/SignalServiceKit/src/Storage/TSYapDatabaseObject.m b/SignalServiceKit/src/Storage/TSYapDatabaseObject.m index 488607ab3..9d7d300b2 100644 --- a/SignalServiceKit/src/Storage/TSYapDatabaseObject.m +++ b/SignalServiceKit/src/Storage/TSYapDatabaseObject.m @@ -201,7 +201,7 @@ NS_ASSUME_NONNULL_BEGIN return object; } -#pragma mark Update With... +#pragma mark - Update With... - (void)applyChangeToSelfAndLatestCopy:(YapDatabaseReadWriteTransaction *)transaction changeBlock:(void (^)(id))changeBlock