We shouldn't be using NSBundle as much as we are (not thread safe)

[pjrobertson]

NSBundle is not thread safe, I've just seen a crash because of it.

Of interest is: http://code.google.com/p/chromium/issues/detail?id=24842
We use it in so many places I know it'll be difficult to root out.

Process:         Quicksilver [300]
Path:            /Applications/Quicksilver.app/Contents/MacOS/Quicksilver
Identifier:      com.blacktree.Quicksilver
Version:         1.2.0 (400B)
Code Type:       X86-64 (Native)
Parent Process:  launchd [171]
Responsible:     Quicksilver [300]
User ID:         501

Date/Time:       2014-05-30 07:17:31.760 -0500
OS Version:      Mac OS X 10.9.3 (13D65)
Report Version:  11
Anonymous UUID:  3BC858F8-D475-A7A4-EC45-EDB2C2E362F1

Sleep/Wake UUID: 060963B5-72EA-4A41-855F-6B9BADFA6523

Crashed Thread:  9  Dispatch queue: com.apple.root.high-priority

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: EXC_I386_GPFLT

Application Specific Information:
objc_msgSend() selector name: iconForAction:

...

Thread 6:: Dispatch queue: com.apple.CoreUI.ThemeRef-cache
0   libsystem_kernel.dylib              0x00007fff911e872e __psynch_mutexdrop + 10
1   libsystem_pthread.dylib             0x00007fff91d95928 pthread_mutex_unlock + 113
2   com.apple.CoreFoundation            0x00007fff92bb88bf CFBundleGetBundleWithIdentifier + 271
3   com.apple.Foundation                0x00007fff8f77be94 +[NSBundle bundleWithIdentifier:] + 29
4   com.apple.coreui                    0x00007fff95cae15e __RunTimeThemeRefForBundleIdentifierAndName_block_invoke + 119
5   libdispatch.dylib                   0x00007fff9784628d _dispatch_client_callout + 8
6   libdispatch.dylib                   0x00007fff97847146 _dispatch_barrier_sync_f_invoke + 39
7   com.apple.coreui                    0x00007fff95ca9ae9 PerformBlockWithThemeRefCache + 102
8   com.apple.coreui                    0x00007fff95ca861e RunTimeThemeRefForBundleIdentifierAndName + 289
9   com.apple.coreui                    0x00007fff95ca96bc +[CUIThemeFacet themeNamed:forBundleIdentifier:error:] + 60
10  com.apple.coreui                    0x00007fff95cd4ba5 -[CUICatalog initWithName:fromBundle:error:] + 149
11  com.apple.coreui                    0x00007fff95cd4a37 __40+[CUICatalog defaultUICatalogForBundle:]_block_invoke21 + 132
12  libdispatch.dylib                   0x00007fff9784628d _dispatch_client_callout + 8
13  libdispatch.dylib                   0x00007fff97847146 _dispatch_barrier_sync_f_invoke + 39
14  com.apple.coreui                    0x00007fff95cd48c1 +[CUICatalog defaultUICatalogForBundle:] + 210
15  com.apple.AppKit                    0x00007fff91229aec +[NSImage _catalogImageWithName:bundle:] + 101
16  com.apple.AppKit                    0x00007fff91223eed +[NSImage imageNamed:] + 261
17  com.blacktree.QSCore                0x000000010f177f81 -[QSActionHandler loadIconForObject:] + 456

...

Thread 9 Crashed:: Dispatch queue: com.apple.root.high-priority
0   libobjc.A.dylib                     0x00007fff959d8097 objc_msgSend + 23
1   com.blacktree.QSCore                0x000000010f177f81 -[QSActionHandler loadIconForObject:] + 456
2   libdispatch.dylib                   0x00007fff978491bb _dispatch_call_block_and_release + 12
3   libdispatch.dylib                   0x00007fff9784628d _dispatch_client_callout + 8
4   libdispatch.dylib                   0x00007fff97848082 _dispatch_root_queue_drain + 326
5   libdispatch.dylib                   0x00007fff97849177 _dispatch_worker_thread2 + 40
6   libsystem_pthread.dylib             0x00007fff91d93ef8 _pthread_wqthread + 314
7   libsystem_pthread.dylib             0x00007fff91d96fb9 start_wqthread + 13

