Crash Fixes

[pjrobertson]

Crash fixes for a few crash logs that have popped up in the latest pre-release.
This is a branch on the Quicksilver repo so if anybody else has any fixes they can push to it… :)

[pjrobertson]

Sorry, I stupidly deleted the crash logs to try and be tidy, before putting them here.

The first commit fixes a crash in the dealloc method of that class. i can only assume because delegate was pointing to some strange memory address.

The second fixes a crash on mnem = [mnem lowercaseString]; (QSMnemonics.m L65). My reasoning is that the mnem was being released before here, so I retained/autoreleased it.
(For those who are silly like me - if it's nil, it doesn't cause a crash. [nil lowercaseString] is nil :) )

[tiennou]

3bfdb033afcf967d96d128b6b7d1f82cc8e753c7 won't fix anything, as ObjC objects are memset to 0 on allocation (which is equivalent of saying blah = nil) ;-). But I can foresee a crash if someone creates a SearchObjectView, and cause it to be released while icons are loading (as in a quick interface open-close). You should check that SOVs call -invalidate in its -dealloc, and -release + nil the icon loader delegate in there.

No opinion on the second though, I'd say move the retain + autorelease directly in the accessor since there are other places that depend on it not being released before ;-) (though I can't get how "before" can happen, unless the crash report had some threading weirdness).

[pjrobertson]

I can confirm you're right about 3bfdb03. I just debugged it and it is already nil.
I was thinking about an ARC feature that's slightly related. I think if you do NSArray *myArray; somewhere in your code, ARC will automatically make it NSArray *myArray = nil; which could potentially avoid crashes.

About your idea - it looks like the delegate to a QSIconLoader is a QSResultController (not a SOV) so shouldn't we need to do something in there? [resultIconLoader setDelegate:nil] or something like that? Since it's self, we can't really release it in dealloc (it's already been done) so the best we can do is make it nil?!

On the 2nd commit - good idea. I'll do that.

Once the QSApp.com backups are up, I'll fish out the crash reports (hopefully), so you can see them.

[tiennou]

Right, I have in mind that IconLoader delegates were SOVs but my brain was failing me ;-). You should make sure that ResultControllers invalidate their loaders (both the normal and child one), and that -[IconLoader invalidate] releases and nils the delegate (but check first that an icon loader can't be restarted once it's been invalidated).

Or mark delegate as _weak and drop 10.5 ;-).

[skurfer]

I'm looking through crash reports from last night and getting dragged down into the catalog scanning muck with you guys. Hope you're happy. :-)

entryContents

In -[QSLibrarian reloadSets:], since entryContents is now a local copy that doesn't exist anywhere else, can we drop @synchronized?

saveIndex

There were two reported crashes in -[QSCatalogEntry saveIndex]. Seems to be because writeArray contains nil. Not sure how that's possible, since

1. it only exists locally
2. it isn't mutable to begin with

Using arrayByPerformingSelector:, it looks like it's possible for writeArray to contain [NSNull null] (not nil). But in that case, writeToFile:atomically: should just fail silently.

In both cases, it looks like -[QSCatalogEntry scanAndCache] is also running at the same time, which attempts to replace contents, but again, there should be multiple reasons why that doesn't matter:

1. contents should be locked
2. contents isn't being modified, but replaced completely
3. We aren't even using contents. We're using writeArray.

Which makes me wonder if we really even need @synchronized(contents), but that's just overhead. Wouldn't explain the crash.

double free

In -[QSObject loadChildren], calling [childLoadedArray addObject:self] seems to trigger a "double free" somehow. Why would adding to a set ever result in something being released?

Unrelated thing I noticed while investigating: Is it necessary to create tempArray in purgeImagesAndChildrenOlderThan:? The array is enumerated immediately after creation in order to send a single message to each object. Why not just send the message when the objects are identified and not add them to an array at all? Seems wasteful.

[tiennou]

Unrelated thing I noticed while investigating: Is it necessary to create tempArray in purgeImagesAndChildrenOlderThan:? The array is enumerated immediately after creation in order to send a single message to each object. Why not just send the message when the objects are identified and not add them to an array at all? Seems wasteful.

