GNOME Bugzilla – Bug 322299
regression in gail 1.8.6 causes file chooser crash
Last modified: 2005-11-28 15:02:32 UTC
- run Glade - click on Open - navigate to a folder containing .glade files - click on Cancel - externally modify one of the .glade files (or just open a project and save it) One of the recent gailtreeview.c commits cause this crash. Backtrace is attached.
Created attachment 55174 [details] ..
*** This bug has been marked as a duplicate of 322044 ***
No, it's another bug. I'm already running the #322044 patch.
Doesn't look like gailtreeview is at fault here, from the backtrace.
I cannot reproduce with the steps you list, above. Please give more detailed directions, and/or confirm that you are running gail 1.8.7.
Created attachment 55211 [details] [review] some fixes Fixes the bug which caused my crash: GtkTreeRowReference leaks in garbage_collect_cell_data(). Also fixes: - order of execution in gail_tree_view_finalize(): clear_cached_data() may queue a garbage collect and must therefore be called before "remove any idle handlers still pending" - a memory leak in return_iter_nth_row() - a main loop source leak in cell_destroyed()
Solution provided; reopening.
Comment on attachment 55211 [details] [review] some fixes Comments inline: >--- gail/gailtreeview.c.orig Fri Nov 25 05:49:12 2005 >+++ gail/gailtreeview.c Fri Nov 25 06:36:24 2005 >@@ -678,7 +678,9 @@ > { > GailTreeView *view = GAIL_TREE_VIEW (object); > >- /* remove anyg idle handlers still pending */ >+ clear_cached_data (view); >+ >+ /* remove any idle handlers still pending */ > if (view->idle_garbage_collect_id) > g_source_remove (view->idle_garbage_collect_id); > >@@ -692,8 +694,6 @@ > if (view->tree_model) > disconnect_model_signals (view); > >- clear_cached_data (view); >- > if (view->col_data) > { > GArray *array = view->col_data; >@@ -3295,8 +3295,10 @@ > GtkTreeIter new_iter; > gboolean row_expanded; > >- if (increment == row) >+ if (increment == row) { >+ gtk_tree_path_free (current_path); > return iter; >+ } > > row_expanded = gtk_tree_view_row_expanded (tree_view, current_path); > gtk_tree_path_free (current_path); OK, this makes sense. Thanks. But... >@@ -3574,7 +3576,7 @@ > > g_assert (GAIL_IS_TREE_VIEW (data)); > tree_view = (GailTreeView *)data; >- temp_list = tree_view->cell_data; >+ temp_list = g_list_copy (tree_view->cell_data); > > tree_view->garbage_collection_pending = FALSE; > tree_view->idle_garbage_collect_id = 0; >@@ -3586,14 +3588,15 @@ > if (!cell_info->in_use) > { > /* g_object_unref (cell_info->cell); */ >- tree_view->cell_data = g_list_remove_link (tree_view->cell_data, >- temp_list); >+ tree_view->cell_data = g_list_remove (tree_view->cell_data, >+ cell_info); > if (cell_info->cell_row_ref) > gtk_tree_row_reference_free (cell_info->cell_row_ref); > g_free (cell_info); > } > temp_list = temp_list->next; > } >+ g_list_free (temp_list); > > return tree_view->garbage_collection_pending; > } This I don't get. I don't understand why we have to operate on a copy here; is there some potential re-enterancy in this loop? I don't see it ATM. Also, it's not obvious to me from the glib API that g_list_remove_link and g_list_remove do different things in these cases; perhaps the g_list docs are failing to say that g_list_remove frees the cell data? You seem to indicate that this leak (above, assuming that I've guessed the intent w.r.t. g_list_free vs. g_list_remove_link) was causing your crash. How can a leaked object cause that? >@@ -3989,7 +3992,8 @@ > g_assert (GAIL_IS_TREE_VIEW (cell_info->view)); > if (!cell_info->view->garbage_collection_pending) { > cell_info->view->garbage_collection_pending = TRUE; >- g_idle_add (garbage_collect_cell_data, cell_info->view); >+ cell_info->view->idle_garbage_collect_id = >+ g_idle_add (garbage_collect_cell_data, cell_info->view); > } > } > } OK, thanks for catching this one - perhaps this was the root of your problem (i.e. not removing the right idle handler?)
You have to operate on a copy because you cannot modify a list while iterating over it. In this case, your g_slist_remove_link() call was setting temp_list->next to NULL (as indicated in the GLib documentation), and so you were only freeing the first cell. It was the leaked GtkTreeRowReference objects of the next cells to cause my crash: - a GtkTreeRowReference keeps a reference to its GtkTreeModel - since the file chooser's model was still alive because of the leaked references, it continued receiving file notifications from GnomeVFS - these notifications caused the file chooser's model to sort itself, crashing in name_sort_func() because the GtkFileChooserDefault widget was gone, destroyed along with the dialog
D'Oh! (temp_list->next == NULL) Gotcha, thanks for the patch. You're welcome to commit. BTW did you see my patch for 322401? gailtreeview.c is a mess ATM. I guess you noticed that.
Comment on attachment 55211 [details] [review] some fixes and thanks for the clear explanation.
I cannot commit. I tested your #322401 patch but it doesn't work.
"- these notifications caused the file chooser's model to sort itself, crashing in name_sort_func() because the GtkFileChooserDefault widget was gone, destroyed along with the dialog" While I agree with the patch, the above behavior sounds like a bug in name_sort_func or gtktreemodel, shouldn't it be robust to destruction of the host widget? I'm writing the ChangeLog for the patch, will commit. THanks again.
The host widget connects name_sort_func(), passing itself as the data pointer. Since it's internal GTK+ code, the host widget does not (and should not) expect the model to survive after it is destroyed.
Comment on attachment 55211 [details] [review] some fixes Patch committed.
*** Bug 322344 has been marked as a duplicate of this bug. ***
Will re-dist today for gnome 2.12.2, with this fix. New dist will be gail-1.8.8.