After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 322299 - regression in gail 1.8.6 causes file chooser crash
regression in gail 1.8.6 causes file chooser crash
Status: RESOLVED FIXED
Product: atk
Classification: Platform
Component: gail
unspecified
Other All
: Normal critical
: ---
Assigned To: bill.haneman
bill.haneman
: 322344 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2005-11-24 06:21 UTC by Jean-Yves Lefort
Modified: 2005-11-28 15:02 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
.. (14.08 KB, text/plain)
2005-11-24 06:22 UTC, Jean-Yves Lefort
  Details
some fixes (2.10 KB, patch)
2005-11-25 06:24 UTC, Jean-Yves Lefort
accepted-commit_now Details | Review

Description Jean-Yves Lefort 2005-11-24 06:21:35 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.
Comment 1 Jean-Yves Lefort 2005-11-24 06:22:02 UTC
Created attachment 55174 [details]
..
Comment 2 bill.haneman 2005-11-24 11:46:28 UTC

*** This bug has been marked as a duplicate of 322044 ***
Comment 3 Jean-Yves Lefort 2005-11-24 12:40:59 UTC
No, it's another bug. I'm already running the #322044 patch.
Comment 4 bill.haneman 2005-11-24 12:57:38 UTC
Doesn't look like gailtreeview is at fault here, from the backtrace.  
Comment 5 bill.haneman 2005-11-24 13:01:06 UTC
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.
Comment 6 Jean-Yves Lefort 2005-11-25 06:24:28 UTC
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()
Comment 7 Jean-Yves Lefort 2005-11-25 06:25:21 UTC
Solution provided; reopening.
Comment 8 bill.haneman 2005-11-25 10:27:21 UTC
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?)
Comment 9 Jean-Yves Lefort 2005-11-25 11:15:05 UTC
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
Comment 10 bill.haneman 2005-11-25 11:23:46 UTC
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 11 bill.haneman 2005-11-25 11:24:18 UTC
Comment on attachment 55211 [details] [review]
some fixes

and thanks for the clear explanation.
Comment 12 Jean-Yves Lefort 2005-11-25 13:48:10 UTC
I cannot commit. I tested your #322401 patch but it doesn't work.
Comment 13 bill.haneman 2005-11-25 15:18:53 UTC
"- 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.
Comment 14 Jean-Yves Lefort 2005-11-25 15:39:11 UTC
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 15 bill.haneman 2005-11-25 15:46:53 UTC
Comment on attachment 55211 [details] [review]
some fixes

Patch committed.
Comment 16 Frederic Crozat 2005-11-28 14:36:25 UTC
*** Bug 322344 has been marked as a duplicate of this bug. ***
Comment 17 bill.haneman 2005-11-28 15:02:32 UTC
Will re-dist today for gnome 2.12.2, with this fix.  New dist will be gail-1.8.8.