Bug 57307 – [GTK] Fix leaked pointer in FontGtk.cpp

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Ryuan Choi]

In convertUniCharToUTF8,
utf8 got allocated memory via utf16_to_utf8(characters, length, utf8, new_length);

But, It looks not released.

[Ryuan Choi]

Created attachment 87280 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 87280 [details]
Patch

Can't we use OwnGPtr?

[Martin Robinson]

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.

[Ryuan Choi]

Created attachment 87452 [details]
Patch

[Ryuan Choi]

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

[Martin Robinson]

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);
}

[Ryuan Choi]

Created attachment 87650 [details]
Patch

[Ryuan Choi]

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

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 87650 [details]:

animations/dynamic-stylesheet-loading.html bug 52669 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 87650 [details]
Patch

Clearing flags on attachment: 87650

Committed r82541: <http://trac.webkit.org/changeset/82541>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/82541 might have broken GTK Linux 32-bit Debug

[Ryuan Choi]

Created attachment 87963 [details]
Patch

[Ryuan Choi]

I recreate patch because it was reverted with other patchs.

[Martin Robinson]

Comment on attachment 87963 [details]
Patch

Sorry. This patch should not have been rolled out!

[WebKit Commit Bot]

Comment on attachment 87963 [details]
Patch

Clearing flags on attachment: 87963

Committed r82772: <http://trac.webkit.org/changeset/82772>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.