Fix crashes caused by failing to unlock or destroy a static mutex while the app is being terminated (#577)

* Fix crashes caused by failing to unlock or destroy a static mutex while the app is being terminated

* Allocate static mutexes on the heap memory to avoid destruction at app exit

* ASThread to use ASDisplayNodeCAssert() instead of assert()

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)

Source/ASImageNode.mm

@@ -442,12 +442,13 @@ typedef void (^ASImageNodeDrawParametersBlock)(ASWeakMapEntry *entry);
 }
 
 static ASWeakMap<ASImageNodeContentsKey *, UIImage *> *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];
   }
 }

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;
   });
   

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<NSAttributedString *, ASTextCacheValue *> *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];

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];
   }

Source/Details/ASThread.h

@@ -52,12 +52,6 @@ static inline BOOL ASDisplayNodeThreadIsMain()
 
 #include <memory>
 
-/**
- 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<T> 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);
+        ASDisplayNodeCAssertTrue(0 == _owner);
-        assert(0 == _count);
+        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<Mutex> MutexSharedUnlocker;
 
   /**
-   If you are creating a static mutex, use StaticMutex and specify its default value as one of ASDISPLAYNODE_MUTEX_INITIALIZER
+   If you are creating a static mutex, use StaticMutex. This avoids expensive constructor overhead at startup (or worse, ordering
-   or ASDISPLAYNODE_MUTEX_RECURSIVE_INITIALIZER. 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;
+  private:
-    StaticMutex &operator=(const StaticMutex&) = delete;
+    pthread_mutex_t _m;
   };
 
   typedef Locker<StaticMutex> StaticMutexLocker;

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;
+    // Allocate __staticMutex on the heap to prevent destruction at app exit (https://github.com/TextureGroup/Texture/issues/136)
-    ASDN::MutexLocker l(__staticMutex);
+    static ASDN::StaticMutex& __staticMutex = *new ASDN::StaticMutex;
+    ASDN::StaticMutexLocker l(__staticMutex);
     
     __instanceLock__ = std::make_shared<ASDN::Mutex>();