RESOLVED FIXED 57307
[GTK] Fix leaked pointer in FontGtk.cpp
https://bugs.webkit.org/show_bug.cgi?id=57307
Summary [GTK] Fix leaked pointer in FontGtk.cpp
Ryuan Choi
Reported 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.
Attachments
Patch (1.06 KB, patch)
2011-03-29 01:04 PDT, Ryuan Choi
no flags
Patch (4.91 KB, patch)
2011-03-29 19:29 PDT, Ryuan Choi
no flags
Patch (5.05 KB, patch)
2011-03-30 18:09 PDT, Ryuan Choi
no flags
Patch (4.94 KB, patch)
2011-04-01 21:40 PDT, Ryuan Choi
no flags
Ryuan Choi
Comment 1 2011-03-29 01:04:30 PDT
Eric Seidel (no email)
Comment 2 2011-03-29 13:05:33 PDT
Comment on attachment 87280 [details] Patch Can't we use OwnGPtr?
Martin Robinson
Comment 3 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.
Ryuan Choi
Comment 4 2011-03-29 19:29:11 PDT
Ryuan Choi
Comment 5 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.
Martin Robinson
Comment 6 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); }
Ryuan Choi
Comment 7 2011-03-30 18:09:51 PDT
Ryuan Choi
Comment 8 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.
WebKit Commit Bot
Comment 9 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.
WebKit Commit Bot
Comment 10 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>
WebKit Commit Bot
Comment 11 2011-03-30 20:47:17 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 12 2011-03-30 21:29:58 PDT
http://trac.webkit.org/changeset/82541 might have broken GTK Linux 32-bit Debug
Ryuan Choi
Comment 13 2011-04-01 21:40:06 PDT
Ryuan Choi
Comment 14 2011-04-01 21:43:54 PDT
I recreate patch because it was reverted with other patchs.
Martin Robinson
Comment 15 2011-04-02 11:57:09 PDT
Comment on attachment 87963 [details] Patch Sorry. This patch should not have been rolled out!
WebKit Commit Bot
Comment 16 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>
WebKit Commit Bot
Comment 17 2011-04-02 12:22:38 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.