Summary: | NSLocale leaks in LocaleMac | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Keishi Hattori <keishi> | ||||||||||
Component: | Forms | Assignee: | Keishi Hattori <keishi> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap, darin, tkent, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Attachments: |
|
Description
Keishi Hattori
2012-09-25 21:20:24 PDT
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. |