[swift-corelibs-dev] Adding retain to CFCalendar.c - SR2879

[Tony Parker]

Yes, that was the “skirting of the issue”. =)

It’s likely not a huge deal in the end; since CF is an implementation detail of swift-corelibs-foundation, we just need to remember to retain/release constant strings as we would others and we would be ok. However, I suspect there are other places in CF where we get away with it on Darwin but can’t on Swift. So it would be nice to have them be consistent, for our own sanity.

- Tony

> On Oct 7, 2016, at 10:31 AM, Alex Blewitt <alblue at apple.com> wrote:
> 
> That's what I was expecting, too, but then I found this:
> 
> https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Base.subproj/CFInternal.h#L387-L389 <https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Base.subproj/CFInternal.h#L387-L389>
> 
> #if DEPLOYMENT_RUNTIME_SWIFT
>  // TODO: Pinned retain count for constants?
>  #define __CFSTR_RC_INIT _CF_CONSTANT_OBJECT_STRONG_RC, 0,
> 
> Alex
> 
>> On 7 Oct 2016, at 17:51, Tony Parker <anthony.parker at apple.com <mailto:anthony.parker at apple.com>> wrote:
>> 
>> Hi Alex,
>> 
>> Thanks for digging into this. This seems like a correct solution for now.
>> 
>> I think there is a larger question though, which is why it’s possible to overrelease kCFEmptyString. I think we skirted the issue early in bringup of SCL-Foundation, but constant strings are supposed to be “pinned” and ref count operations on them a no-op.
>> 
>> - Tony
>> 
>>> On Oct 7, 2016, at 6:47 AM, Alex Blewitt via swift-corelibs-dev <swift-corelibs-dev at swift.org <mailto:swift-corelibs-dev at swift.org>> wrote:
>>> 
>>> I'm looking at https://bugs.swift.org/browse/SR-2879 <https://bugs.swift.org/browse/SR-2879> which is exposing itself through an over-release of a constant CF string (in this case, kCFEmptyString). I don't believe it to be a Swift related problem, because Swift doesn't get into the internals of CFCalendar where the problem occurs.
>>> 
>>> The problem is that CFCalendar releases the localeID when it's deallocated:
>>> 
>>> if (calendar->_localeID) CFRelease(calendar->_localeID);
>>> 
>>> https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFCalendar.c#L54 <https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFCalendar.c#L54> 
>>> 
>>> The problem is that when the localeID is assigned, it doesn't appear to be copied or renamed:
>>> 
>>> calendar->_localeID = CFLocaleGetIdentifier(CFLocaleGetSystem());
>>> 
>>> https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFCalendar.c#L252 <https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFCalendar.c#L252> 
>>> https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFCalendar.c#L281 <https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFCalendar.c#L281>  
>>> 
>>> but elsewhere in the code, we do retain it:
>>> 
>>>    CFStringRef localeID = CFLocaleGetIdentifier(locale);
>>>     if (localeID != calendar->_localeID) {
>>> 	CFRelease(calendar->_localeID);
>>> 	CFRetain(localeID);
>>> 
>>> https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFCalendar.c#L303-L306 <https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFCalendar.c#L303-L306> 
>>> 
>>> When a locale isn't supplied, it uses the default global one, which is defined to be an empty string:
>>> 
>>> CFLocaleRef CFLocaleGetSystem(void) {
>>>     CFLocaleRef locale;
>>>     CFLocaleRef uselessLocale = NULL; //if we lose the race creating the global locale, we need to release the one we created, but we want to do it outside the lock.
>>>     __CFLocaleLockGlobal();
>>>     if (NULL == __CFLocaleSystem) {
>>> 	__CFLocaleUnlockGlobal();
>>> 	locale = CFLocaleCreate(kCFAllocatorSystemDefault, CFSTR(""));
>>> 	if (!locale) return NULL;
>>> 
>>> https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFLocale.c#L255-L261 <https://github.com/apple/swift-corelibs-foundation/blob/1a76e814212e781a9d50782ee24117760cfe9b48/CoreFoundation/Locale.subproj/CFLocale.c#L255-L261> 
>>> 
>>> The CFSTR("") results in a reference to kCFEmptyString, which reduces by one each time a CFCalendar is created and destroyed, leading to the (unrelated) test failures of https://github.com/apple/swift-corelibs-foundation/pull/667 <https://github.com/apple/swift-corelibs-foundation/pull/667> as documented in https://bugs.swift.org/browse/SR-2879 <https://bugs.swift.org/browse/SR-2879>
>>> 
>>> My suggestion is to insert a CFRetain when the calendar->locale is set, to balance out the CFRelease that's being performed in the deallocator. Building with this simple change and checking the retain count of kCFEmptyString verifies that it does fix the problem, although I'm open to suggestions as to improvements of where the retain takes place, if not on lines 252 and 282.
>>> 
>>>   1> import Foundation 
>>>   2> :p (int)swift_retainCount(&__kCFEmptyString) 
>>> (int) $11 = 1
>>>   2> _ = Calendar(identifier:.gregorian)
>>>   3> :p (int)swift_retainCount(&__kCFEmptyString) 
>>> (int) $12 = 3
>>>   3> _ = Calendar(identifier:.chinese)
>>>   4> :p (int)swift_retainCount(&__kCFEmptyString) 
>>> (int) $13 = 3
>>>   4> _ = Calendar(identifier:.hebrew)
>>>   5> :p (int)swift_retainCount(&__kCFEmptyString) 
>>> (int) $14 = 3
>>>   5> ^D
>>> 
>>> Alex
>>> _______________________________________________
>>> swift-corelibs-dev mailing list
>>> swift-corelibs-dev at swift.org <mailto:swift-corelibs-dev at swift.org>
>>> https://lists.swift.org/mailman/listinfo/swift-corelibs-dev <https://lists.swift.org/mailman/listinfo/swift-corelibs-dev>
>> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.swift.org/pipermail/swift-corelibs-dev/attachments/20161007/18970a4f/attachment.html>