Merge lp:~gue5t/midori/addons-iter-invalidation into lp:midori

[gue5t gue5t]

Avoid bugs due to race condition in addons delete dialog

From the bug report comments:
It looks like addons is sloppy in keeping track of references to rows in its treeview, which is repopulated when it notices the filesystem changes (it notices this at most once per second). Thus, if you delete a script and then delete another without waiting a couple seconds in between, the second "delete?" dialog is interrupted by the filesystem change being noticed and loses its reference to the selected row.

This change uses a GtkTreeRowReference to catch changes to the GtkTreeView and avoids keeping the pointer to the element across a call to gtk_dialog_run, which invokes the main loop and may result in the element list being reconstructed and old elements being freed.

[lp:~gue5t/midori/addons-iter-invalidation]

6703. By gue5t <email address hidden> on 2014-05-21

Fix leak

[Cris Dywan (kalikiana)]

I like the approach. Presumably unit testing it isn't easy… has anyone else manually tested it? Volunteers step up!

review: Approve (needs-testing)

[Paweł Forysiuk (tuxator)]

review: Approve

=== modified file 'extensions/addons.c'
--- extensions/addons.c	2014-02-22 22:35:42 +0000
+++ extensions/addons.c	2014-05-21 03:33:43 +0000
@@ -393,6 +393,9 @@
 {
     GtkTreeModel* model;
     GtkTreeIter iter;
+    GtkTreePath* path;
+    GtkTreeRowReference* row;
+    gchar* fullpath;
 
     if (katze_tree_view_get_selected_iter (GTK_TREE_VIEW (addons->treeview),
                                                           &model, &iter))
@@ -403,6 +406,11 @@
         gchar* markup;
 
         gtk_tree_model_get (model, &iter, 0, &element, -1);
+        fullpath = g_strdup (element->fullpath);
+        
+        path = gtk_tree_model_get_path (model, &iter);
+        row = gtk_tree_row_reference_new (model, path);
+        gtk_tree_path_free (path);
         dialog = gtk_message_dialog_new (
             GTK_WINDOW (gtk_widget_get_toplevel (GTK_WIDGET (addons))),
             GTK_DIALOG_MODAL | GTK_DIALOG_DESTROY_WITH_PARENT,
@@ -424,6 +432,9 @@
             GTK_MESSAGE_DIALOG (dialog), "%s", markup);
         g_free (markup);
 
+        /* The execution of gtk_dialog_run allows the directory watcher to
+        rebuild the treeview and the element list, so our references may be
+        invalid afterward */
         delete_response = gtk_dialog_run (GTK_DIALOG (dialog));
         gtk_widget_destroy (GTK_WIDGET (dialog));
 
@@ -433,7 +444,7 @@
             GFile* file;
             gboolean result;
 
-            file = g_file_new_for_path (element->fullpath);
+            file = g_file_new_for_path (fullpath);
             result = g_file_delete (file, NULL, &error);
 
             if (!result && error)
@@ -452,11 +463,20 @@
                     g_error_free (error);
             }
 
-            if (result)
+            /* The row reference may have been invalidated if the
+            filesystem watcher deleted the row concurrently */
+            if (result && gtk_tree_row_reference_valid (row))
+            {
+                path = gtk_tree_row_reference_get_path (row);
+                gtk_tree_model_get_iter (model, &iter, path);
+                gtk_tree_path_free (path);
                 gtk_list_store_remove (GTK_LIST_STORE (model), &iter);
+            }
 
+            gtk_tree_row_reference_free (row);
             g_object_unref (file);
         }
+        g_free (fullpath);
     }
 }
 static void