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

nguyenhuy · 2017-09-20T18:42:57Z

This is an interesting crash. Here is an example report in Pinterest app, happening on the static cacheLock mutex of ASImageNode:

There are other similar crashes related to __staticMutex in ASTextKitContext reported in #136. For instance, see thread #0 and #5 in this report, or thread #0 and #14 here.

Basically, what's happening is that all static mutexes are destroyed when an app is terminated by the system. At the same time, a static mutex is being unlocked or destroyed on another thread. The unlock or destruction yields a non-zero error code, which triggers an assertion, even in production.

A similar issue was discussed and fixed in Realm-Core (realm/realm-core#2243). For Texture, I went with a different approach that is arguably safer, IMHO. This should fix #136.

Mutex initialization using the PTHREAD_MUTEX_INITIALIZER does not immediately initialize the mutex. Instead, on first use, pthread_mutex_timedlock_np() or pthread_mutex_lock() or pthread_mutex_trylock() branches into a slow path and causes the initialization of the mutex. Because a mutex is not just a simple memory object and requires that some resources be allocated by the system, an attempt to call pthread_mutex_destroy() or pthread_mutex_unlock() on a mutex that was statically initialized using PTHREAD_MUTEX_INITIALIZER and was not yet locked causes an EINVAL error.

(https://www.ibm.com/support/knowledgecenter/en/ssw_i5_54/apis/users_65.htm)

…le the app is being terminated

nguyenhuy · 2017-09-20T19:11:26Z

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;


Not sure why this is a recursive mutex in the first place.

I hope for no good reason! Otherwise deadlocks…

3a4oT · 2017-09-20T19:14:20Z

@nguyenhuy thanks for the agenda and patch, very interesting. I still not understand how it triggers the assertion. Just two questions: 1 Have you reproduced it? And when do you plan next release?;)

Adlai-Holler

Wow it's awesome to know what is going on here.

What do you think about changing assert in ASDISPLAYNODE_THREAD_ASSERT_ON_ERROR into NSCAssert so that we won't get crashes in production? assert is enabled unless NDEBUG is defined – which most apps don't do.

garrettmoon

@nguyenhuy Since this is a failure case when the app exits I'm wondering if there's a solution which doesn't pollute client (to the lock) code. It seems that there is no user facing cost to these crashes?

While this is more formally correct, the react solution seems cleaner and likely has the same user facing impacts?

garrettmoon · 2017-09-20T19:56:53Z

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;


I hope for no good reason! Otherwise deadlocks…

maicki

Awesome work @nguyenhuy. I would agree with @garrettmoon. The realm solution seems may a bit more simpler and we can repeat this pattern in other places if this repeats.
Otherwise it may makes it harder to change code were we handling the unlock of the lock.

…p exit

nguyenhuy · 2017-09-21T15:05:36Z

@3a4oT To answer your questions:

If a static mutex has been destroyed on the main thread during an app exit, calling pthread_mutex_unlock() to unlock it on a background thread causes the method to return a non-zero value. The value then triggers the assertion here.
No, I have not been able to reproduce it.
Soon™ :P

nguyenhuy · 2017-09-21T15:08:49Z

@Adlai-Holler Good idea. I went ahead and convert all assertions in ASThread to ASDisplayNodeCAssert.

@garrettmoon @maicki Thinking again, my approach requires more effort on client-side and so it is not scalable. I changed to the approach used in Realm.

nguyenhuy · 2017-09-21T15:10:34Z

Source/Details/ASBasicImageDownloader.mm

@@ -46,11 +46,12 @@ @interface ASBasicImageDownloaderContext ()
 @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;


I double-checked the code here and I'm 99% sure that this mutex doesn't need to be recursive.

^ @garrettmoon

nguyenhuy · 2017-09-22T10:48:45Z

Merging. Thanks, everyone!

…le the app is being terminated (TextureGroup#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()

Fix crashes caused by failing to unlock or destroy a static mutex whi…

cc59301

…le the app is being terminated

nguyenhuy commented Sep 20, 2017

View reviewed changes

Adlai-Holler reviewed Sep 20, 2017

View reviewed changes

garrettmoon reviewed Sep 20, 2017

View reviewed changes

maicki reviewed Sep 20, 2017

View reviewed changes

nguyenhuy added 2 commits September 21, 2017 15:43

Allocate static mutexes on the heap memory to avoid destruction at ap…

ea74bf2

…p exit

ASThread to use ASDisplayNodeCAssert() instead of assert()

c8096b4

nguyenhuy commented Sep 21, 2017

View reviewed changes

garrettmoon approved these changes Sep 21, 2017

View reviewed changes

nguyenhuy merged commit 8e0aa1e into TextureGroup:master Sep 22, 2017

This was referenced Sep 28, 2017

Crash when restoring from lock screen. #325

Closed

[Crash] ASDN::Mutex::~Mutex() #136

Closed

This was referenced Nov 29, 2017

We got some crashes: "ASDN::Mutex::~Mutex()" #688

Closed

ASDN::SharedLocker destructor crash #692

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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 #577

nguyenhuy commented Sep 20, 2017 •

edited

Loading

nguyenhuy Sep 20, 2017

garrettmoon Sep 20, 2017

3a4oT commented Sep 20, 2017

Adlai-Holler left a comment

garrettmoon left a comment

garrettmoon Sep 20, 2017

maicki left a comment •

edited

Loading

nguyenhuy commented Sep 21, 2017

nguyenhuy commented Sep 21, 2017 •

edited

Loading

nguyenhuy Sep 21, 2017

nguyenhuy Sep 21, 2017

garrettmoon Sep 21, 2017

nguyenhuy commented Sep 22, 2017

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 #577

Conversation

nguyenhuy commented Sep 20, 2017 • edited Loading

nguyenhuy Sep 20, 2017

Choose a reason for hiding this comment

garrettmoon Sep 20, 2017

Choose a reason for hiding this comment

3a4oT commented Sep 20, 2017

Adlai-Holler left a comment

Choose a reason for hiding this comment

garrettmoon left a comment

Choose a reason for hiding this comment

garrettmoon Sep 20, 2017

Choose a reason for hiding this comment

maicki left a comment • edited Loading

Choose a reason for hiding this comment

nguyenhuy commented Sep 21, 2017

nguyenhuy commented Sep 21, 2017 • edited Loading

nguyenhuy Sep 21, 2017

Choose a reason for hiding this comment

nguyenhuy Sep 21, 2017

Choose a reason for hiding this comment

garrettmoon Sep 21, 2017

Choose a reason for hiding this comment

nguyenhuy commented Sep 22, 2017

nguyenhuy commented Sep 20, 2017 •

edited

Loading

maicki left a comment •

edited

Loading

nguyenhuy commented Sep 21, 2017 •

edited

Loading