cleanObjectDictionary causing crashes

[pjrobertson]

There have been quite a few log reports relating to the the above method (in QSObject.m) crashing.
I've seen these before, but have never figured out how to solve the problem.

I initially thought the objectDictionary was being modified whilst being enumerated over, but that would cause an exception (that's what fast enumeration would give) and I see @tiennou implemented the @synchronize {} block round this bit of code so it should be locked.

This lead me to delve deeper into the code and a few things struck me:

• Why is the objectDictionary declared as a static and not an iVar?
• Should objectDictionary be an NSMutableSet as opposed to a Dict, to avoid duplicates?
• The cleanObjectDictionary method seems very wasteful. Multiple reasons why:
  • It gets called EVERY time you change app (due to the appChanged notification calling hideMainWindowWithEffect: in QSInterfaceController). I've fixed this so QS only attempts to close the window if it's actually open.
  • Most of the time no objects are removed/cleaned from objectDictionary, so we're enumerating over possibly 1000s of objects for no reason. Maybe 90% of the time for me, with my testing.

And upon further investigation - I see that I've left a long message in QSObject.m line 611 which would lead to the conclusion that an NSMutableSet would be much better.

So here's what I propose:

• The cleanObjectDictionary method be run a lot less, or not at all (remove the method entirely)
• Convert objectDictionary an iVar NSMutableSet

I'd appreciate some input, especially from @tiennou since he made the first @synchronize {} changes, and probably knows memory management a lot better than me... :)

Here are some of the crash logs we've received, for reference (multiple identical crash reports, down to the memory address and everything):

User Comments:

After autoupdate of latest plugins, Clipboard and some other: Quicksilver-Relaunch->Crash.

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

Exception Type:  EXC_BAD_ACCESS (SIGSEGV)
Exception Codes: KERN_INVALID_ADDRESS at 0x00000000ffffffff

VM Regions Near 0xffffffff:
--> shared memory          00000000ffff0000-00000000ffff2000 [    8K] r-x/r-x SM=SHM  


Application Specific Information:
objc[458]: garbage collection is OFF

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.CoreFoundation        0x909e495b CFHash + 43
1   com.apple.CoreFoundation        0x909db700 __CFDictionaryStandardHashKey + 32
2   com.apple.CoreFoundation        0x909db12f CFBasicHashFindBucket + 1679
3   com.apple.CoreFoundation        0x909daa87 CFDictionaryGetValue + 135
4   com.apple.CoreFoundation        0x90a3c97c -[__NSCFDictionary objectForKey:] + 28
5   com.blacktree.QSCore            0x000acf79 +[QSObject cleanObjectDictionary] + 310

[tiennou]

Why is the objectDictionary declared as a static and not an iVar?

Because it's shared between all QSObjects. You could move that in QSRegistry though.

Should objectDictionary be an NSMutableSet as opposed to a Dict, to avoid duplicates?

It could, but you would lose the ability to query for an object identifier.

The cleanObjectDictionary method seems very wasteful. Multiple reasons why:

That's good reasons ;-).

IIRC, objectDictionary holds references to temporary objects (like text input mode and file system navigation in non-indexed places) and recently accessed Catalog ones, akin to a cache. And -cleanObjectDictionary is there to break retain cycles it creates (since NSDictionary retains its keys and values). You could try replacing that with something else, but I'm not sure what would work actually... Maybe NSMapTable (though see this for possible shortcomings.

Maybe drop the thing entirely and check performance when navigating the filesystem ?

About the crash, I have absolutely no idea of what could cause that (since objects are retained when they enter the dictionary)... Maybe a weird pointer to something that isn't a real object, causing CFHash to fail ?

[skurfer]

Should objectDictionary be an NSMutableSet as opposed to a Dict, to avoid duplicates?

It could, but you would lose the ability to query for an object identifier.

Dictionary keys act a bit like a set anyway (since they enforce uniqueness). There’s nothing to stop values from being duplicates, but I wonder how often that happens.

We could still get an object by its identifier, but we’d have to use a predicate or a block to filter the set. Probably faster than iterating through it, but not as fast as just getting it from a dictionary by ID.

Maybe drop the thing entirely and check performance when navigating the filesystem ?

Sounds like that’s worth a try.

[pjrobertson]

So we haven't seen a single crash related to cleanObjectDictionary since I made this change. Coincidence... I hope not!

Considering we had ~500 users on ß3926 and about 6-7 of these crash reports, and we now have 20k+ users on ß3928 without a single report... I think the stats can speak for themselves.

I still think it might be worth removing the thing entirely as Etienne suggests, but at least we've reduced the number of crashes by 1 :)