Bug 97628 – NSLocale leaks in LocaleMac

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Keishi Hattori]

Fix leaking NSLocale in LocaleMac

[Keishi Hattori]

Created attachment 165728 [details]
Patch

[Kent Tamura]

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.

[Keishi Hattori]

Created attachment 165944 [details]
Patch

[Alexey Proskuryakov]

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.

[Darin Adler]

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.

[Keishi Hattori]

Created attachment 166408 [details]
Patch

[Keishi Hattori]

> 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.

[Kent Tamura]

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<>.

[Alexey Proskuryakov]

RetainPtr is used without AdoptNS tag here, so it will retain first.

[Kent Tamura]

(In reply to comment #9)
> RetainPtr is used without AdoptNS tag here, so it will retain first.

Thanks.  I understand.

[Keishi Hattori]

Created attachment 171217 [details]
Patch

[WebKit Review Bot]

Comment on attachment 171217 [details]
Patch

Clearing flags on attachment: 171217

Committed r132868: <http://trac.webkit.org/changeset/132868>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.