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

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,31 +442,48 @@ + (UIImage *)displayWithParameters:(id<NSObject>)parameter isCancelled:(asdispla
 }
 
 static ASWeakMap<ASImageNodeContentsKey *, UIImage *> *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

Source/ASTextNode.mm

@@ -1384,51 +1384,52 @@ + (void)_registerAttributedText:(NSAttributedString *)str
 }
 #endif
 
-static ASDN::Mutex _experimentLock;
+static ASDN::StaticMutex _experimentLock = ASDISPLAYNODE_MUTEX_INITIALIZER;
 static ASTextNodeExperimentOptions _experimentOptions;
 static BOOL _hasAllocatedNode;
 
 + (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
   });
 }
 
 + (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)

Source/ASTextNode2.mm

@@ -376,20 +376,22 @@ + (ASTextLayout *)compatibleLayoutWithContainer:(ASTextContainer *)container
                                            text:(NSAttributedString *)text
 
 {
-  static ASDN::Mutex layoutCacheLock;
+  static ASDN::StaticMutex layoutCacheLock = ASDISPLAYNODE_MUTEX_INITIALIZER;
   static NSCache<NSAttributedString *, ASTextCacheValue *> *textLayoutCache;
   static dispatch_once_t onceToken;
   dispatch_once(&onceToken, ^{
     textLayoutCache = [[NSCache alloc] init];
   });
 
   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;
   });
 

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.

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<StaticMutex> StaticMutexLocker;
-  typedef Unlocker<StaticMutex> StaticMutexUnlocker;
-
   struct Condition
   {
     Condition () {

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

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<ASDN::Mutex>();
 
@@ -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;
 }