From cc59301a560790971d88a47f82dd3778c4babbb7 Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Wed, 20 Sep 2017 18:56:21 +0100 Subject: [PATCH 1/3] Fix crashes caused by failing to unlock or destroy a static mutex while the app is being terminated --- CHANGELOG.md | 1 + Source/ASImageNode.mm | 37 ++++++++++---- Source/ASTextNode.mm | 51 ++++++++++--------- Source/ASTextNode2.mm | 16 +++--- Source/Details/ASBasicImageDownloader.mm | 49 ++++++++++++------ Source/Details/ASThread.h | 18 +++++-- .../Private/ASBasicImageDownloaderInternal.h | 6 ++- Source/TextKit/ASTextKitContext.mm | 16 +++++- 8 files changed, 128 insertions(+), 66 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 913912959..58d653bf9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ - Add support for URLs on ASNetworkImageNode. [Garrett Moon](https://github.com/garrettmoon) - [ASImageNode] Always dealloc images in a background queue [Huy Nguyen](https://github.com/nguyenhuy) [#561](https://github.com/TextureGroup/Texture/pull/561) - Mark ASRunLoopQueue as drained if it contains only NULLs [Cesar Estebanez](https://github.com/cesteban) [#558](https://github.com/TextureGroup/Texture/pull/558) +- Fix crashes caused by failing to unlock or destroy a static mutex while the app is being terminated [Huy Nguyen](https://github.com/nguyenhuy) ##2.4 - Fix an issue where inserting/deleting sections could lead to inconsistent supplementary element behavior. [Adlai Holler](https://github.com/Adlai-Holler) diff --git a/Source/ASImageNode.mm b/Source/ASImageNode.mm index 165d74535..19970a3b0 100644 --- a/Source/ASImageNode.mm +++ b/Source/ASImageNode.mm @@ -442,31 +442,48 @@ + (UIImage *)displayWithParameters:(id)parameter isCancelled:(asdispla } static ASWeakMap *cache = nil; -static ASDN::Mutex cacheLock; +static ASDN::StaticMutex cacheLock = ASDISPLAYNODE_MUTEX_INITIALIZER; + (ASWeakMapEntry *)contentsForkey:(ASImageNodeContentsKey *)key drawParameters:(id)drawParameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelled { - { - ASDN::MutexLocker l(cacheLock); + cacheLock.lock(); if (!cache) { cache = [[ASWeakMap alloc] init]; } ASWeakMapEntry *entry = [cache entryForKey:key]; - if (entry != nil) { - return entry; - } + int unlockCode = cacheLock.unlock(); + + if (unlockCode != 0) { + // Failed to unlock cacheLock static mutex. + // Since the mutex was locked just a few lines above, this usually means there is a race condition going on. + // The race condition occurs when a static mutex is being destroyed as part of an app exit on main thread, + // and, at the same time, being used here on another thread. + // See https://github.com/TextureGroup/Texture/issues/136. + // + // Return nil here to signal that this operation should be cancelled. + return nil; } - + + if (entry != nil) { + return entry; + } + // cache miss UIImage *contents = [self createContentsForkey:key drawParameters:drawParameters isCancelled:isCancelled]; if (contents == nil) { // If nil, we were cancelled return nil; } - { - ASDN::MutexLocker l(cacheLock); - return [cache setObject:contents forKey:key]; + cacheLock.lock(); + entry = [cache setObject:contents forKey:key]; + unlockCode = cacheLock.unlock(); + + if (unlockCode != 0) { + // Same as above, return nil if fail to unlock. + return nil; } + + return entry; } + (UIImage *)createContentsForkey:(ASImageNodeContentsKey *)key drawParameters:(id)drawParameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelled diff --git a/Source/ASTextNode.mm b/Source/ASTextNode.mm index 18f0198ef..1cf97a74b 100644 --- a/Source/ASTextNode.mm +++ b/Source/ASTextNode.mm @@ -1384,7 +1384,7 @@ + (void)_registerAttributedText:(NSAttributedString *)str } #endif -static ASDN::Mutex _experimentLock; +static ASDN::StaticMutex _experimentLock = ASDISPLAYNODE_MUTEX_INITIALIZER; static ASTextNodeExperimentOptions _experimentOptions; static BOOL _hasAllocatedNode; @@ -1392,34 +1392,34 @@ + (void)setExperimentOptions:(ASTextNodeExperimentOptions)options { static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ - ASDN::MutexLocker lock(_experimentLock); - - // They must call this before allocating any text nodes. - ASDisplayNodeAssertFalse(_hasAllocatedNode); - - _experimentOptions = options; - - // Set superclass of all subclasses to ASTextNode2 - if (options & ASTextNodeExperimentSubclasses) { - unsigned int classCount; - Class originalClass = [ASTextNode class]; - Class newClass = [ASTextNode2 class]; - Class *classes = objc_copyClassList(&classCount); - for (int i = 0; i < classCount; i++) { - Class c = classes[i]; - if (class_getSuperclass(c) == originalClass) { + _experimentLock.lock(); + // They must call this before allocating any text nodes. + ASDisplayNodeAssertFalse(_hasAllocatedNode); + + _experimentOptions = options; + + // Set superclass of all subclasses to ASTextNode2 + if (options & ASTextNodeExperimentSubclasses) { + unsigned int classCount; + Class originalClass = [ASTextNode class]; + Class newClass = [ASTextNode2 class]; + Class *classes = objc_copyClassList(&classCount); + for (int i = 0; i < classCount; i++) { + Class c = classes[i]; + if (class_getSuperclass(c) == originalClass) { #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wdeprecated-declarations" - class_setSuperclass(c, newClass); + class_setSuperclass(c, newClass); #pragma clang diagnostic pop + } } + free(classes); } - free(classes); - } - if (options & ASTextNodeExperimentDebugging) { - [ASTextNode2 enableDebugging]; - } + if (options & ASTextNodeExperimentDebugging) { + [ASTextNode2 enableDebugging]; + } + _experimentLock.unlock(); // Ignoring unlock failure }); } @@ -1427,8 +1427,9 @@ + (id)allocWithZone:(struct _NSZone *)zone { static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ - ASDN::MutexLocker lock(_experimentLock); - _hasAllocatedNode = YES; + _experimentLock.lock(); + _hasAllocatedNode = YES; + _experimentLock.unlock(); // Ignoring unlock failure }); // All instances || (random instances && rand() != 0) diff --git a/Source/ASTextNode2.mm b/Source/ASTextNode2.mm index 48ff3c4bb..5711573e8 100644 --- a/Source/ASTextNode2.mm +++ b/Source/ASTextNode2.mm @@ -376,7 +376,7 @@ + (ASTextLayout *)compatibleLayoutWithContainer:(ASTextContainer *)container text:(NSAttributedString *)text { - static ASDN::Mutex layoutCacheLock; + static ASDN::StaticMutex layoutCacheLock = ASDISPLAYNODE_MUTEX_INITIALIZER; static NSCache *textLayoutCache; static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ @@ -384,12 +384,14 @@ + (ASTextLayout *)compatibleLayoutWithContainer:(ASTextContainer *)container }); ASTextCacheValue *cacheValue = ({ - ASDN::MutexLocker lock(layoutCacheLock); - cacheValue = [textLayoutCache objectForKey:text]; - if (cacheValue == nil) { - cacheValue = [[ASTextCacheValue alloc] init]; - [textLayoutCache setObject:cacheValue forKey:text]; - } + layoutCacheLock.lock(); + cacheValue = [textLayoutCache objectForKey:text]; + if (cacheValue == nil) { + cacheValue = [[ASTextCacheValue alloc] init]; + [textLayoutCache setObject:cacheValue forKey:text]; + } + layoutCacheLock.unlock(); // Ignoring unlock failure. Maybe bail early and return nil instead? + cacheValue; }); diff --git a/Source/Details/ASBasicImageDownloader.mm b/Source/Details/ASBasicImageDownloader.mm index 57fb41214..7dcd738db 100644 --- a/Source/Details/ASBasicImageDownloader.mm +++ b/Source/Details/ASBasicImageDownloader.mm @@ -46,28 +46,42 @@ @interface ASBasicImageDownloaderContext () @implementation ASBasicImageDownloaderContext static NSMutableDictionary *currentRequests = nil; -static ASDN::RecursiveMutex currentRequestsLock; +static ASDN::StaticMutex currentRequestsLock = ASDISPLAYNODE_MUTEX_INITIALIZER; + (ASBasicImageDownloaderContext *)contextForURL:(NSURL *)URL { - ASDN::MutexLocker l(currentRequestsLock); - if (!currentRequests) { - currentRequests = [[NSMutableDictionary alloc] init]; - } - ASBasicImageDownloaderContext *context = currentRequests[URL]; - if (!context) { - context = [[ASBasicImageDownloaderContext alloc] initWithURL:URL]; - currentRequests[URL] = context; + currentRequestsLock.lock(); + if (!currentRequests) { + currentRequests = [[NSMutableDictionary alloc] init]; + } + ASBasicImageDownloaderContext *context = currentRequests[URL]; + if (!context) { + context = [[ASBasicImageDownloaderContext alloc] initWithURL:URL]; + currentRequests[URL] = context; + } + int unlockCode = currentRequestsLock.unlock(); + + if (unlockCode != 0) { + // Failed to unlock currentRequestsLock static mutex. + // Since the mutex was locked just a few lines above, this usually means there is a race condition going on. + // The race condition occurs when a static mutex is being destroyed as part of an app exit on main thread, + // and, at the same time, being used here on another thread. + // See https://github.com/TextureGroup/Texture/issues/136. + // + // Return nil here to signal that the image should not be loaded. + return nil; } + return context; } + (void)cancelContextWithURL:(NSURL *)URL { - ASDN::MutexLocker l(currentRequestsLock); - if (currentRequests) { - [currentRequests removeObjectForKey:URL]; - } + currentRequestsLock.lock(); + if (currentRequests) { + [currentRequests removeObjectForKey:URL]; + } + currentRequestsLock.unlock(); // Ignoring unlock failure } - (instancetype)initWithURL:(NSURL *)URL @@ -234,11 +248,14 @@ - (instancetype)_init #pragma mark ASImageDownloaderProtocol. - (id)downloadImageWithURL:(NSURL *)URL - callbackQueue:(dispatch_queue_t)callbackQueue - downloadProgress:(nullable ASImageDownloaderProgress)downloadProgress - completion:(ASImageDownloaderCompletion)completion + callbackQueue:(dispatch_queue_t)callbackQueue + downloadProgress:(nullable ASImageDownloaderProgress)downloadProgress + completion:(ASImageDownloaderCompletion)completion { ASBasicImageDownloaderContext *context = [ASBasicImageDownloaderContext contextForURL:URL]; + if (context == nil) { + return nil; + } // NSURLSessionDownloadTask will do file I/O to create a temp directory. If called on the main thread this will // cause significant performance issues. diff --git a/Source/Details/ASThread.h b/Source/Details/ASThread.h index ca049d72a..e57dbc29c 100644 --- a/Source/Details/ASThread.h +++ b/Source/Details/ASThread.h @@ -314,8 +314,19 @@ namespace ASDN { ASDISPLAYNODE_THREAD_ASSERT_ON_ERROR(pthread_mutex_lock (this->mutex())); } - void unlock () { - ASDISPLAYNODE_THREAD_ASSERT_ON_ERROR(pthread_mutex_unlock (this->mutex())); + /** + Unlocks this mutex. + + This method calls pthread_mutex_unlock() internally and returns its result. + Clients are expected to handle errors themselves. + + Because of this behavior, StaticMutexLocker and StaticMutexUnlocker are not (and should not be) provided. + + For more information, see https://github.com/TextureGroup/Texture/issues/136 and + https://www.ibm.com/support/knowledgecenter/en/ssw_i5_54/apis/users_65.htm + */ + int unlock () { + return pthread_mutex_unlock (this->mutex()); } pthread_mutex_t *mutex () { return &_m; } @@ -324,9 +335,6 @@ namespace ASDN { StaticMutex &operator=(const StaticMutex&) = delete; }; - typedef Locker StaticMutexLocker; - typedef Unlocker StaticMutexUnlocker; - struct Condition { Condition () { diff --git a/Source/Private/ASBasicImageDownloaderInternal.h b/Source/Private/ASBasicImageDownloaderInternal.h index e3d6d6dc5..241c1eb52 100644 --- a/Source/Private/ASBasicImageDownloaderInternal.h +++ b/Source/Private/ASBasicImageDownloaderInternal.h @@ -15,9 +15,11 @@ // http://www.apache.org/licenses/LICENSE-2.0 // +NS_ASSUME_NONNULL_BEGIN + @interface ASBasicImageDownloaderContext : NSObject -+ (ASBasicImageDownloaderContext *)contextForURL:(NSURL *)URL; ++ (nullable ASBasicImageDownloaderContext *)contextForURL:(NSURL *)URL; @property (nonatomic, strong, readonly) NSURL *URL; @property (nonatomic, weak) NSURLSessionTask *sessionTask; @@ -26,3 +28,5 @@ - (void)cancel; @end + +NS_ASSUME_NONNULL_END diff --git a/Source/TextKit/ASTextKitContext.mm b/Source/TextKit/ASTextKitContext.mm index 64fcf6509..9df94387a 100755 --- a/Source/TextKit/ASTextKitContext.mm +++ b/Source/TextKit/ASTextKitContext.mm @@ -40,8 +40,8 @@ - (instancetype)initWithAttributedString:(NSAttributedString *)attributedString { if (self = [super init]) { // Concurrently initialising TextKit components crashes (rdar://18448377) so we use a global lock. - static ASDN::Mutex __staticMutex; - ASDN::MutexLocker l(__staticMutex); + static ASDN::StaticMutex __staticMutex = ASDISPLAYNODE_MUTEX_INITIALIZER; + __staticMutex.lock(); __instanceLock__ = std::make_shared(); @@ -65,6 +65,18 @@ - (instancetype)initWithAttributedString:(NSAttributedString *)attributedString _textContainer.maximumNumberOfLines = maximumNumberOfLines; _textContainer.exclusionPaths = exclusionPaths; [_layoutManager addTextContainer:_textContainer]; + + if (__staticMutex.unlock() != 0) { + // Failed to unlock __staticMutex + // Since the mutex was locked at the beginninng of this method, the failure usually means there is a race condition going on. + // The race condition occurs when a static mutex is being destroyed as part of an app exit on main thread, + // and, at the same time, being used here on another thread. + // See https://github.com/TextureGroup/Texture/issues/136. + // + // Return nil here to signal that this initialization should not be done in the first place, + // and to ensure that `self` won't be used later. + return nil; + } } return self; } From ea74bf26fb85e5dc0a80a0e67a2e19b0b6564a50 Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Thu, 21 Sep 2017 15:43:39 +0100 Subject: [PATCH 2/3] Allocate static mutexes on the heap memory to avoid destruction at app exit --- Source/ASImageNode.mm | 38 ++++---------- Source/ASTextNode.mm | 52 +++++++++---------- Source/ASTextNode2.mm | 17 +++--- Source/Details/ASBasicImageDownloader.mm | 50 ++++++------------ Source/Details/ASThread.h | 39 +++++--------- .../Private/ASBasicImageDownloaderInternal.h | 6 +-- Source/TextKit/ASTextKitContext.mm | 17 ++---- 7 files changed, 79 insertions(+), 140 deletions(-) diff --git a/Source/ASImageNode.mm b/Source/ASImageNode.mm index 19970a3b0..df39741b0 100644 --- a/Source/ASImageNode.mm +++ b/Source/ASImageNode.mm @@ -442,48 +442,32 @@ + (UIImage *)displayWithParameters:(id)parameter isCancelled:(asdispla } static ASWeakMap *cache = nil; -static ASDN::StaticMutex cacheLock = ASDISPLAYNODE_MUTEX_INITIALIZER; +// Allocate cacheLock on the heap to prevent destruction at app exit (https://github.com/TextureGroup/Texture/issues/136) +static ASDN::StaticMutex& cacheLock = *new ASDN::StaticMutex; + (ASWeakMapEntry *)contentsForkey:(ASImageNodeContentsKey *)key drawParameters:(id)drawParameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelled { - cacheLock.lock(); + { + ASDN::StaticMutexLocker l(cacheLock); if (!cache) { cache = [[ASWeakMap alloc] init]; } ASWeakMapEntry *entry = [cache entryForKey:key]; - int unlockCode = cacheLock.unlock(); - - if (unlockCode != 0) { - // Failed to unlock cacheLock static mutex. - // Since the mutex was locked just a few lines above, this usually means there is a race condition going on. - // The race condition occurs when a static mutex is being destroyed as part of an app exit on main thread, - // and, at the same time, being used here on another thread. - // See https://github.com/TextureGroup/Texture/issues/136. - // - // Return nil here to signal that this operation should be cancelled. - return nil; - } - - if (entry != nil) { - return entry; + if (entry != nil) { + return entry; + } } - + // cache miss UIImage *contents = [self createContentsForkey:key drawParameters:drawParameters isCancelled:isCancelled]; if (contents == nil) { // If nil, we were cancelled return nil; } - cacheLock.lock(); - entry = [cache setObject:contents forKey:key]; - unlockCode = cacheLock.unlock(); - - if (unlockCode != 0) { - // Same as above, return nil if fail to unlock. - return nil; + { + ASDN::StaticMutexLocker l(cacheLock); + return [cache setObject:contents forKey:key]; } - - return entry; } + (UIImage *)createContentsForkey:(ASImageNodeContentsKey *)key drawParameters:(id)drawParameters isCancelled:(asdisplaynode_iscancelled_block_t)isCancelled diff --git a/Source/ASTextNode.mm b/Source/ASTextNode.mm index 1cf97a74b..6caaec8cd 100644 --- a/Source/ASTextNode.mm +++ b/Source/ASTextNode.mm @@ -1384,7 +1384,8 @@ + (void)_registerAttributedText:(NSAttributedString *)str } #endif -static ASDN::StaticMutex _experimentLock = ASDISPLAYNODE_MUTEX_INITIALIZER; +// Allocate _experimentLock on the heap to prevent destruction at app exit (https://github.com/TextureGroup/Texture/issues/136) +static ASDN::StaticMutex& _experimentLock = *new ASDN::StaticMutex; static ASTextNodeExperimentOptions _experimentOptions; static BOOL _hasAllocatedNode; @@ -1392,34 +1393,34 @@ + (void)setExperimentOptions:(ASTextNodeExperimentOptions)options { static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ - _experimentLock.lock(); - // They must call this before allocating any text nodes. - ASDisplayNodeAssertFalse(_hasAllocatedNode); - - _experimentOptions = options; - - // Set superclass of all subclasses to ASTextNode2 - if (options & ASTextNodeExperimentSubclasses) { - unsigned int classCount; - Class originalClass = [ASTextNode class]; - Class newClass = [ASTextNode2 class]; - Class *classes = objc_copyClassList(&classCount); - for (int i = 0; i < classCount; i++) { - Class c = classes[i]; - if (class_getSuperclass(c) == originalClass) { + ASDN::StaticMutexLocker lock(_experimentLock); + + // They must call this before allocating any text nodes. + ASDisplayNodeAssertFalse(_hasAllocatedNode); + + _experimentOptions = options; + + // Set superclass of all subclasses to ASTextNode2 + if (options & ASTextNodeExperimentSubclasses) { + unsigned int classCount; + Class originalClass = [ASTextNode class]; + Class newClass = [ASTextNode2 class]; + Class *classes = objc_copyClassList(&classCount); + for (int i = 0; i < classCount; i++) { + Class c = classes[i]; + if (class_getSuperclass(c) == originalClass) { #pragma clang diagnostic push #pragma clang diagnostic ignored "-Wdeprecated-declarations" - class_setSuperclass(c, newClass); + class_setSuperclass(c, newClass); #pragma clang diagnostic pop - } } - free(classes); } + free(classes); + } - if (options & ASTextNodeExperimentDebugging) { - [ASTextNode2 enableDebugging]; - } - _experimentLock.unlock(); // Ignoring unlock failure + if (options & ASTextNodeExperimentDebugging) { + [ASTextNode2 enableDebugging]; + } }); } @@ -1427,9 +1428,8 @@ + (id)allocWithZone:(struct _NSZone *)zone { static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ - _experimentLock.lock(); - _hasAllocatedNode = YES; - _experimentLock.unlock(); // Ignoring unlock failure + ASDN::StaticMutexLocker lock(_experimentLock); + _hasAllocatedNode = YES; }); // All instances || (random instances && rand() != 0) diff --git a/Source/ASTextNode2.mm b/Source/ASTextNode2.mm index 5711573e8..ecd9496c5 100644 --- a/Source/ASTextNode2.mm +++ b/Source/ASTextNode2.mm @@ -376,7 +376,8 @@ + (ASTextLayout *)compatibleLayoutWithContainer:(ASTextContainer *)container text:(NSAttributedString *)text { - static ASDN::StaticMutex layoutCacheLock = ASDISPLAYNODE_MUTEX_INITIALIZER; + // Allocate layoutCacheLock on the heap to prevent destruction at app exit (https://github.com/TextureGroup/Texture/issues/136) + static ASDN::StaticMutex& layoutCacheLock = *new ASDN::StaticMutex; static NSCache *textLayoutCache; static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ @@ -384,14 +385,12 @@ + (ASTextLayout *)compatibleLayoutWithContainer:(ASTextContainer *)container }); ASTextCacheValue *cacheValue = ({ - layoutCacheLock.lock(); - cacheValue = [textLayoutCache objectForKey:text]; - if (cacheValue == nil) { - cacheValue = [[ASTextCacheValue alloc] init]; - [textLayoutCache setObject:cacheValue forKey:text]; - } - layoutCacheLock.unlock(); // Ignoring unlock failure. Maybe bail early and return nil instead? - + ASDN::StaticMutexLocker lock(layoutCacheLock); + cacheValue = [textLayoutCache objectForKey:text]; + if (cacheValue == nil) { + cacheValue = [[ASTextCacheValue alloc] init]; + [textLayoutCache setObject:cacheValue forKey:text]; + } cacheValue; }); diff --git a/Source/Details/ASBasicImageDownloader.mm b/Source/Details/ASBasicImageDownloader.mm index 7dcd738db..0d8084265 100644 --- a/Source/Details/ASBasicImageDownloader.mm +++ b/Source/Details/ASBasicImageDownloader.mm @@ -46,42 +46,29 @@ @interface ASBasicImageDownloaderContext () @implementation ASBasicImageDownloaderContext static NSMutableDictionary *currentRequests = nil; -static ASDN::StaticMutex currentRequestsLock = ASDISPLAYNODE_MUTEX_INITIALIZER; +// Allocate currentRequestsLock on the heap to prevent destruction at app exit (https://github.com/TextureGroup/Texture/issues/136) +static ASDN::StaticMutex& currentRequestsLock = *new ASDN::StaticMutex; + (ASBasicImageDownloaderContext *)contextForURL:(NSURL *)URL { - currentRequestsLock.lock(); - if (!currentRequests) { - currentRequests = [[NSMutableDictionary alloc] init]; - } - ASBasicImageDownloaderContext *context = currentRequests[URL]; - if (!context) { - context = [[ASBasicImageDownloaderContext alloc] initWithURL:URL]; - currentRequests[URL] = context; - } - int unlockCode = currentRequestsLock.unlock(); - - if (unlockCode != 0) { - // Failed to unlock currentRequestsLock static mutex. - // Since the mutex was locked just a few lines above, this usually means there is a race condition going on. - // The race condition occurs when a static mutex is being destroyed as part of an app exit on main thread, - // and, at the same time, being used here on another thread. - // See https://github.com/TextureGroup/Texture/issues/136. - // - // Return nil here to signal that the image should not be loaded. - return nil; + ASDN::StaticMutexLocker l(currentRequestsLock); + if (!currentRequests) { + currentRequests = [[NSMutableDictionary alloc] init]; + } + ASBasicImageDownloaderContext *context = currentRequests[URL]; + if (!context) { + context = [[ASBasicImageDownloaderContext alloc] initWithURL:URL]; + currentRequests[URL] = context; } - return context; } + (void)cancelContextWithURL:(NSURL *)URL { - currentRequestsLock.lock(); - if (currentRequests) { - [currentRequests removeObjectForKey:URL]; - } - currentRequestsLock.unlock(); // Ignoring unlock failure + ASDN::StaticMutexLocker l(currentRequestsLock); + if (currentRequests) { + [currentRequests removeObjectForKey:URL]; + } } - (instancetype)initWithURL:(NSURL *)URL @@ -248,14 +235,11 @@ - (instancetype)_init #pragma mark ASImageDownloaderProtocol. - (id)downloadImageWithURL:(NSURL *)URL - callbackQueue:(dispatch_queue_t)callbackQueue - downloadProgress:(nullable ASImageDownloaderProgress)downloadProgress - completion:(ASImageDownloaderCompletion)completion + callbackQueue:(dispatch_queue_t)callbackQueue + downloadProgress:(nullable ASImageDownloaderProgress)downloadProgress + completion:(ASImageDownloaderCompletion)completion { ASBasicImageDownloaderContext *context = [ASBasicImageDownloaderContext contextForURL:URL]; - if (context == nil) { - return nil; - } // NSURLSessionDownloadTask will do file I/O to create a temp directory. If called on the main thread this will // cause significant performance issues. diff --git a/Source/Details/ASThread.h b/Source/Details/ASThread.h index e57dbc29c..d97d351bf 100644 --- a/Source/Details/ASThread.h +++ b/Source/Details/ASThread.h @@ -52,12 +52,6 @@ static inline BOOL ASDisplayNodeThreadIsMain() #include -/** - For use with ASDN::StaticMutex only. - */ -#define ASDISPLAYNODE_MUTEX_INITIALIZER {PTHREAD_MUTEX_INITIALIZER} -#define ASDISPLAYNODE_MUTEX_RECURSIVE_INITIALIZER {PTHREAD_RECURSIVE_MUTEX_INITIALIZER} - // This MUST always execute, even when assertions are disabled. Otherwise all lock operations become no-ops! // (To be explicit, do not turn this into an NSAssert, assert(), or any other kind of statement where the // evaluation of x_ can be compiled out.) @@ -297,44 +291,37 @@ namespace ASDN { typedef SharedUnlocker MutexSharedUnlocker; /** - If you are creating a static mutex, use StaticMutex and specify its default value as one of ASDISPLAYNODE_MUTEX_INITIALIZER - or ASDISPLAYNODE_MUTEX_RECURSIVE_INITIALIZER. This avoids expensive constructor overhead at startup (or worse, ordering + If you are creating a static mutex, use StaticMutex. This avoids expensive constructor overhead at startup (or worse, ordering issues between different static objects). It also avoids running a destructor on app exit time (needless expense). Note that you can, but should not, use StaticMutex for non-static objects. It will leak its mutex on destruction, so avoid that! - - If you fail to specify a default value (like ASDISPLAYNODE_MUTEX_INITIALIZER) an assert will be thrown when you attempt to lock. */ struct StaticMutex { - pthread_mutex_t _m; // public so it can be provided by ASDISPLAYNODE_MUTEX_INITIALIZER and friends + StaticMutex () : _m (PTHREAD_MUTEX_INITIALIZER) {} + + // non-copyable. + StaticMutex(const StaticMutex&) = delete; + StaticMutex &operator=(const StaticMutex&) = delete; void lock () { ASDISPLAYNODE_THREAD_ASSERT_ON_ERROR(pthread_mutex_lock (this->mutex())); } - /** - Unlocks this mutex. - - This method calls pthread_mutex_unlock() internally and returns its result. - Clients are expected to handle errors themselves. - - Because of this behavior, StaticMutexLocker and StaticMutexUnlocker are not (and should not be) provided. - - For more information, see https://github.com/TextureGroup/Texture/issues/136 and - https://www.ibm.com/support/knowledgecenter/en/ssw_i5_54/apis/users_65.htm - */ - int unlock () { - return pthread_mutex_unlock (this->mutex()); + void unlock () { + ASDISPLAYNODE_THREAD_ASSERT_ON_ERROR(pthread_mutex_unlock (this->mutex())); } pthread_mutex_t *mutex () { return &_m; } - StaticMutex(const StaticMutex&) = delete; - StaticMutex &operator=(const StaticMutex&) = delete; + private: + pthread_mutex_t _m; }; + typedef Locker StaticMutexLocker; + typedef Unlocker StaticMutexUnlocker; + struct Condition { Condition () { diff --git a/Source/Private/ASBasicImageDownloaderInternal.h b/Source/Private/ASBasicImageDownloaderInternal.h index 241c1eb52..e3d6d6dc5 100644 --- a/Source/Private/ASBasicImageDownloaderInternal.h +++ b/Source/Private/ASBasicImageDownloaderInternal.h @@ -15,11 +15,9 @@ // http://www.apache.org/licenses/LICENSE-2.0 // -NS_ASSUME_NONNULL_BEGIN - @interface ASBasicImageDownloaderContext : NSObject -+ (nullable ASBasicImageDownloaderContext *)contextForURL:(NSURL *)URL; ++ (ASBasicImageDownloaderContext *)contextForURL:(NSURL *)URL; @property (nonatomic, strong, readonly) NSURL *URL; @property (nonatomic, weak) NSURLSessionTask *sessionTask; @@ -28,5 +26,3 @@ NS_ASSUME_NONNULL_BEGIN - (void)cancel; @end - -NS_ASSUME_NONNULL_END diff --git a/Source/TextKit/ASTextKitContext.mm b/Source/TextKit/ASTextKitContext.mm index 9df94387a..f8b3d1565 100755 --- a/Source/TextKit/ASTextKitContext.mm +++ b/Source/TextKit/ASTextKitContext.mm @@ -40,8 +40,9 @@ - (instancetype)initWithAttributedString:(NSAttributedString *)attributedString { if (self = [super init]) { // Concurrently initialising TextKit components crashes (rdar://18448377) so we use a global lock. - static ASDN::StaticMutex __staticMutex = ASDISPLAYNODE_MUTEX_INITIALIZER; - __staticMutex.lock(); + // Allocate __staticMutex on the heap to prevent destruction at app exit (https://github.com/TextureGroup/Texture/issues/136) + static ASDN::StaticMutex& __staticMutex = *new ASDN::StaticMutex; + ASDN::StaticMutexLocker l(__staticMutex); __instanceLock__ = std::make_shared(); @@ -65,18 +66,6 @@ - (instancetype)initWithAttributedString:(NSAttributedString *)attributedString _textContainer.maximumNumberOfLines = maximumNumberOfLines; _textContainer.exclusionPaths = exclusionPaths; [_layoutManager addTextContainer:_textContainer]; - - if (__staticMutex.unlock() != 0) { - // Failed to unlock __staticMutex - // Since the mutex was locked at the beginninng of this method, the failure usually means there is a race condition going on. - // The race condition occurs when a static mutex is being destroyed as part of an app exit on main thread, - // and, at the same time, being used here on another thread. - // See https://github.com/TextureGroup/Texture/issues/136. - // - // Return nil here to signal that this initialization should not be done in the first place, - // and to ensure that `self` won't be used later. - return nil; - } } return self; } From c8096b4b6841bafb0099f33c4cf330744e460f60 Mon Sep 17 00:00:00 2001 From: Huy Nguyen Date: Thu, 21 Sep 2017 15:56:35 +0100 Subject: [PATCH 3/3] ASThread to use ASDisplayNodeCAssert() instead of assert() --- Source/Details/ASThread.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/Source/Details/ASThread.h b/Source/Details/ASThread.h index d97d351bf..989db9a26 100644 --- a/Source/Details/ASThread.h +++ b/Source/Details/ASThread.h @@ -59,7 +59,7 @@ static inline BOOL ASDisplayNodeThreadIsMain() _Pragma("clang diagnostic push"); \ _Pragma("clang diagnostic ignored \"-Wunused-variable\""); \ volatile int res = (x_); \ - assert(res == 0); \ + ASDisplayNodeCAssert(res == 0, @"Expected %@ to return 0, got %d instead", @#x_, res); \ _Pragma("clang diagnostic pop"); \ } while (0) @@ -136,7 +136,7 @@ namespace ASDN { #if !TIME_LOCKER SharedLocker (std::shared_ptr const& l) ASDISPLAYNODE_NOTHROW : _l (l) { - assert(_l != nullptr); + ASDisplayNodeCAssertTrue(_l != nullptr); _l->lock (); } @@ -211,12 +211,12 @@ namespace ASDN { mach_port_t thread_id = pthread_mach_thread_np(pthread_self()); if (thread_id != _owner) { // New owner. Since this mutex can't be acquired by another thread if there is an existing owner, _owner and _count must be 0. - assert(0 == _owner); - assert(0 == _count); + ASDisplayNodeCAssertTrue(0 == _owner); + ASDisplayNodeCAssertTrue(0 == _count); _owner = thread_id; } else { // Existing owner tries to reacquire this (recursive) mutex. _count must already be positive. - assert(_count > 0); + ASDisplayNodeCAssertTrue(_count > 0); } ++_count; #endif @@ -226,9 +226,9 @@ namespace ASDN { #if CHECK_LOCKING_SAFETY mach_port_t thread_id = pthread_mach_thread_np(pthread_self()); // Unlocking a mutex on an unowning thread causes undefined behaviour. Assert and fail early. - assert(thread_id == _owner); + ASDisplayNodeCAssertTrue(thread_id == _owner); // Current thread owns this mutex. _count must be positive. - assert(_count > 0); + ASDisplayNodeCAssertTrue(_count > 0); --_count; if (0 == _count) { // Current thread is no longer the owner.