Avoid a lockup when adding an entry to the catalog. Fixes #1388

[pjrobertson]

Use runOnMainQueueSync instead of blindly trying to run the block on the main thread

The lock would only occur if you have 'run tasks and actions in the background' disabled in the extras prefs.

Delete this branch when it's merged, I might be on a plane somewhere :D

[pjrobertson]

Fixes #1388

[pjrobertson]

I've added another commit which (hopefully) fixes one of the two remaining crash reports we're getting (yay!)

Reasoning behind why these might fix the code is from Mike Ash:

-[NSImage imageNamed:], followed by mutation
This call is very convenient, but it returns a shared object. A lot of code will make this call, and then immediately start resizing or drawing into the object that it returns. This can mess up the shared image for any other code that uses it. If you're going to modify the image, then you need to copy it first, but this isn't immediately obvious.

OK, we're not using imageNamed here, but the object icon is really a 'shared icon' (used all over the places in different forms) so we should make a copy of it before playing with it. We should probably copy all images obtained with imageNamed

Also, retaining the drawObject is just seen as a good idea from a threading/memory management point of view. From Apple's docs (search for 'Be Aware of Threats to Code Correctness')

The problem with the code is not that the critical region was poorly defined, but that the actual problem was not understood. The real problem is a memory management issue that is triggered only by the presence of other threads. Because it can be released by another thread, a better solution would be to retain anObject before releasing the lock. This solution addresses the real problem of the object being released and does so without introducing a potential performance penalty.

So this is perhaps an easy and less performance-affecting way of solving threading problems

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: _numberOfColorComponentsNotIncludingAlpha


Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libobjc.A.dylib                 0x00007fff8a6cf24c objc_msgSend + 12
1   com.apple.AppKit                0x00007fff8836ff20 -[NSImage bestRepresentationAmongRepresentations:forRect:context:hints:] + 988
2   com.apple.AppKit                0x00007fff8836f693 -[NSImage bestRepresentationForRect:context:hints:] + 398
3   com.blacktree.QSFoundation      0x000000010005635e -[NSImage(Scaling) bestRepresentationForSize:] + 80
4   com.blacktree.QSInterface       0x000000010016dab5 -[QSObjectCell drawObjectImage:inRect:cellFrame:controlView:flipped:opacity:] + 417
5   com.blacktree.QSInterface       0x000000010016cb29 -[QSObjectCell drawIconForObject:withFrame:inView:] + 428
6   com.blacktree.QSInterface       0x000000010016c93f -[QSObjectCell drawInteriorWithFrame:inView:] + 212
7   com.blacktree.QSInterface       0x000000010016c07d -[QSObjectCell drawWithFrame:inView:] + 974
8   com.apple.AppKit                0x00007fff8848963e -[NSTableView drawRow:clipRect:] + 2131
9   com.blacktree.QSInterface       0x000000010017cd04 -[QSTableView drawRow:clipRect:] + 352
10  com.apple.AppKit                0x00007fff884794c3 -[NSTableView drawRowIndexes:clipRect:] + 397
11  com.apple.AppKit                0x00007fff88477ea6 -[NSTableView drawRect:] + 1269

[skurfer]

OK, we're not using imageNamed here, but the object icon is really a 'shared icon' (used all over the places in different forms) so we should make a copy of it before playing with it. We should probably copy all images obtained with imageNamed

Seems like a good way to chew up memory. :-) I'd say only copy in the places where a change is made.

I read that article and thought the NSBundle thing would kill us (for catalog scanning and icon loading), but I actually don't see any examples of us misusing it. Surely there are plug-ins affected, though.

Also, retaining the drawObject is just seen as a good idea from a threading/memory management point of view.

Agreed. I wondered if we should be doing that in other places, like retaining a QSObject while its icon is being loaded in the background, but the block itself seems to increase the retain count.

Anyway, changes here look good.

[tiennou]

Seems like a good way to chew up memory. :-) I'd say only copy in the places where a change is made.

Maybe the NSImage system is clever enough to handle copy-on-write ;-). This should be checked, I'd prefer all code to go through a [[NSImage imageNamed:@"blah"] copy] than remember I have to add the necessary copy before modifying it...

[pjrobertson]

Maybe the NSImage system is clever enough to handle copy-on-write ;-). This should be checked, I'd prefer all code to go through a [[NSImage imageNamed:@"blah"] copy] than remember I have to add the necessary copy before modifying it...

Agreed. Copying is preferable to having to remember. I've just done a quick comparison on the memory usage after loading /usr/bin with this branch and master, and it seems that this does increase memory usage quite a bit (maybe up to 10MB) so should we remove the copy?

[skurfer]

Not really sure what the code is doing, but it looks like icon isn't changed unless (!handlerDraw), so maybe we should only copy there instead of unconditionally? I'm suspicious of any dealing with specific representations now anyway, after making the retina branch changes, they mostly seem unnecessary. What if we just don't change icon at all?

[pjrobertson]

OK, I've reverted that commit and added another one. I've taken out all the bestRepresentation code so none of the icons are ever modified. In doing so I've set the imageInterpolation to NSImageInterpolationHigh all the time, I haven't noticed a performance penalty.