I wrote that part so this one's easy : not doing it like that will cause "collection changed while enumerating" since -unloadIcon will remove the icon from the array you're, well, enumerating on ;-). At least that's what it did when I changed it in 1713e12 (it's been 4 years...).

[tiennou]

About the -saveIndex crashes : using @synchronized(contents) is useless if the pointer changes, because the lock is set on the memory address of it (at least that's what I remember @synchronized() is doing). So you should lock on the entry directly if you're swapping contents somewhere.

[skurfer]

not doing it like that will cause "collection changed while enumerating" since -unloadIcon will remove the icon from the array you're, well, enumerating on

Yeah, I figured that out. Thanks. :-)

About the -saveIndex crashes : using @synchronized(contents) is useless if the pointer changes, because the lock is set on the memory address of it (at least that's what I remember @synchronized() is doing). So you should lock on the entry directly if you're swapping contents somewhere.

We're enumerating over it with arrayByPerformingSelector:, but that method calls @synchronized(self) before enumeration starts, so it shouldn't be necessary here. The mystery is how nil can end up in the resulting array.

[skurfer]

History rewritten to bring in 58d0f25 and avoid a conflict. I've also removed the "pointless" commit.

My commit won't prevent any crashes, but should reduce overhead. Reasoning is in the commit message. Let me know if I'm off on any of it.

[skurfer]

RE: Quicksilver_2012-12-14-141255_Eowyn.crash

Basically, they got EXC_BAD_ACCESS (SIGSEGV) on isKindOfClass: in -[QSObject isEqual:].

What's the answer here? Should we test !anObject before doing anything?

[pjrobertson]

What's the answer here? Should we test !anObject before doing anything?

I don't think so, since it's not a nil pointer crash but an invalid pointer crash. That is, the object's been released, but the memory pointed to by the object's pointer is not nil.

I'm still thinking that QSObjectIconModified notifs aren't correctly being removed every time. and maybe an object is getting released before the notifs are sent

what I think may be happening is that the object is getting released after its icon is set, and the notification is sent.
Say you put a movie in QS's 1st pane, QuickLook generator will work away loading the movie. Then close the QS interface… a few seconds later the image loads, but the object doesn't exist.

Tbh, I don't really know. Maybe @tiennou will do a better job deciphering

---

Crap: not worth reading but kept for historical purposes ;-)

Looking at threads 7 and 8, it seems like the notif is being sent to the same QSResultController (?)… ok looking at threads 10, 13, 15 perhaps multiple QSObjectIconModified are being sent for different objects (as the icons become available) but

[pjrobertson]

And a quick comment on Orochi 12-12-12 crash: perhaps we should copy the array here as well (like I did for entryContents in QSLibrarian recently)
It's been the number 1 or 2 place I've seen catalog crashes

Exception Type:  EXC_CRASH (SIGABRT)
Exception Codes: 0x0000000000000000, 0x0000000000000000

Application Specific Information:
*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '*** -[NSMutableArray replaceObjectsInRange:withObjects:count:]: attempt to insert nil object from objects[5]'
abort() called
terminate called throwing an exception

Application Specific Backtrace 1:
0   CoreFoundation                      0x00007fff906ef0a6 __exceptionPreprocess + 198
1   libobjc.A.dylib                     0x00007fff931443f0 objc_exception_throw + 43
2   CoreFoundation                      0x00007fff906a33eb -[NSMutableArray replaceObjectsInRange:withObjects:count:] + 203
3   CoreFoundation                      0x00007fff906bc2a1 _CFAppendXML0 + 1681
4   CoreFoundation                      0x00007fff906bc3c9 _CFAppendXML0 + 1977
5   CoreFoundation                      0x00007fff906bc0cd _CFAppendXML0 + 1213
6   CoreFoundation                      0x00007fff906bb9d7 _CFPropertyListCreateXMLData + 199
7   Foundation                          0x00007fff933f1be5 -[NSArray(NSArray) writeToFile:atomically:] + 200
8   QSCore                              0x00000001000d4bf7 -[QSCatalogEntry saveIndex] + 218
9   libdispatch.dylib                   0x00007fff8faf0723 _dispatch_barrier_sync_f_invoke + 39
10  QSCore                              0x00000001000d4ffe -[QSCatalogEntry scanAndCache] + 181

