Bug 57307

Summary: [GTK] Fix leaked pointer in FontGtk.cpp
Product: WebKit Reporter: Ryuan Choi <ryuan.choi>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, eric, kevin.cs.oh, mrobinson, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Ryuan Choi 2011-03-29 00:43:22 PDT
In convertUniCharToUTF8,
utf8 got allocated memory via utf16_to_utf8(characters, length, utf8, new_length);

But, It looks not released.
Comment 1 Ryuan Choi 2011-03-29 01:04:30 PDT
Created attachment 87280 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-03-29 13:05:33 PDT
Comment on attachment 87280 [details]
Patch

Can't we use OwnGPtr?
Comment 3 Martin Robinson 2011-03-29 13:22:34 PDT
Comment on attachment 87280 [details]
Patch

Great catch! I think we can use this opportunity to clean this code up a bit. We should probably make utf16_to_utf8 return a char* instead of taking a char*& (which is bad form, IMO). Then in the caller we should immediately place the return value in a GOwnPtr<gchar> and remove the calls to g_free. This will prevent any mistakes in the future as well.
Comment 4 Ryuan Choi 2011-03-29 19:29:11 PDT
Created attachment 87452 [details]
Patch
Comment 5 Ryuan Choi 2011-03-29 19:33:13 PDT
(In reply to comment #3)
> (From update of attachment 87280 [details])
> Great catch! I think we can use this opportunity to clean this code up a bit. We should probably make utf16_to_utf8 return a char* instead of taking a char*& (which is bad form, IMO). Then in the caller we should immediately place the return value in a GOwnPtr<gchar> and remove the calls to g_free. This will prevent any mistakes in the future as well.

I update patch as mentioned with refactoring utf16_to_utf8(because of fixing indentation and coding style).

And also, can we remove 'from' variable in convertUniCharToUTF8?
this value will be passed 0 always.

Thanks.
Comment 6 Martin Robinson 2011-03-30 15:21:34 PDT
Comment on attachment 87452 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=87452&action=review

This code probably needs quite a bit more cleanup in the future, but this is a good incremental change. Please make the following changes before landing.

> Source/WebCore/platform/graphics/gtk/FontGtk.cpp:92
>      for (i = 0; i < aLength; i++) {

Moving the declaration of i here would be more idiomatic in C++.

> Source/WebCore/platform/graphics/gtk/FontGtk.cpp:135
> +        g_free((gpointer)aText);

You do not need to cast here.

> Source/WebCore/platform/graphics/gtk/FontGtk.cpp:142
>      gint new_length = 0;

new_length --> newLength;

> Source/WebCore/platform/graphics/gtk/FontGtk.cpp:154
>      if (from > 0) {
>          // discard the first 'from' characters
>          // FIXME: we should do this before the conversion probably
> -        gchar* str_left = g_utf8_offset_to_pointer(utf8, from);
> -        gchar* tmp = g_strdup(str_left);
> -        g_free(utf8);
> -        utf8 = tmp;
> +        gchar* strLeft = g_utf8_offset_to_pointer(utf8Text.get(), from);
> +        utf8Text.set(g_strdup(strLeft));
>      }
>  
> -    gchar* pos = utf8;
> +    gchar* pos = utf8Text.get();

I think you can easily optimize this section by doing something like:

gchar* pos = utf8Text.get();
if (from > 0) {
    // FIXME: we should do this before the conversion probably
    pos = g_utf8_offset_to_pointer(pos, from);
}
Comment 7 Ryuan Choi 2011-03-30 18:09:51 PDT
Created attachment 87650 [details]
Patch
Comment 8 Ryuan Choi 2011-03-30 18:14:21 PDT
(In reply to comment #6)
> (From update of attachment 87452 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=87452&action=review
> 
> This code probably needs quite a bit more cleanup in the future, but this is a good incremental change. Please make the following changes before landing.
> 
> > Source/WebCore/platform/graphics/gtk/FontGtk.cpp:92
> >      for (i = 0; i < aLength; i++) {
> 
> Moving the declaration of i here would be more idiomatic in C++.
Done.

> 
> > Source/WebCore/platform/graphics/gtk/FontGtk.cpp:135
> > +        g_free((gpointer)aText);
> 
> You do not need to cast here.

Because aText is const variable, I couldn't remove casting.
Instead, I removed g_free itself using GOwnPtr.
 
> 
> > Source/WebCore/platform/graphics/gtk/FontGtk.cpp:142
> >      gint new_length = 0;
> 
> new_length --> newLength;
> 
Done.

> > Source/WebCore/platform/graphics/gtk/FontGtk.cpp:154
> >      if (from > 0) {
> >          // discard the first 'from' characters
> >          // FIXME: we should do this before the conversion probably
> > -        gchar* str_left = g_utf8_offset_to_pointer(utf8, from);
> > -        gchar* tmp = g_strdup(str_left);
> > -        g_free(utf8);
> > -        utf8 = tmp;
> > +        gchar* strLeft = g_utf8_offset_to_pointer(utf8Text.get(), from);
> > +        utf8Text.set(g_strdup(strLeft));
> >      }
> >  
> > -    gchar* pos = utf8;
> > +    gchar* pos = utf8Text.get();
> 
> I think you can easily optimize this section by doing something like:
> 
> gchar* pos = utf8Text.get();
> if (from > 0) {
>     // FIXME: we should do this before the conversion probably
>     pos = g_utf8_offset_to_pointer(pos, from);
> }
Done.
Comment 9 WebKit Commit Bot 2011-03-30 20:44:54 PDT
The commit-queue encountered the following flaky tests while processing attachment 87650 [details]:

animations/dynamic-stylesheet-loading.html bug 52669 (author: cmarrin@apple.com)
The commit-queue is continuing to process your patch.
Comment 10 WebKit Commit Bot 2011-03-30 20:47:11 PDT
Comment on attachment 87650 [details]
Patch

Clearing flags on attachment: 87650

Committed r82541: <http://trac.webkit.org/changeset/82541>
Comment 11 WebKit Commit Bot 2011-03-30 20:47:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 WebKit Review Bot 2011-03-30 21:29:58 PDT
http://trac.webkit.org/changeset/82541 might have broken GTK Linux 32-bit Debug
Comment 13 Ryuan Choi 2011-04-01 21:40:06 PDT
Created attachment 87963 [details]
Patch
Comment 14 Ryuan Choi 2011-04-01 21:43:54 PDT
I recreate patch because it was reverted with other patchs.
Comment 15 Martin Robinson 2011-04-02 11:57:09 PDT
Comment on attachment 87963 [details]
Patch

Sorry. This patch should not have been rolled out!
Comment 16 WebKit Commit Bot 2011-04-02 12:22:32 PDT
Comment on attachment 87963 [details]
Patch

Clearing flags on attachment: 87963

Committed r82772: <http://trac.webkit.org/changeset/82772>
Comment 17 WebKit Commit Bot 2011-04-02 12:22:38 PDT
All reviewed patches have been landed.  Closing bug.