Hi Hannes!
> I think this is a good idea, but as you probably guessed (and noticed,
> seeing your followup patch), such a major change will also need major
> testing. I will get to it by using your version for my regular work,
> but it will take some time to gain overall confidence. Testing by
> others would be appreciated, too, of course!
If the refactored code needs major testing, we can also test some more patches
too:)
Today I spent some time to check vimprobable for memory leaks with valgrind
and found some none freed strings in conjunction with the values filled by
jsapi_evaluate_script. It isn't easy for me to under stand the code related to
the editor feature - which of the variables 'value' or 'message' contains a
newly allocated string and when can they be freed. At the moment I've put in
some possible missed g_free() calls, but I'm not sure if this was done right.
In generals wen should find a way to reduce the number of g_free calls in the
open_editor function, if this is possible.
During the hunting of memory leaky I found out, that the hint could not be
cleaned/removed if the incremental search feature was disabled. This problem
is fixed in patch 07-Fixed-wrong-placed-ifdef-ENABLE_INCREMENTAL_SEARCH.patch.
Like HP annotated in the code, there are still heavy memory leaks in complete
function. But this function is difficult to understand. I think we should
split the logic and have a function that build up a list of matching items
(for each completion type 'commands', 'settings', 'tags', 'url-history'),
and another function to visualize these items to reduce the complexity of this
part of code. In the way the complete function is implemented at the moment,
I can't see when the memory should be freed and when not.
Daniel
>From 005762d122e003ad7ef93bc54b41acdddc01a277 Mon Sep 17 00:00:00 2001
From: Daniel Carl <[email protected]>
Date: Sun, 3 Feb 2013 14:56:15 +0100
Subject: [PATCH 1/4] Fixed missed g_free.
---
main.c | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/main.c b/main.c
index b797afc..0078ad5 100644
--- a/main.c
+++ b/main.c
@@ -1503,6 +1503,7 @@ script(const Arg *arg) {
jsapi_evaluate_script(arg->s, &value, &message);
if (message) {
set_error(message);
+ g_free(value);
g_free(message);
return FALSE;
}
@@ -1795,8 +1796,10 @@ open_editor(const Arg *arg) {
/* check if active element is suitable for text editing */
jsapi_evaluate_script("document.activeElement.tagName", &value, &message);
- if (value == NULL)
+ if (value == NULL) {
+ g_free(message);
return FALSE;
+ }
tag = g_strdup(value);
if (strcmp(tag, "INPUT") == 0) {
/* extra check: type == text */
@@ -1806,6 +1809,8 @@ open_editor(const Arg *arg) {
g_free(message);
return FALSE;
}
+ g_free(value);
+ g_free(message);
} else if (strcmp(tag, "TEXTAREA") != 0) {
g_free(value);
g_free(message);
@@ -1895,6 +1900,8 @@ _resume_from_editor(GPid child_pid, int child_status, gpointer data) {
"document.activeElement.disabled = true;"
"document.activeElement.style.background = '#aaaaaa';"
,&value, &message);
+ g_free(value);
+ g_free(message);
if (child_status) {
echo_message(Error, "External editor returned with non-zero status,"
@@ -1915,6 +1922,8 @@ _resume_from_editor(GPid child_pid, int child_status, gpointer data) {
}
jsapi_evaluate_script("document.activeElement.value = '';",
&value, &message);
+ g_free(value);
+ g_free(message);
while (EOF != (char_read = fgetc(fp))) {
if (char_read == '\n') {
@@ -1942,6 +1951,8 @@ _resume_from_editor(GPid child_pid, int child_status, gpointer data) {
fclose(fp);
jsapi_evaluate_script(set_value_js->str, &value, &message);
+ g_free(value);
+ g_free(message);
/* Fall through, error and normal exit are identical */
error_exit:
@@ -2650,10 +2661,12 @@ scripts_run_user_file() {
gchar *value = NULL, *message = NULL;
jsapi_evaluate_script(js, &value, &message);
+ g_free(js);
if (message) {
fprintf(stderr, "%s", message);
- g_free(message);
}
+ g_free(value);
+ g_free(message);
} else {
fprintf(stderr, "Cannot open %s: %s\n", user_scriptfile, error ? error->message : "file not found");
}
--
1.7.9.5
>From 750f472d6395be83e0ecad3185c74bae4ba1807c Mon Sep 17 00:00:00 2001
From: Daniel Carl <[email protected]>
Date: Sun, 3 Feb 2013 18:11:10 +0100
Subject: [PATCH 2/4] Removed unused success variable in inputbox_activate_cb.
---
main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/main.c b/main.c
index 0078ad5..7a40201 100644
--- a/main.c
+++ b/main.c
@@ -471,7 +471,7 @@ inputbox_activate_cb(GtkEntry *entry, gpointer user_data) {
char *text;
guint16 length = gtk_entry_get_text_length(entry);
Arg a;
- gboolean success = FALSE, forward = FALSE;
+ gboolean forward = FALSE;
a.i = HideCompletion;
complete(&a);
@@ -484,7 +484,7 @@ inputbox_activate_cb(GtkEntry *entry, gpointer user_data) {
gtk_widget_grab_focus(GTK_WIDGET(gui->webview));
if (length > 1 && text[0] == ':') {
- success = process_line((text + 1));
+ process_line((text + 1));
} else if (length > 1 && ((forward = text[0] == '/') || text[0] == '?')) {
webkit_web_view_unmark_text_matches(gui->webview);
#ifdef ENABLE_MATCH_HIGHLITING
--
1.7.9.5
>From 1b4906df0a794cad6bee9c5eca8fd608d9b003ce Mon Sep 17 00:00:00 2001
From: Daniel Carl <[email protected]>
Date: Sun, 3 Feb 2013 18:30:54 +0100
Subject: [PATCH 3/4] Added possible missed g_free for search_handle.
---
main.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/main.c b/main.c
index 7a40201..d79ec50 100644
--- a/main.c
+++ b/main.c
@@ -498,6 +498,9 @@ inputbox_activate_cb(GtkEntry *entry, gpointer user_data) {
search(&a);
#else
state->search_direction = forward;
+ if (state->search_handle) {
+ g_free(state->search_handle);
+ }
state->search_handle = g_strdup(&text[1]);
#endif
} else if (text[0] == '.' || text[0] == ',' || text[0] == ';') {
@@ -1371,7 +1374,9 @@ search(const Arg *arg) {
gboolean success, direction = arg->i & DirectionPrev;
if (arg->s) {
- free(state->search_handle);
+ if (state->search_handle) {
+ g_free(state->search_handle);
+ }
state->search_handle = g_strdup(arg->s);
}
if (!state->search_handle)
@@ -1406,6 +1411,7 @@ set(const Arg *arg) {
switch (arg->i) {
case ModeNormal:
if (client.state.search_handle) {
+ g_free(client.state.search_handle);
client.state.search_handle = NULL;
webkit_web_view_unmark_text_matches(client.gui.webview);
}
--
1.7.9.5
>From 25f222aecd377cbbff37a7890324dba1ad3344b5 Mon Sep 17 00:00:00 2001
From: Daniel Carl <[email protected]>
Date: Sun, 3 Feb 2013 18:33:27 +0100
Subject: [PATCH 4/4] Fixed wrong placed #ifdef ENABLE_INCREMENTAL_SEARCH.
If incremental search was not enabled, the hints where not removed after
hinting was aborted by <esc>.
---
main.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/main.c b/main.c
index d79ec50..8ed0a18 100644
--- a/main.c
+++ b/main.c
@@ -2643,9 +2643,7 @@ setup_signals() {
"signal::activate", G_CALLBACK(inputbox_activate_cb), NULL,
"signal::key-press-event", G_CALLBACK(inputbox_keypress_cb), NULL,
"signal::key-release-event", G_CALLBACK(inputbox_keyrelease_cb), NULL,
-#ifdef ENABLE_INCREMENTAL_SEARCH
"signal::changed", G_CALLBACK(inputbox_changed_cb), NULL,
-#endif
NULL);
/* inspector */
g_signal_connect(G_OBJECT(client.gui.inspector),
--
1.7.9.5
------------------------------------------------------------------------------
Everyone hates slow websites. So do we.
Make your web apps faster with AppDynamics
Download AppDynamics Lite for free today:
http://p.sf.net/sfu/appdyn_d2d_jan
_______________________________________________
Vimprobable-users mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/vimprobable-users