[skurfer]

I'm still thinking that QSObjectIconModified notifs aren't correctly being removed every time. and maybe an object is getting released before the notifs are sent

Well, the result controller watches for all of them, so removing it as an observer isn't an option. I can see how it would be a problem if the object from the notification had been released, but the notification is now only posted in one place, and the object passed is self. For that to be a problem, wouldn't it mean self has already been dealloc'd? And if so, how could it post a notification? Why doesn't the call to updateIcon: on the freed object, which posts the notification, cause a crash first?

Also, should we have -[QSObject isEqual:] test (self == anObject) first, as it's probably the fastest test in there?

[skurfer]

And a quick comment on Orochi 12-12-12 crash: perhaps we should copy the array here as well (like I did for entryContents in QSLibrarian recently)

But how does the array end up containing nil at all? And what's to say the copy won't, too?

On a related note, I'm wondering why arrayByPerformingSelector goes to so much trouble to keep the count the same (by inserting [NSNull null]). What does it matter? I'm trying a build where those methods just return arrays with valid objects, ignoring the count of the original. FYI, in my testing, the presence of NSNull in an array does not crash with writeToFile:atomically:.

[tiennou]

(My connection is not sufficient for doing high-level things like fetching crash reports, so I'll comment instead ;-)).

An array can never contain nil, because you'd eat an exception immediately upon adding it. So it looks to me the crash is related to one object inside getting released and a dangling pointer left in its place, which will cause a crash on next access and thus will have a backtrace utterly unrelated. If you're happy enough to have reproducible, then use leaks (and MallocStackLogging and NSZombieEnabled, hopefully we got those to cover our backs ;-)) to divine who created this object and who released it. But you have to have a reproducible crash in the first place...
And creating copies won't fix anything (it will just make the crash happen on the -copy call, because you'll send it to a dangling pointer), unless they just don't blindly copy the pointer and perform the NSCopying thing (the rationale being that copying a dangling (not-yet-but-will-soon) pointer will get you another dangling pointer somewhere else, which will crash all the same and thus doesn't sound good to me ;-).

And arrayByPerformingSelector: works this way so you can easily use -dictionaryWithObjects:andKeys:. Changing the count prevents that.

[skurfer]

But you have to have a reproducible crash in the first place...

Unfortunately not, in this case. I even tried enabling that damned "child results list" for a while to see if it would happen for me, but no luck.

So… Time to switch to ARC? ;-)

And arrayByPerformingSelector: works this way so you can easily use -dictionaryWithObjects:andKeys:. Changing the count prevents that.

Makes sense. We should add a comment in there to explain that.

[pjrobertson]

Great, so those 'crash fixes' I made are pointless ;-)

So… can we do anything more? Should we just leave this and get another pre release build out?

[pjrobertson]

How about if we make the saveIndex method run on the new dispatch_queue I created?

[skurfer]

@pjrobertson on IRC:

When an object is removed from one of the fundamental collection classes, it is sent a release (rather than autorelease) message. If the collection was the only owner of the removed object, the removed object (heisenObject in the example ) is then immediately deallocated.

I had always assumed that objects were sent release when removed from collections. But in our case, the problem seems to be that something in the collection is being released elsewhere. So, yes, it would crash when the array gets freed (and tries to send release to all its members) but it never gets that far. Right?

Since we're talking about an array of dictionaries, I wonder if the released object isn't one of the dictionaries itself, but an object inside it somewhere. (Not that this helps narrow it down at all.)

[pjrobertson]

I think this is the only pull we're waiting on for release right?

Since we can't figure out how to actually fix the catalog fixes, shall we merge what we have and release another ß71 preview (hopefully the final)

[pjrobertson]