Thread 10:
0   libsystem_kernel.dylib              0x00007fff911e8e6a __workq_kernreturn + 10
1   libsystem_pthread.dylib             0x00007fff91d93f08 _pthread_wqthread + 330
2   libsystem_pthread.dylib             0x00007fff91d96fb9 start_wqthread + 13

Thread 11:: Dispatch queue: com.apple.root.high-priority
0   libsystem_kernel.dylib              0x00007fff911e8746 __psynch_mutexwait + 10
1   libsystem_pthread.dylib             0x00007fff91d95779 _pthread_mutex_lock + 372
2   com.apple.CoreFoundation            0x00007fff92b75215 _CFBundleCopyBundleForURL + 37
3   com.apple.CoreFoundation            0x00007fff92b7454f _CFBundleCreate + 175
4   com.apple.Foundation                0x00007fff8f77376a -[NSBundle _cfBundle] + 77
5   com.apple.Foundation                0x00007fff8f783459 -[NSBundle bundlePath] + 96
6   com.blacktree.QSCore                0x000000010f1b6111 -[QSResourceManager pathWithLocatorInformation:] + 954
7   com.blacktree.QSCore                0x000000010f1b631b -[QSResourceManager imageWithLocatorInformation:] + 230
8   com.blacktree.QSCore                0x000000010f1b59ca -[QSResourceManager imageNamed:inBundle:] + 466
9   com.blacktree.QSCore                0x000000010f1b526a +[QSResourceManager imageNamed:inBundle:] + 98
10  com.blacktree.QSCore                0x000000010f177ec1 -[QSActionHandler loadIconForObject:] + 264
11  libdispatch.dylib                   0x00007fff978491bb _dispatch_call_block_and_release + 12
12  libdispatch.dylib                   0x00007fff9784628d _dispatch_client_callout + 8
13  libdispatch.dylib                   0x00007fff97848082 _dispatch_root_queue_drain + 326
14  libdispatch.dylib                   0x00007fff97849177 _dispatch_worker_thread2 + 40
15  libsystem_pthread.dylib             0x00007fff91d93ef8 _pthread_wqthread + 314
16  libsystem_pthread.dylib             0x00007fff91d96fb9 start_wqthread + 13

Maybe in this case the real solution is to make sure that all images are loaded on the same thread.

[pjrobertson]

P.S. I think I've hit the nail on my head with my last comment:

Maybe in this case the real solution is to make sure that all images are loaded on the same thread.

Looking at the crash reports, about 60% of our crashes come from something to do with -[QSObject loadIcon].
My solution would be to create our own serial dispatch_queue for icon loading, but then that means icons are loaded synchronously, which isn't very nice. @tiennou - do you have any other suggestions, since I know you like dispatch_barriers and the like.

[pjrobertson]

Set milestone to v1.2.0 (@skurfer) since the nasty commit is d6180e9 - and I think it might be new to v1.2.0

[skurfer]

I’m pretty sure it’s in 1.1.x too, but I’m OK with holding up 1.2.0 if it fixes 60% of the crashes.

I knew it wasn’t thread safe from one of Mike Ash’s Friday Q&A’s, but wasn’t sure of it was causing us any problems or what to do about it.

Do you think all icon loading needs to be on the same thread, or just the NSBundle-using code in QSResourceManager?

[pjrobertson]

Do you think all icon loading needs to be on the same thread, or just the NSBundle-using code in QSResourceManager?

From that crash I've posted, probably just the NSBundle stuff, but then you look at the other 43% of crash logs (I've adjusted my figure ;-) ) and they're not all related to NSBundle.

Does it all need to be on the same thread? Probably not. Should the code be running on a high priority queue? Definitely not. Should we be using dispatch_barrier instead? Probably - but I don't know how :/ ( @tiennou !)

---

Some stats:

