Working on the Nextcloud desktop client, I stumbled into a nasty little bug. If you tried to add or remove a user account, the app would often freeze for a while.
This was super annoying during testing, so I decided to look into it. Surprisingly this wasn’t a new bug I’d unwittingly introduced; tracing the source of the freeze led all the way down into QtKeychain, which is what the desktop client uses to store user credentials.
QtKeychain, like Qt itself, is great at making life easy for developers targeting multiple platforms as it abstracts away the different keychain/keyring/wallet implementations for the different platforms. Looking at the Objective-C++ code handling Apple platforms, QtKeychain’s different job classes called code using Keychain Services functions, as expected.
Fortunately the problem was pretty clear. All of the calls to the Keychain Services functions — SecItemUpdate, SecItemDelete, SecItemAdd, SecItemCopyMatching — block the calling thread. From Apple’s documentation:
SecItemUpdate blocks the calling thread, so it can cause your app’s UI to hang if called from the main thread. Instead, call SecItemUpdate from a background dispatch queue or async function.
This means that, when called on the main thread, doing any QtKeychain operations would lead to freezing the UI. Since this is not a problem on other platforms (that I know of!), it’s pretty easy to overlook this quirk of Keychain Services’ API.
void DeletePasswordJobPrivate::scheduledStart()
{
const NSDictionary *const query = @{
(__bridge id) kSecClass: (__bridge id) kSecClassGenericPassword,
(__bridge id) kSecAttrService: (__bridge NSString *) service.toCFString(),
(__bridge id) kSecAttrAccount: (__bridge NSString *) key.toCFString(),
};
const OSStatus status = SecItemDelete((__bridge CFDictionaryRef) query);
if (status == errSecSuccess) {
q->emitFinished();
} else {
const ErrorDescription error = ErrorDescription::fromStatus(status);
q->emitFinishedWithError(error.code, Job::tr("Could not remove private key from keystore: %1").arg(error.message));
}
}With use of Grand Central Dispatch, a fix was relatively straightforward. All that needed to be done was to call the offending functions from a different dispatch queue. I rewrote the Apple-specific keychain implementation in order to call the Keychain Services functions asynchronously from the global queue, passing an interface object that would call the relevant completion methods and signals on the read/write/delete QtKeychain jobs.
static void StartDeletePassword(const QString &service, const QString &key, AppleKeychainInterface * const interface)
{
dispatch_async(dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_BACKGROUND, 0), ^{
NSDictionary * const query = @{
(__bridge NSString *)kSecClass: (__bridge NSString *)kSecClassGenericPassword,
(__bridge NSString *)kSecAttrService: service.toNSString(),
(__bridge NSString *)kSecAttrAccount: key.toNSString(),
};
const OSStatus status = SecItemDelete((__bridge CFDictionaryRef)query);
if (status == errSecSuccess) {
dispatch_async(dispatch_get_main_queue(), ^{
[interface keychainTaskFinished];
[interface release];
});
} else {
NSString * const descriptiveErrorString = @"Could not remove private key from keystore";
dispatch_async(dispatch_get_main_queue(), ^{
[interface keychainTaskFinishedWithError:status descriptiveMessage:descriptiveErrorString];
[interface release];
});
}
});
}Note that after these changes, your application will need to either use QGuiApplication or you will need to initialise NSApplication yourself if you use QCoreApplication.
Testing the changes with a fresh build of QtKeychain, I was happy to see the freezes are now gone in the Nextcloud desktop client. 🙂
Thanks to Frank Osterfeld for reviewing the pull request!