Sorry, I forgot I had a few more 'fixes' lying around. I've pushed 2 more commits.
The 1st doesn't fix any known crashes, but I thought it would be neater to use an @Synthesized atomic setter/getter, since contents is known to be problematic with threads.

The 2nd attempts to fix the following crash:

Process:         Quicksilver [8308]
Path:            /Applications/Quicksilver.app/Contents/MacOS/Quicksilver
Identifier:      com.blacktree.Quicksilver
Version:         ß71 (3937)
Code Type:       X86-64 (Native)
Parent Process:  ??? [1]
User ID:         501

Date/Time:       2012-12-14 18:16:57.006 -0600
OS Version:      Mac OS X 10.8.2 (12C60)
Report Version:  10

Crashed Thread:  16  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: setStatus:
…
Thread 16 Crashed:: Dispatch queue: com.apple.root.high-priority
0   libobjc.A.dylib                 0x00007fff8dfb6250 objc_msgSend + 16
1   com.blacktree.QSCore            0x00000001000fb4e5 -[QSTaskController updateTask:status:progress:] + 45
2   com.blacktree.QSCore            0x00000001000ebced -[QSURLObjectHandler loadChildrenForObject:] + 55
3   com.blacktree.QSCore            0x00000001000e16d8 -[QSObject(Hierarchy) loadChildren] + 50
4   com.blacktree.QSCore            0x00000001000e1ac9 -[QSObject(Accessors) children] + 51
5   com.apple.CoreFoundation        0x00007fff8dd4c63c __invoking___ + 140
6   com.apple.CoreFoundation        0x00007fff8dd4c4d7 -[NSInvocation invoke] + 263
7   com.apple.CoreFoundation        0x00007fff8dd4c6a9 -[NSInvocation invokeWithTarget:] + 57
8   com.apple.CoreFoundation        0x00007fff8dd47737 ___forwarding___ + 775
9   com.apple.CoreFoundation        0x00007fff8dd473b8 _CF_forwarding_prep_0 + 232
10  com.blacktree.QSInterface       0x0000000100165ff3 -[QSResultController objectIconModified:] + 296
11  com.apple.CoreFoundation        0x00007fff8dd0b47a _CFXNotificationPost + 2554
12  com.apple.Foundation            0x00007fff8e135846 -[NSNotificationCenter postNotificationName:object:userInfo:] + 64
13  com.blacktree.QSCore            0x00000001000e3a2c -[QSFileSystemObjectHandler previewIcon:] + 891
14  libdispatch.dylib               0x00007fff8b5acf01 _dispatch_call_block_and_release + 15
15  libdispatch.dylib               0x00007fff8b5a90b6 _dispatch_client_callout + 8
16  libdispatch.dylib               0x00007fff8b5aa1fa _dispatch_worker_thread2 + 304
17  libsystem_c.dylib               0x00007fff8cb7bcab _pthread_wqthread + 404
18  libsystem_c.dylib               0x00007fff8cb66171 start_wqthread + 13

[pjrobertson]

OK so I've had another think about this crash:

*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '*** -[NSMutableArray replaceObjectsInRange:withObjects:count:]: attempt to insert nil object from objects[5]'
abort() called

Still can't really figure it out, but the nil is coming from a C Array, not and NSArray, which is perfectly legal. Don't ask me where the C Array is coming from (foundation stuff), but either one of the elements of the C array is getting set to nil at some point, or we're overrunning the length of the array.
I thought that perhaps [NSNull null] got converted to nil in a C Array, but it's not the case.

Doesn't really help us towards figuring out the problem, but we know that nil is perfectly legal in a C Array, so it explains the crash a little more.

See my sample code:
http://cl.ly/0x2m0F441U2q

---

Just a though: where we're getting crashes, could we just wrap them in @try @catch blocks? Probably not the true solution, but we can at least deal with the problem gracefully instead of crashing

[skurfer]

Well, we clearly aren't manipulating the C array, so I don't know where this is coming from. Yes, I think we should catch the exception. Maybe log some info that would help us see what is in the array?

FYI, I got tired of creating full-blown Xcode projects for little stand-alone tests like that. I saw a tip from a guy at Big Nerd Ranch. Now I just have a directory full of single-purpose .m files that I can run with this script:

#!/bin/zsh
EXE=$1:r
clang -Wall -framework Foundation -framework CoreServices $1 -o $EXE
$EXE

If you keep it simple enough (use older syntax), you can just do ⌘R in TextMate. I assume it's using GCC and could be configured to use LLVM somehow, but haven't dug into it.

[pjrobertson]

Alright, I'll add a try catch in there.

FYI, I got tired of creating full-blown Xcode projects for little
stand-alone tests like that…

Thanks for that tie bit. Useful :)

On 18 December 2012 18:50, Rob McBroom notifications@github.com wrote:

Well, we clearly aren't manipulating the C array, so I don't know where
this is coming from. Yes, I think we should catch the exception. Maybe log
some info that would help us see what is in the array?

FYI, I got tired of creating full-blown Xcode projects for little
stand-alone tests like that. I saw a tip from a guy at Big Nerd Ranch. Now
I just have a directory full of single-purpose .m files that I can run
with this script:

#!/bin/zshEXE=$1:r
clang -Wall -framework Foundation -framework CoreServices $1 -o $EXE$EXE

If you keep it simple enough (use older syntax), you can just do ⌘R in
TextMate. I assume it's using GCC and could be configured to use LLVM
somehow, but haven't dug into it.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/1279#issuecomment-11499727.

[pjrobertson]

OK, another commit added

[skurfer]

Great. I'll try it out for a while and get another release out.

[pjrobertson]

RE the following crash, @HenningJ had this crash, but couldn't reproduce with a fresh build of the release branch. I just tried to update from 3936→3937 on 10.7.5 and got the crash. I made sure that I needed admin rights to write over my old app.

Possible reasons:

• A problem with the 3936 build
• Code signing messes things up

We can hopefully check for the crash again going from 3937→3938 (next pre).
Maybe Rob you'll be able to reproduce it and test it on your machine with a code-signed debug version of the app(if you make sure /Applications/Quicksilver.app requires admin rights (here's what I have on my 10.7 machine:
drwxr-xr-x@ 504 staff ))
If not, feel free to send a debug & code signed version of the app to me/Henning.

Crash:

EXC_BAD_INSTRUCTION
Thread 0 Crashed:  Dispatch queue: com.apple.main-thread
0   com.blacktree.QSFoundation      0x000645e3 +[SUPlainInstaller copyPathWithAuthentication:overPath:temporaryName:error:] + 616

[tiennou]

Could this be simplified to just [entryContents count] since 0 == nil ?

[skurfer]

Maybe Rob you'll be able to reproduce it and test it on your machine with a code-signed debug version of the app

I'll try. For some reason, other users are never able to run a copy I build under my account, so it could be difficult. I usually just copy it out of the DMG as the other user, but Debug = no DMG. I'll figure something out, though.

[pjrobertson]

I'll try. For some reason, other users are never able to run a copy I
build under my account, so it could be difficult. I usually just copy it
out of the DMG as the other user, but Debug = no DMG. I'll figure something
out, though.

Yep, I've had that problem. I suggest you do the following, to save having
to use another account:

• Build a debug version (of 3936 say)
• Move it to /Applications
• Run sudo chown 504:staff /Applications/Quicksilver.app
• Use ⌘⇧< to change the executable file in the debug menu to the one at
  /Applications/Quicksilver.app
• Debug

On 19 December 2012 14:06, Rob McBroom notifications@github.com wrote:

I'll try. For some reason, other users are never able to run a copy I
build under my account, so it could be difficult. I usually just copy it
out of the DMG as the other user, but Debug = no DMG. I'll figure something
out, though.

[skurfer]

What I did:

• Checked out B71-pre (a.k.a. 3936)
• Built using Debug with code signing
• Made sure it would need admin rights: sudo cp -r /tmp/QS/build/Debug/Quicksilver.app /Users/Shared
• The problem turned out to be the permissions on Info.plist. I changed that to 644, leaving everything owned by root and then I was able to run the app. (The change in permissions didn't appear to bother Gatekeeper.)
• Switched to a non-admin user.
• Went through the update process.

No problems. I was asked to authenticate, and I ended up with a running copy of 3937.

Here's the build if you want to try it yourself.

FYI, if Gatekeeper thinks the app is OK, it should say this:

% spctl -av Quicksilver.app
Quicksilver.app: accepted
source=Developer ID

[pjrobertson]

So neither Rob nor I could reproduce the crash @HenningJ first mentioned with the latest pre-release.
Henning - have you managed to debug the crash any further?

If not, I suggest we merge this (I've added a couple more commits from @tiennou's suggestions) then get the next pre-release out.

[skurfer]

Here's crash I've gotten myself 3 times. Not sure how to reproduce it at will. I think there are only 3 places in the Core Support plug-in that call timeIntervalSinceReferenceDate, but I think it's the comparison to the result and not the actual call that's causing a problem.

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: EXC_I386_GPFLT

Application Specific Information:
objc_msgSend() selector name: timeIntervalSinceReferenceDate


Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   libobjc.A.dylib                 0x00007fff9a075250 objc_msgSend + 16
1   com.apple.CoreFoundation        0x00007fff9876eefe -[NSDate compare:] + 62
2   com.blacktree.Quicksilver.QSCorePlugIn  0x00000001067d54e2 0x1067ca000 + 46306
3   com.blacktree.QSCore            0x00000001000d4f11 -[QSCatalogEntry indexIsValid] + 386
4   com.blacktree.QSCore            0x00000001000d55d3 -[QSCatalogEntry scanForced:] + 578
5   com.apple.CoreFoundation        0x00007fff9874d47a _CFXNotificationPost + 2554
6   com.apple.Foundation            0x00007fff8ec77846 -[NSNotificationCenter postNotificationName:object:userInfo:] + 64
7   com.blacktree.QSCore            0x00000001000ffb7b __26-[VDKQueue watcherThread:]_block_invoke_0 + 223
8   libdispatch.dylib               0x00007fff954e1f01 _dispatch_call_block_and_release + 15
9   libdispatch.dylib               0x00007fff954de0b6 _dispatch_client_callout + 8
10  libdispatch.dylib               0x00007fff954e30c8 _dispatch_main_queue_callback_4CF + 275
11  com.apple.CoreFoundation        0x00007fff9873d0fe __CFRunLoopRun + 1614
12  com.apple.CoreFoundation        0x00007fff9873c6b2 CFRunLoopRunSpecific + 290
13  com.apple.HIToolbox             0x00007fff99b380a4 RunCurrentEventLoopInMode + 209
14  com.apple.HIToolbox             0x00007fff99b37e42 ReceiveNextEventCommon + 356
15  com.apple.HIToolbox             0x00007fff99b37cd3 BlockUntilNextEventMatchingListInMode + 62
16  com.apple.AppKit                0x00007fff95a46613 _DPSNextEvent + 685
17  com.apple.AppKit                0x00007fff95a45ed2 -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] + 128
18  com.apple.AppKit                0x00007fff95a3d283 -[NSApplication run] + 517
19  com.apple.AppKit                0x00007fff959e1cb6 NSApplicationMain + 869
20  com.blacktree.Quicksilver       0x00000001000019d4 0x100000000 + 6612

[pjrobertson]

We can deduce from the log that the crash is in QSFileSystemObjectSource.m, since the method is called from a notif sent by VDKQueue (file observing class), and only file object sources use QSVoyer (the file watching class which calls VDKQueue).

So the crash is in QSFileSystemObjectSource.m:indexIsValidFromDate:...
But it may well be a red herring. Are any other threads working on the catalog entry?

Possibly, indexDate is being released and a new value set whilst indexIsValidFromDate:... is being called.
If you look in QSCatalogEntry.m, at line 460, indexDate is being set there, and then passed in in line 466 (where the crash occurs). I don't really see how this could be possible.

My guess is that this is a red herring.

[pjrobertson]

OK, merging. You said on IRC that it may be somewhere else where the crash is occurring, so I'll let you look into it further.