For 400B:
find . | grep -c '' --> 104
grep -lr 'loadIcon' ./* | grep -c '' --> 45

If you expand the ... in that commit I linked to, you'll see that the commit is in 1.1.0 and 1.1.0-pre, so you're right. Here's some stats on 4007 showing 67% of crashes are related to loadIcon :(

find . | grep -c '' --> 896
grep -lr 'loadIcon' ./* | grep -c '' --> 598

---

An example of a crash unrelated to NSBundle (I'm pretty sure the high-priority thread 4 is also loading an icon:

Crashed Thread:  0  Dispatch queue: com.apple.main-thread

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: EXC_I386_GPFLT

Application Specific Information:
objc_msgSend() selector name: isEqual:


Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libobjc.A.dylib                     0x00007fff96e04097 objc_msgSend + 23
1   com.apple.CoreFoundation            0x00007fff9292e898 -[__NSSetM addObject:] + 408
2   com.blacktree.QSCore                0x000000010f4bc8ad -[QSObject(Icon) loadIcon] + 94
3   com.blacktree.QSInterface           0x000000010f54bb4f -[QSObjectView setObjectValue:] + 68
4   com.blacktree.QSInterface           0x000000010f553157 -[QSSearchObjectView selectObjectValue:] + 305
5   com.blacktree.QSInterface           0x000000010f553411 -[QSSearchObjectView selectIndex:] + 196
6   com.blacktree.QSInterface           0x000000010f545145 -[QSInterfaceController updateActionsNow] + 242
7   com.apple.Foundation                0x00007fff8aa7d0f4 __NSFireTimer + 96
8   com.apple.CoreFoundation            0x00007fff92976494 __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ + 20
9   com.apple.CoreFoundation            0x00007fff92975fcf __CFRunLoopDoTimer + 1151
10  com.apple.CoreFoundation            0x00007fff929e75aa __CFRunLoopDoTimers + 298
11  com.apple.CoreFoundation            0x00007fff92931755 __CFRunLoopRun + 1525
12  com.apple.CoreFoundation            0x00007fff92930f25 CFRunLoopRunSpecific + 309
13  com.apple.HIToolbox                 0x00007fff98246a0d RunCurrentEventLoopInMode + 226
14  com.apple.HIToolbox                 0x00007fff982467b7 ReceiveNextEventCommon + 479
15  com.apple.HIToolbox                 0x00007fff982465bc _BlockUntilNextEventMatchingListInModeWithFilter + 65
16  com.apple.AppKit                    0x00007fff956d026e _DPSNextEvent + 1434
17  com.apple.AppKit                    0x00007fff956cf8bb -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 122
18  com.apple.AppKit                    0x00007fff956c39bc -[NSApplication run] + 553
19  com.apple.AppKit                    0x00007fff956ae7a3 NSApplicationMain + 940
20  com.blacktree.Quicksilver           0x000000010f3c89d4 0x10f3c7000 + 6612

... 

Thread 4:: Dispatch queue: com.apple.root.high-priority
0   libobjc.A.dylib                     0x00007fff96e1b835 (anonymous namespace)::AutoreleasePoolPage::push() + 27
1   libdispatch.dylib                   0x00007fff9086bf7b _dispatch_root_queue_drain + 63
2   libdispatch.dylib                   0x00007fff9086d177 _dispatch_worker_thread2 + 40
3   libsystem_pthread.dylib             0x00007fff90b38ef8 _pthread_wqthread + 314
4   libsystem_pthread.dylib             0x00007fff90b3bfb9 start_wqthread + 13

Thread 5:: Dispatch queue: com.apple.root.low-priority
0   libsystem_kernel.dylib              0x00007fff94675a1a mach_msg_trap + 10
1   libsystem_kernel.dylib              0x00007fff94674d18 mach_msg + 64
2   libdispatch.dylib                   0x00007fff908702f6 _dispatch_send_wakeup_runloop_thread + 68
3   libdispatch.dylib                   0x00007fff90870167 _dispatch_main_queue_wakeup + 43
4   libdispatch.dylib                   0x00007fff9086b1c4 _dispatch_wakeup + 66
5   libdispatch.dylib                   0x00007fff9086b788 _dispatch_queue_push_list_slow2 + 30
6   com.apple.AppKit                    0x00007fff956fb1c7 -[NSWindow _postWindowNeedsDisplayOrLayoutOrUpdateConstraintsUnlessPostingDisabled] + 497
7   com.apple.AppKit                    0x00007fff95e325c9 NSViewSetNeedsDisplayInRect + 647
8   com.blacktree.QSInterface           0x000000010f54e15e -[QSResultController iconLoader:loadedIndex:inArray:] + 221
9   com.blacktree.QSCore                0x000000010f4b46d5 __25-[QSIconLoader loadIcons]_block_invoke + 471
10  libdispatch.dylib                   0x00007fff9086d1bb _dispatch_call_block_and_release + 12
11  libdispatch.dylib                   0x00007fff9086a28d _dispatch_client_callout + 8
12  libdispatch.dylib                   0x00007fff9086c082 _dispatch_root_queue_drain + 326
13  libdispatch.dylib                   0x00007fff9086d177 _dispatch_worker_thread2 + 40
14  libsystem_pthread.dylib             0x00007fff90b38ef8 _pthread_wqthread + 314
15  libsystem_pthread.dylib             0x00007fff90b3bfb9 start_wqthread + 13

[pjrobertson]

I've thrown together a simple proof of concept of this. It shows that it's not as easy as I thought. There are so many threading issues with QSObject (esp. in dealloc)

That branch works, but icon loading is obviously slow

[pjrobertson]

Sorry for the bombardment... perhaps another solution (which I don't think will fix everything) is to use __weak. We should probably do this, no matter what solution we come up with

[craigotis]

I would be happy to help with whatever solution is chosen - I've got a good bit of experience with GCD. Your solution looks appropriate, @pjrobertson. What specifically is the issue regarding icon loading on multiple threads? Do they just need to synchronize at a specific point, or does the loading itself need to be completely isolated to a single thread? Using dispatch_barrier unfortunately won't be much help here - using it for all your icon-loading operations is nearly identical to simply doing it on a serial queue, but with the disadvantage of being more complicated, and of also locking out any other tasks that would have been happy to run concurrently on the queue you're occupying.

Do you see better performance by specifying a custom priority for your serial queue? (Using dispatch_set_target_queue())

Edit: Tweaking the serial queue to run on the high-priority global queue, I see no difference in performance.

[pjrobertson]

Hi @craigotis I appreciate the offer of help :)

The issue regarding icon loading and multiple threads... I'm not entirely sure :(
Basically icon loading is a multi-threading madness. I'm not sure if objects are getting released on one thread whilst having their icon created on another thread (shouldn't happen, since a __strong reference to self has always been passed to the icon loading code... although linking to that, I see that my __weak commit got accidentally included in master... oops! (@skurfer - I think it got merged with the 'Xcode 6 changes' pull)
I think the crashes are just coming from calling things like -[NSImage imageNamed:] on multiple threads at the same time... which, it turns out, is not thread safe (see the link to the Apple dev forums - he refers to UIImage but I assume it's the same as NSImage):

To extend what vfr said, in particular +[UIImage imageNamed:] is not thread safe. As of iOS 4.0 most of UIKit's graphics methods are thredsafe, but +imageNamed: is a notable exception.

Also see this

so you can see, the primary issue is using non thread-safe methods on a background thread (imageNamed and +[NSBundle bundleWithIdentifier:]). How we fix this... we can perhaps change all [NSImage imageNamed:] calls to [QSResourceManager imageNamed:] and then make that thread safe, but that doesn't fix the bundleWithIdentifier problem (although it might do, since that first crash I posted originates from two imageNamed: calls on two different threads)

Thoughts on the best way of making or calls to [NSImage imageNamed:] thread safe?

[pjrobertson]

Making QSResourceManager work on a single thread is probably the way to go. Perhaps we should make QSResourceManager a subclass of NSProxy, then forward ALL calls to QSResourceManagerA (or whatever) through forwardInvocation, making sure everything is running on only a single thread.

[pjrobertson]

NSProxy wasn't the way to go, since not all methods in the class need to be called on one thread.

See my new pull request, hopefully you'll agree that that is sufficient?