Fix leaking NSLocale in LocaleMac
Created attachment 165728 [details] Patch
Comment on attachment 165728 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165728&action=review > Source/WebCore/ChangeLog:14 > + (WebCore::determineLocale): > + (WebCore::LocaleMac::LocaleMac): Please write why you changed. > Source/WebCore/platform/text/mac/LocaleMac.mm:64 > + return [currentLocale retain]; ok The returned object will be held by a RetainPtr, which will release the object later. So we need to retain here. > Source/WebCore/platform/text/mac/LocaleMac.mm:86 > + : m_locale(AdoptNS, locale) ok We should use the constructor with AdoptNS for NS* objects. > Source/WebCore/platform/text/mac/LocaleMac.mm:93 > + m_locale.adoptNS([[NSLocale alloc] initWithLocaleIdentifier:defaultLanguage()]); ok. RetainPtr::operator= doesn't call adoptNSReference(). We need to use adoptNS() for NS* objects.
Created attachment 165944 [details] Patch
Comment on attachment 165944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165944&action=review > Source/WebCore/platform/text/mac/LocaleMac.mm:64 > - return currentLocale; > + return [currentLocale retain]; This specifically is not right. A function without "copy" or "create" in its name should return pointers that don't need to be released. > Source/WebCore/platform/text/mac/LocaleMac.mm:93 > - m_locale = [[NSLocale alloc] initWithLocaleIdentifier:defaultLanguage()]; > + m_locale.adoptNS([[NSLocale alloc] initWithLocaleIdentifier:defaultLanguage()]); I think that this is the only change you need here.
Comment on attachment 165944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165944&action=review > Source/WebCore/platform/text/mac/LocaleMac.mm:66 > return [[NSLocale alloc] initWithLocaleIdentifier:locale]; This needs to either use autorelease or the function needs to be changed to return a RetainPtr. > Source/WebCore/platform/text/mac/LocaleMac.mm:86 > + : m_locale(AdoptNS, locale) This is not the right design pattern. We don’t adopt arguments. The call sites need to be changed to release.
Created attachment 166408 [details] Patch
> This needs to either use autorelease or the function needs to be changed to return a RetainPtr. I was hesitant to use autorelease so I changed determineLocale() to return a RetainPtr.
Comment on attachment 166408 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=166408&action=review > Source/WebCore/platform/text/mac/LocaleMac.mm:63 > +static RetainPtr<NSLocale> determineLocale(const String& locale) > { > - NSLocale* currentLocale = [NSLocale currentLocale]; > - String currentLocaleLanguage = languageFromLocale(String([currentLocale localeIdentifier])); > + RetainPtr<NSLocale> currentLocale = [NSLocale currentLocale]; > + String currentLocaleLanguage = languageFromLocale(String([currentLocale.get() localeIdentifier])); > String localeLanguage = languageFromLocale(locale); > if (equalIgnoringCase(currentLocaleLanguage, localeLanguage)) > return currentLocale; I think this code causes release-without-retain. [NSLocale currentLocale] returns a singleton object, and we need to retain it before putting into RetainPtr<>.
RetainPtr is used without AdoptNS tag here, so it will retain first.
(In reply to comment #9) > RetainPtr is used without AdoptNS tag here, so it will retain first. Thanks. I understand.
Created attachment 171217 [details] Patch
Comment on attachment 171217 [details] Patch Clearing flags on attachment: 171217 Committed r132868: <http://trac.webkit.org/changeset/132868>
All reviewed patches have been landed. Closing bug.