Merge lp:~rye/ubuntuone-client/no-shared-cb-crash-stable-1-4 into lp:ubuntuone-client/stable-1-4

[Roman Yepishev]

Reuse shared structure to avoid crashes due to referencing already released memory.

This time the branch for stable-1-4 is proposed for merging into stable-1-4.

This branch fixes the crashes happening when people click Ubuntu One items in nautilus.
As tested by lp:~rye/+junk/nautilus-bogus-extension there is an extra call to menu entry generation code before activation handler. Since we allocate a new CBData structure every time this routine is called this should lead to a crash when g_free()d memory is referenced.

I agree that this is not an optimal way but it fixes the issue we've been having for quite a long time.

The trunk will have a proper version for this that will not have any shared structure thus eliminating all possible issues with concurrency. For Maverick this branch can be used.

[Rodrigo Moya (rodrigo-moya)]

review: Approve

[Alejandro J. Cura (alecu)]

Looks good.

review: Approve

=== modified file 'nautilus/ubuntuone-nautilus.c'
--- nautilus/ubuntuone-nautilus.c	2010-09-14 14:20:39 +0000
+++ nautilus/ubuntuone-nautilus.c	2010-09-23 13:35:01 +0000
@@ -378,21 +378,24 @@
   iface->get_widget = ubuntuone_nautilus_get_location_widget;
 }
 
-static void __cb_data_free (struct _CBData * data) {
+static void __cb_data_free (struct _CBData * data, gboolean destroy) {
   if (!data)
     return;
 
   data->uon = NULL;
   data->parent = NULL;
+  data->make_public = FALSE;
 
   if (data->path) {
     g_free (data->path);
     data->path = NULL;
   }
 
-  g_free (data);
+  if (destroy) {
+    g_free (data);
+    data = NULL;
+  }
 
-  data = NULL;
 }
 
 /* Menu callbacks */
@@ -522,7 +525,6 @@
   gchar * homedir_path;
   gboolean is_managed, is_root, is_udf, is_public, is_shared, is_pending;
   gboolean is_inhome, is_dir, is_regular;
-  struct _CBData * cb_data;
 
   uon = UBUNTUONE_NAUTILUS (provider);
 
@@ -565,15 +567,15 @@
   is_dir = nautilus_file_info_is_directory (file);
   is_regular = nautilus_file_info_get_file_type (file) == G_FILE_TYPE_REGULAR;
 
+  /* Create new structure only if it does not already exist */
   if (uon->cb_data)
-    __cb_data_free (uon->cb_data);
-
-  cb_data = g_new0 (struct _CBData, 1);
-  cb_data->uon = uon;
-  cb_data->parent = window;
-  cb_data->path = g_strdup (path);
-
-  uon->cb_data = cb_data;
+    __cb_data_free (uon->cb_data, FALSE);
+  else
+    uon->cb_data = g_new0 (struct _CBData, 1);
+
+  uon->cb_data->uon = uon;
+  uon->cb_data->parent = window;
+  uon->cb_data->path = g_strdup (path);
 
   root_item = nautilus_menu_item_new ("ubuntuone",
                                       _("_Ubuntu One"),
@@ -599,7 +601,7 @@
 
       g_signal_connect (item, "activate",
                         G_CALLBACK (ubuntuone_nautilus_share_folder),
-                        cb_data);
+                        uon->cb_data);
     } else {
       /* the different tooltips will probably do us no good */
       if (is_root) {
@@ -636,7 +638,7 @@
 
     g_signal_connect (item, "activate",
 		      G_CALLBACK (ubuntuone_nautilus_unshare_folder),
-		      cb_data);
+		      uon->cb_data);
     nautilus_menu_append_item (submenu, item);
   }
 
@@ -667,7 +669,7 @@
 
 	  g_signal_connect (item, "activate",
 			    G_CALLBACK (ubuntuone_nautilus_unsubscribe_folder),
-			    cb_data);
+			    uon->cb_data);
 	}
       } else {
 	/* unmanaged */
@@ -679,7 +681,7 @@
 
 	g_signal_connect (item, "activate",
 			  G_CALLBACK (ubuntuone_nautilus_subscribe_folder),
-			  cb_data);
+			  uon->cb_data);
       }
     } else {
       if (is_dir) {
@@ -721,7 +723,7 @@
 
         g_signal_connect (urlitem, "activate",
                           G_CALLBACK (ubuntuone_nautilus_copy_public_url),
-                          cb_data);
+                          uon->cb_data);
         item = nautilus_menu_item_new ("ubuntuone-unpublish",
                                        _("Stop _Publishing"),
                                        _("No longer share this file with everyone via Ubuntu One."),
@@ -729,17 +731,17 @@
         if (is_pending)
           g_object_set (item, "sensitive", FALSE, NULL);
 
-        cb_data->make_public = FALSE;
+        uon->cb_data->make_public = FALSE;
       } else {
         item = nautilus_menu_item_new ("ubuntuone-publish",
                                        _("_Publish"),
                                        _("Make this file available to anyone via Ubuntu One."),
                                        "ubuntuone");
-        cb_data->make_public = TRUE;
+        uon->cb_data->make_public = TRUE;
       }
       g_signal_connect (item, "activate",
                         G_CALLBACK (ubuntuone_nautilus_toggle_publicity),
-                        cb_data);
+                        uon->cb_data);
     }
     if (!urlitem) {
       urlitem = nautilus_menu_item_new ("ubuntuone-geturl",
@@ -778,7 +780,7 @@
     }
     g_signal_connect (item, "activate",
                       G_CALLBACK (toggle_location),
-                      cb_data);
+                      uon->cb_data);
     nautilus_menu_append_item (submenu, item);
   }
 
@@ -876,7 +878,7 @@
 static void ubuntuone_nautilus_finalize(GObject * object) {
   UbuntuOneNautilus * uon = UBUNTUONE_NAUTILUS(object);
 
-  __cb_data_free (uon->cb_data);
+  __cb_data_free (uon->cb_data, TRUE);
 
   if (uon->syncdaemon)
     g_object_unref (uon->syncdaemon);