diff --git a/CHANGELOG.md b/CHANGELOG.md index 9139129598..58d653bf94 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 165d745352..df39741b00 100644 --- a/Source/ASImageNode.mm +++ b/Source/ASImageNode.mm @@ -442,12 +442,13 @@ typedef void (^ASImageNodeDrawParametersBlock)(ASWeakMapEntry *entry); } static ASWeakMap *cache = nil; -static ASDN::Mutex cacheLock; +// 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 { { - ASDN::MutexLocker l(cacheLock); + ASDN::StaticMutexLocker l(cacheLock); if (!cache) { cache = [[ASWeakMap alloc] init]; } @@ -464,7 +465,7 @@ static ASDN::Mutex cacheLock; } { - ASDN::MutexLocker l(cacheLock); + ASDN::StaticMutexLocker l(cacheLock); return [cache setObject:contents forKey:key]; } } diff --git a/Source/ASTextNode.mm b/Source/ASTextNode.mm index 18f0198efc..6caaec8cd9 100644 --- a/Source/ASTextNode.mm +++ b/Source/ASTextNode.mm @@ -1384,7 +1384,8 @@ static NSAttributedString *DefaultTruncationAttributedString() } #endif -static ASDN::Mutex _experimentLock; +// 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,7 +1393,7 @@ static BOOL _hasAllocatedNode; { static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ - ASDN::MutexLocker lock(_experimentLock); + ASDN::StaticMutexLocker lock(_experimentLock); // They must call this before allocating any text nodes. ASDisplayNodeAssertFalse(_hasAllocatedNode); @@ -1427,7 +1428,7 @@ static BOOL _hasAllocatedNode; { static dispatch_once_t onceToken; dispatch_once(&onceToken, ^{ - ASDN::MutexLocker lock(_experimentLock); + ASDN::StaticMutexLocker lock(_experimentLock); _hasAllocatedNode = YES; }); diff --git a/Source/ASTextNode2.mm b/Source/ASTextNode2.mm index 48ff3c4bb9..ecd9496c51 100644 --- a/Source/ASTextNode2.mm +++ b/Source/ASTextNode2.mm @@ -376,7 +376,8 @@ static NSArray *DefaultLinkAttributeNames = @[ NSLinkAttributeName ]; text:(NSAttributedString *)text { - static ASDN::Mutex layoutCacheLock; + // 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,7 +385,7 @@ static NSArray *DefaultLinkAttributeNames = @[ NSLinkAttributeName ]; }); ASTextCacheValue *cacheValue = ({ - ASDN::MutexLocker lock(layoutCacheLock); + ASDN::StaticMutexLocker lock(layoutCacheLock); cacheValue = [textLayoutCache objectForKey:text]; if (cacheValue == nil) { cacheValue = [[ASTextCacheValue alloc] init]; diff --git a/Source/Details/ASBasicImageDownloader.mm b/Source/Details/ASBasicImageDownloader.mm index 57fb412144..0d80842654 100644 --- a/Source/Details/ASBasicImageDownloader.mm +++ b/Source/Details/ASBasicImageDownloader.mm @@ -46,11 +46,12 @@ NSString * const kASBasicImageDownloaderContextCompletionBlock = @"kASBasicImage @implementation ASBasicImageDownloaderContext static NSMutableDictionary *currentRequests = nil; -static ASDN::RecursiveMutex currentRequestsLock; +// 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 { - ASDN::MutexLocker l(currentRequestsLock); + ASDN::StaticMutexLocker l(currentRequestsLock); if (!currentRequests) { currentRequests = [[NSMutableDictionary alloc] init]; } @@ -64,7 +65,7 @@ static ASDN::RecursiveMutex currentRequestsLock; + (void)cancelContextWithURL:(NSURL *)URL { - ASDN::MutexLocker l(currentRequestsLock); + ASDN::StaticMutexLocker l(currentRequestsLock); if (currentRequests) { [currentRequests removeObjectForKey:URL]; } diff --git a/Source/Details/ASThread.h b/Source/Details/ASThread.h index ca049d72a4..989db9a26c 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.) @@ -65,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) @@ -142,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 (); } @@ -217,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 @@ -232,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. @@ -297,18 +291,19 @@ 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())); @@ -320,8 +315,8 @@ namespace ASDN { pthread_mutex_t *mutex () { return &_m; } - StaticMutex(const StaticMutex&) = delete; - StaticMutex &operator=(const StaticMutex&) = delete; + private: + pthread_mutex_t _m; }; typedef Locker StaticMutexLocker; diff --git a/Source/TextKit/ASTextKitContext.mm b/Source/TextKit/ASTextKitContext.mm index 64fcf65092..f8b3d15655 100755 --- a/Source/TextKit/ASTextKitContext.mm +++ b/Source/TextKit/ASTextKitContext.mm @@ -40,8 +40,9 @@ { 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); + // 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();