From 88f343a0aa351c1c9427ef1cd2c9b079602ceafb Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Mon, 13 Mar 2017 18:33:54 -0300 Subject: [PATCH 1/2] Attempt to fix the "frequent attachment download errors with low server ids". // FREEBIE --- src/Devices/OWSReadReceiptsProcessor.m | 6 +- .../Attachments/OWSAttachmentsProcessor.m | 55 ++++++++++++++---- src/Messages/Attachments/TSAttachment.h | 22 ++++++++ src/Messages/Attachments/TSAttachment.m | 56 ++++++++++++++++++- .../Attachments/TSAttachmentPointer.m | 4 -- src/Messages/Attachments/TSAttachmentStream.h | 2 - src/Messages/Attachments/TSAttachmentStream.m | 9 ++- src/Network/WebSockets/TSSocketManager.m | 10 +++- 8 files changed, 136 insertions(+), 28 deletions(-) diff --git a/src/Devices/OWSReadReceiptsProcessor.m b/src/Devices/OWSReadReceiptsProcessor.m index ac671641a..a41bcc6d6 100644 --- a/src/Devices/OWSReadReceiptsProcessor.m +++ b/src/Devices/OWSReadReceiptsProcessor.m @@ -1,4 +1,6 @@ -// Copyright © 2016 Open Whisper Systems. All rights reserved. +// +// Copyright (c) 2017 Open Whisper Systems. All rights reserved. +// #import "OWSReadReceiptsProcessor.h" #import "OWSDisappearingMessagesJob.h" @@ -91,7 +93,7 @@ NSString *const OWSReadReceiptsProcessorMarkedMessageAsReadNotification = object:message]; } else { - DDLogDebug(@"%@ Received read receipt for an unkown message. Saving it for later.", self.tag); + DDLogDebug(@"%@ Received read receipt for an unknown message. Saving it for later.", self.tag); [readReceipt save]; } } diff --git a/src/Messages/Attachments/OWSAttachmentsProcessor.m b/src/Messages/Attachments/OWSAttachmentsProcessor.m index b0e75b914..3683e78d4 100644 --- a/src/Messages/Attachments/OWSAttachmentsProcessor.m +++ b/src/Messages/Attachments/OWSAttachmentsProcessor.m @@ -119,6 +119,9 @@ NS_ASSUME_NONNULL_BEGIN } }; + if (attachment.serverId < 100) { + DDLogError(@"%@ Suspicious attachment id: %llu", self.tag, (unsigned long long)attachment.serverId); + } TSAttachmentRequest *attachmentRequest = [[TSAttachmentRequest alloc] initWithId:attachment.serverId relay:attachment.relay]; [self.networkManager makeRequest:attachmentRequest @@ -139,18 +142,48 @@ NS_ASSUME_NONNULL_BEGIN dispatch_async([OWSDispatch attachmentsQueue], ^{ [self downloadFromLocation:location - pointer:attachment - success:^(NSData *_Nonnull encryptedData) { - [self decryptAttachmentData:encryptedData - pointer:attachment - success:markAndHandleSuccess - failure:markAndHandleFailure]; - } - failure:markAndHandleFailure]; + pointer:attachment + success:^(NSData *_Nonnull encryptedData) { + [self decryptAttachmentData:encryptedData + pointer:attachment + success:markAndHandleSuccess + failure:markAndHandleFailure]; + } + failure:^(NSURLSessionDataTask *_Nullable task, NSError *_Nonnull error) { + if (attachment.serverId < 100) { + // This looks like the symptom of the "frequent 404 + // downloading attachments with low server ids". + NSHTTPURLResponse *httpResponse = (NSHTTPURLResponse *)task.response; + NSInteger statusCode = [httpResponse statusCode]; + DDLogError(@"%@ %d Failure with suspicious attachment id: %llu, %@", + self.tag, + (int)statusCode, + (unsigned long long)attachment.serverId, + error); + [DDLog flushLog]; + OWSAssert(0); + } + if (markAndHandleFailure) { + markAndHandleFailure(error); + } + }]; }); } failure:^(NSURLSessionDataTask *task, NSError *error) { DDLogError(@"Failed retrieval of attachment with error: %@", error); + if (attachment.serverId < 100) { + // This _shouldn't_ be the symptom of the "frequent 404 + // downloading attachments with low server ids". + NSHTTPURLResponse *httpResponse = (NSHTTPURLResponse *)task.response; + NSInteger statusCode = [httpResponse statusCode]; + DDLogError(@"%@ %d Failure with suspicious attachment id: %llu, %@", + self.tag, + (int)statusCode, + (unsigned long long)attachment.serverId, + error); + [DDLog flushLog]; + OWSAssert(0); + } return markAndHandleFailure(error); }]; } @@ -184,7 +217,7 @@ NS_ASSUME_NONNULL_BEGIN - (void)downloadFromLocation:(NSString *)location pointer:(TSAttachmentPointer *)pointer success:(void (^)(NSData *encryptedData))successHandler - failure:(void (^)(NSError *error))failureHandler + failure:(void (^)(NSURLSessionDataTask *_Nullable task, NSError *_Nonnull error))failureHandler { AFHTTPSessionManager *manager = [AFHTTPSessionManager manager]; manager.requestSerializer = [AFHTTPRequestSerializer serializer]; @@ -200,13 +233,13 @@ NS_ASSUME_NONNULL_BEGIN if (![responseObject isKindOfClass:[NSData class]]) { DDLogError(@"%@ Failed retrieval of attachment. Response had unexpected format.", self.tag); NSError *error = OWSErrorMakeUnableToProcessServerResponseError(); - return failureHandler(error); + return failureHandler(task, error); } successHandler((NSData *)responseObject); } failure:^(NSURLSessionDataTask *_Nullable task, NSError *_Nonnull error) { DDLogError(@"Failed to retrieve attachment with error: %@", error.description); - return failureHandler(error); + return failureHandler(task, error); }]; } diff --git a/src/Messages/Attachments/TSAttachment.h b/src/Messages/Attachments/TSAttachment.h index 67f1dd274..150fa7199 100644 --- a/src/Messages/Attachments/TSAttachment.h +++ b/src/Messages/Attachments/TSAttachment.h @@ -6,20 +6,42 @@ NS_ASSUME_NONNULL_BEGIN +@class TSAttachmentPointer; + @interface TSAttachment : TSYapDatabaseObject { @protected NSString *_contentType; } +// TSAttachment is a base class for TSAttachmentPointer (a yet-to-be-downloaded +// incoming attachment) and TSAttachmentStream (an outgoing or already-downloaded +// incoming attachment). +// +// The attachmentSchemaVersion and serverId properties only apply to +// TSAttachmentPointer, which can be distinguished by the isDownloaded +// property. @property (atomic, readwrite) UInt64 serverId; @property (atomic, readwrite) NSData *encryptionKey; @property (nonatomic, readonly) NSString *contentType; +// This property + +@property (atomic, readwrite) BOOL isDownloaded; +// This constructor is used for new instances of TSAttachmentPointer, +// i.e. undownloaded incoming attachments. - (instancetype)initWithServerId:(UInt64)serverId encryptionKey:(NSData *)encryptionKey contentType:(NSString *)contentType; +// This constructor is used for new instances of TSAttachmentStream +// that represent new, un-uploaded outgoing attachments. +- (instancetype)initWithContentType:(NSString *)contentType; + +// This constructor is used for new instances of TSAttachmentStream +// that represent downloaded incoming attachments. +- (instancetype)initWithPointer:(TSAttachmentPointer *)pointer; + - (void)upgradeFromAttachmentSchemaVersion:(NSUInteger)attachmentSchemaVersion; @end diff --git a/src/Messages/Attachments/TSAttachment.m b/src/Messages/Attachments/TSAttachment.m index c955fbff4..3fcbd3946 100644 --- a/src/Messages/Attachments/TSAttachment.m +++ b/src/Messages/Attachments/TSAttachment.m @@ -4,6 +4,7 @@ #import "TSAttachment.h" #import "MIMETypeUtil.h" +#import "TSAttachmentPointer.h" NS_ASSUME_NONNULL_BEGIN @@ -17,6 +18,8 @@ NSUInteger const TSAttachmentSchemaVersion = 3; @implementation TSAttachment +// This constructor is used for new instances of TSAttachmentPointer, +// i.e. undownloaded incoming attachments. - (instancetype)initWithServerId:(UInt64)serverId encryptionKey:(NSData *)encryptionKey contentType:(NSString *)contentType @@ -34,6 +37,44 @@ NSUInteger const TSAttachmentSchemaVersion = 3; return self; } +// This constructor is used for new instances of TSAttachmentStream +// that represent new, un-uploaded outgoing attachments. +- (instancetype)initWithContentType:(NSString *)contentType +{ + self = [super init]; + if (!self) { + return self; + } + + _contentType = contentType; + _attachmentSchemaVersion = TSAttachmentSchemaVersion; + + return self; +} + +// This constructor is used for new instances of TSAttachmentStream +// that represent downloaded incoming attachments. +- (instancetype)initWithPointer:(TSAttachmentPointer *)pointer +{ + // Once saved, this AttachmentStream will replace the AttachmentPointer in the attachments collection. + self = [super initWithUniqueId:pointer.uniqueId]; + if (!self) { + return self; + } + + _serverId = pointer.serverId; + _encryptionKey = pointer.encryptionKey; + _contentType = pointer.contentType; + _attachmentSchemaVersion = TSAttachmentSchemaVersion; + + return self; +} + +- (BOOL)isDecimalNumberText:(NSString *)text +{ + return [text componentsSeparatedByCharactersInSet:[NSCharacterSet decimalDigitCharacterSet]].count == 1; +} + - (nullable instancetype)initWithCoder:(NSCoder *)coder { self = [super initWithCoder:coder]; @@ -51,11 +92,22 @@ NSUInteger const TSAttachmentSchemaVersion = 3; - (void)upgradeFromAttachmentSchemaVersion:(NSUInteger)attachmentSchemaVersion { - if (attachmentSchemaVersion < 2) { + // TSAttachment is a base class for TSAttachmentPointer (a yet-to-be-downloaded + // incoming attachment) and TSAttachmentStream (an outgoing or already-downloaded + // incoming attachment). + // + // The attachmentSchemaVersion and serverId properties only apply to + // TSAttachmentPointer, which can be distinguished by the isDownloaded + // property. + if (!_isDownloaded && _attachmentSchemaVersion < 2) { if (!_serverId) { + OWSAssert([self isDecimalNumberText:self.uniqueId]); + if (![self isDecimalNumberText:self.uniqueId]) { + DDLogError(@"%@ invalid legacy attachment uniqueId: %@.", self.tag, self.uniqueId); + } _serverId = [self.uniqueId integerValue]; if (!_serverId) { - DDLogError(@"%@ failed to parse legacy uniqueId:%@ as integer.", self.tag, self.uniqueId); + DDLogError(@"%@ failed to parse legacy attachment uniqueId: %@.", self.tag, self.uniqueId); } } } diff --git a/src/Messages/Attachments/TSAttachmentPointer.m b/src/Messages/Attachments/TSAttachmentPointer.m index 426878483..8701335f4 100644 --- a/src/Messages/Attachments/TSAttachmentPointer.m +++ b/src/Messages/Attachments/TSAttachmentPointer.m @@ -28,10 +28,6 @@ NS_ASSUME_NONNULL_BEGIN return self; } -- (BOOL)isDownloaded { - return NO; -} - @end NS_ASSUME_NONNULL_END diff --git a/src/Messages/Attachments/TSAttachmentStream.h b/src/Messages/Attachments/TSAttachmentStream.h index 8bd2b6a2b..6e4439b52 100644 --- a/src/Messages/Attachments/TSAttachmentStream.h +++ b/src/Messages/Attachments/TSAttachmentStream.h @@ -17,8 +17,6 @@ NS_ASSUME_NONNULL_BEGIN - (instancetype)initWithContentType:(NSString *)contentType NS_DESIGNATED_INITIALIZER; - (instancetype)initWithPointer:(TSAttachmentPointer *)pointer NS_DESIGNATED_INITIALIZER; -@property (atomic, readwrite) BOOL isDownloaded; - // Though now required, `digest` may be null for pre-existing records or from // messages received from other clients @property (nullable, nonatomic) NSData *digest; diff --git a/src/Messages/Attachments/TSAttachmentStream.m b/src/Messages/Attachments/TSAttachmentStream.m index a3dd9f364..7ce6e5bf4 100644 --- a/src/Messages/Attachments/TSAttachmentStream.m +++ b/src/Messages/Attachments/TSAttachmentStream.m @@ -14,13 +14,12 @@ NS_ASSUME_NONNULL_BEGIN - (instancetype)initWithContentType:(NSString *)contentType { - self = [super init]; + self = [super initWithContentType:contentType]; if (!self) { return self; } - _contentType = contentType; - _isDownloaded = YES; + self.isDownloaded = YES; // TSAttachmentStream doesn't have any "incoming vs. outgoing" // state, but this constructor is used only for new outgoing // attachments which haven't been uploaded yet. @@ -32,13 +31,13 @@ NS_ASSUME_NONNULL_BEGIN - (instancetype)initWithPointer:(TSAttachmentPointer *)pointer { // Once saved, this AttachmentStream will replace the AttachmentPointer in the attachments collection. - self = [super initWithUniqueId:pointer.uniqueId]; + self = [super initWithPointer:pointer]; if (!self) { return self; } _contentType = pointer.contentType; - _isDownloaded = YES; + self.isDownloaded = YES; // TSAttachmentStream doesn't have any "incoming vs. outgoing" // state, but this constructor is used only for new incoming // attachments which don't need to be uploaded. diff --git a/src/Network/WebSockets/TSSocketManager.m b/src/Network/WebSockets/TSSocketManager.m index 6b6a412b5..6d8d6846f 100644 --- a/src/Network/WebSockets/TSSocketManager.m +++ b/src/Network/WebSockets/TSSocketManager.m @@ -114,6 +114,8 @@ NSString *const SocketConnectingNotification = @"SocketConnectingNotification"; } } + DDLogWarn(@"Creating new websocket"); + // If socket is not already open or connecting, connect now. self.status = kSocketStatusConnecting; } @@ -229,6 +231,7 @@ NSString *const SocketConnectingNotification = @"SocketConnectingNotification"; + (void)resignActivity { OWSAssert([NSThread isMainThread]); + DDLogWarn(@"resignActivity closing web socket"); [[self sharedManager] closeWebSocket]; } @@ -253,8 +256,8 @@ NSString *const SocketConnectingNotification = @"SocketConnectingNotification"; - (void)webSocket:(SRWebSocket *)webSocket didFailWithError:(NSError *)error { OWSAssert([NSThread isMainThread]); - - DDLogError(@"Error connecting to socket %@", error); + + DDLogError(@"Websocket did fail with error: %@", error); [self closeWebSocket]; @@ -352,6 +355,8 @@ NSString *const SocketConnectingNotification = @"SocketConnectingNotification"; wasClean:(BOOL)wasClean { OWSAssert([NSThread isMainThread]); + DDLogWarn(@"Websocket did close with code: %ld", (long)code); + [self closeWebSocket]; if (!wasClean && [self shouldKeepWebSocketAlive]) { @@ -370,6 +375,7 @@ NSString *const SocketConnectingNotification = @"SocketConnectingNotification"; [self scheduleRetry]; } } else { + DDLogWarn(@"webSocketHeartBeat closing web socket"); [self closeWebSocket]; } } From c8efd83c394f605392eba149c2a8967acbd60dcf Mon Sep 17 00:00:00 2001 From: Matthew Chen Date: Wed, 15 Mar 2017 12:32:58 -0300 Subject: [PATCH 2/2] Respond to CR. // FREEBIE --- src/Messages/Attachments/TSAttachment.h | 5 +-- src/Messages/Attachments/TSAttachment.m | 29 ++--------------- .../Attachments/TSAttachmentPointer.m | 32 +++++++++++++++++++ 3 files changed, 36 insertions(+), 30 deletions(-) diff --git a/src/Messages/Attachments/TSAttachment.h b/src/Messages/Attachments/TSAttachment.h index 150fa7199..537ae0a33 100644 --- a/src/Messages/Attachments/TSAttachment.h +++ b/src/Messages/Attachments/TSAttachment.h @@ -6,8 +6,6 @@ NS_ASSUME_NONNULL_BEGIN -@class TSAttachmentPointer; - @interface TSAttachment : TSYapDatabaseObject { @protected @@ -24,7 +22,6 @@ NS_ASSUME_NONNULL_BEGIN @property (atomic, readwrite) UInt64 serverId; @property (atomic, readwrite) NSData *encryptionKey; @property (nonatomic, readonly) NSString *contentType; -// This property @property (atomic, readwrite) BOOL isDownloaded; @@ -40,7 +37,7 @@ NS_ASSUME_NONNULL_BEGIN // This constructor is used for new instances of TSAttachmentStream // that represent downloaded incoming attachments. -- (instancetype)initWithPointer:(TSAttachmentPointer *)pointer; +- (instancetype)initWithPointer:(TSAttachment *)pointer; - (void)upgradeFromAttachmentSchemaVersion:(NSUInteger)attachmentSchemaVersion; diff --git a/src/Messages/Attachments/TSAttachment.m b/src/Messages/Attachments/TSAttachment.m index 3fcbd3946..cd92bbf38 100644 --- a/src/Messages/Attachments/TSAttachment.m +++ b/src/Messages/Attachments/TSAttachment.m @@ -4,7 +4,6 @@ #import "TSAttachment.h" #import "MIMETypeUtil.h" -#import "TSAttachmentPointer.h" NS_ASSUME_NONNULL_BEGIN @@ -54,7 +53,7 @@ NSUInteger const TSAttachmentSchemaVersion = 3; // This constructor is used for new instances of TSAttachmentStream // that represent downloaded incoming attachments. -- (instancetype)initWithPointer:(TSAttachmentPointer *)pointer +- (instancetype)initWithPointer:(TSAttachment *)pointer { // Once saved, this AttachmentStream will replace the AttachmentPointer in the attachments collection. self = [super initWithUniqueId:pointer.uniqueId]; @@ -70,11 +69,6 @@ NSUInteger const TSAttachmentSchemaVersion = 3; return self; } -- (BOOL)isDecimalNumberText:(NSString *)text -{ - return [text componentsSeparatedByCharactersInSet:[NSCharacterSet decimalDigitCharacterSet]].count == 1; -} - - (nullable instancetype)initWithCoder:(NSCoder *)coder { self = [super initWithCoder:coder]; @@ -92,25 +86,8 @@ NSUInteger const TSAttachmentSchemaVersion = 3; - (void)upgradeFromAttachmentSchemaVersion:(NSUInteger)attachmentSchemaVersion { - // TSAttachment is a base class for TSAttachmentPointer (a yet-to-be-downloaded - // incoming attachment) and TSAttachmentStream (an outgoing or already-downloaded - // incoming attachment). - // - // The attachmentSchemaVersion and serverId properties only apply to - // TSAttachmentPointer, which can be distinguished by the isDownloaded - // property. - if (!_isDownloaded && _attachmentSchemaVersion < 2) { - if (!_serverId) { - OWSAssert([self isDecimalNumberText:self.uniqueId]); - if (![self isDecimalNumberText:self.uniqueId]) { - DDLogError(@"%@ invalid legacy attachment uniqueId: %@.", self.tag, self.uniqueId); - } - _serverId = [self.uniqueId integerValue]; - if (!_serverId) { - DDLogError(@"%@ failed to parse legacy attachment uniqueId: %@.", self.tag, self.uniqueId); - } - } - } + // This method is overridden by the base classes TSAttachmentPointer and + // TSAttachmentStream. } + (NSString *)collection { diff --git a/src/Messages/Attachments/TSAttachmentPointer.m b/src/Messages/Attachments/TSAttachmentPointer.m index 8701335f4..0010c79c8 100644 --- a/src/Messages/Attachments/TSAttachmentPointer.m +++ b/src/Messages/Attachments/TSAttachmentPointer.m @@ -28,6 +28,38 @@ NS_ASSUME_NONNULL_BEGIN return self; } +- (BOOL)isDecimalNumberText:(NSString *)text +{ + return [text componentsSeparatedByCharactersInSet:[NSCharacterSet decimalDigitCharacterSet]].count == 1; +} + +- (void)upgradeFromAttachmentSchemaVersion:(NSUInteger)attachmentSchemaVersion +{ + // Legacy instances of TSAttachmentPointer apparently used the serverId as their + // uniqueId. + if (attachmentSchemaVersion < 2 && self.serverId == 0) { + OWSAssert([self isDecimalNumberText:self.uniqueId]); + if ([self isDecimalNumberText:self.uniqueId]) { + // For legacy instances, try to parse the serverId from the uniqueId. + self.serverId = [self.uniqueId integerValue]; + } else { + DDLogError(@"%@ invalid legacy attachment uniqueId: %@.", self.tag, self.uniqueId); + } + } +} + +#pragma mark - Logging + ++ (NSString *)tag +{ + return [NSString stringWithFormat:@"[%@]", self.class]; +} + +- (NSString *)tag +{ + return self.class.tag; +} + @end NS_ASSUME_NONNULL_END