WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
Formatted Diff
Diff
Patch
(4.91 KB, patch)
2011-03-29 19:29 PDT
,
Ryuan Choi
no flags
Details
Formatted Diff
Diff
Patch
(5.05 KB, patch)
2011-03-30 18:09 PDT
,
Ryuan Choi
no flags
Details
Formatted Diff
Diff
Patch
(4.94 KB, patch)
2011-04-01 21:40 PDT
,
Ryuan Choi
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Ryuan Choi
Comment 1
2011-03-29 01:04:30 PDT
Created
attachment 87280
[details]
Patch
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
Created
attachment 87452
[details]
Patch
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
Created
attachment 87650
[details]
Patch
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
Created
attachment 87963
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug