Bug 97628 - NSLocale leaks in LocaleMac
Summary: NSLocale leaks in LocaleMac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keishi Hattori
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-25 21:20 PDT by Keishi Hattori
Modified: 2012-10-29 19:39 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.35 KB, patch)
2012-09-25 21:52 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (2.44 KB, patch)
2012-09-27 00:17 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (3.95 KB, patch)
2012-09-30 23:07 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff
Patch (3.67 KB, patch)
2012-10-29 05:54 PDT, Keishi Hattori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keishi Hattori 2012-09-25 21:20:24 PDT
Fix leaking NSLocale in LocaleMac
Comment 1 Keishi Hattori 2012-09-25 21:52:47 PDT
Created attachment 165728 [details]
Patch
Comment 2 Kent Tamura 2012-09-26 23:16:21 PDT
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.
Comment 3 Keishi Hattori 2012-09-27 00:17:18 PDT
Created attachment 165944 [details]
Patch
Comment 4 Alexey Proskuryakov 2012-09-27 11:14:35 PDT
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 5 Darin Adler 2012-09-28 10:44:02 PDT
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.
Comment 6 Keishi Hattori 2012-09-30 23:07:00 PDT
Created attachment 166408 [details]
Patch
Comment 7 Keishi Hattori 2012-09-30 23:11:18 PDT
> 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 8 Kent Tamura 2012-10-26 20:34:41 PDT
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<>.
Comment 9 Alexey Proskuryakov 2012-10-26 20:50:05 PDT
RetainPtr is used without AdoptNS tag here, so it will retain first.
Comment 10 Kent Tamura 2012-10-28 18:05:24 PDT
(In reply to comment #9)
> RetainPtr is used without AdoptNS tag here, so it will retain first.

Thanks.  I understand.
Comment 11 Keishi Hattori 2012-10-29 05:54:42 PDT
Created attachment 171217 [details]
Patch
Comment 12 WebKit Review Bot 2012-10-29 19:39:52 PDT
Comment on attachment 171217 [details]
Patch

Clearing flags on attachment: 171217

Committed r132868: <http://trac.webkit.org/changeset/132868>
Comment 13 WebKit Review Bot 2012-10-29 19:39:56 PDT
All reviewed patches have been landed.  Closing bug.