two crashes while editing nodes (one of those is reported as bug 1453558).

* fixed by making SPKnotHolder a referenced class and adding ref/unref in knot_click_handler (which sometime change attributes and knot handler is destroyed before calling knotholder_update_knots);
* fixed another crash by adding ref/unref to sp_knot_handler
* also added signal disconnecting

(bzr r445)

5 files changed, 82 insertions, 19 deletions

diff --git a/src/knot-holder-entity.h b/src/knot-holder-entity.h
index 1e9001e67..bbc0c5634 100644
--- a/src/knot-holder-entity.h
+++ b/src/knot-holder-entity.h
@@ -29,6 +29,10 @@ struct SPKnotHolderEntity {
 
     /** Connection to \a knot's "moved" signal. */
     guint   handler_id;
+    /** Connection to \a knot's "clicked" signal. */
+    guint   _click_handler_id;
+    /** Connection to \a knot's "ungrabbed" signal. */
+    guint   _ungrab_handler_id;
 
     /**
      * Called solely from knot_moved_handler.
diff --git a/src/knot.cpp b/src/knot.cpp
index 01ccca693..95cadfdb7 100644
--- a/src/knot.cpp
+++ b/src/knot.cpp
@@ -195,6 +195,7 @@ static void sp_knot_init(SPKnot *knot)
     knot->shape = SP_KNOT_SHAPE_SQUARE;
     knot->mode = SP_KNOT_MODE_XOR;
     knot->tip = NULL;
+    knot->_event_handler_id = 0;
 
     knot->fill[SP_KNOT_STATE_NORMAL] = 0xffffff00;
     knot->fill[SP_KNOT_STATE_MOUSEOVER] = 0xff0000ff;
@@ -228,6 +229,12 @@ static void sp_knot_dispose(GObject *object)
         gdk_pointer_ungrab (GDK_CURRENT_TIME);
     }
 
+    if (knot->_event_handler_id > 0)
+        {
+        g_signal_handler_disconnect(GTK_OBJECT (knot->item), knot->_event_handler_id);
+        knot->_event_handler_id = 0;
+        }
+
     if (knot->item) {
         gtk_object_destroy (GTK_OBJECT (knot->item));
         knot->item = NULL;
@@ -280,6 +287,7 @@ static int sp_knot_handler(SPCanvasItem *item, GdkEvent *event, SPKnot *knot)
     g_assert(knot != NULL);
     g_assert(SP_IS_KNOT(knot));
 
+    g_object_ref(G_OBJECT(knot));
     tolerance = prefs_get_int_attribute_limited("options.dragtolerance", "value", 0, 0, 100);
 
     gboolean consumed = FALSE;
@@ -424,6 +432,7 @@ static int sp_knot_handler(SPCanvasItem *item, GdkEvent *event, SPKnot *knot)
             break;
     }
 
+    g_object_unref(G_OBJECT(knot));
 
     return consumed;
 }
@@ -454,8 +463,8 @@ SPKnot *sp_knot_new(SPDesktop *desktop, const gchar *tip)
                                     "mode", SP_KNOT_MODE_XOR,
                                     NULL);
 
-    gtk_signal_connect(GTK_OBJECT(knot->item), "event",
-                       GTK_SIGNAL_FUNC(sp_knot_handler), knot);
+    knot->_event_handler_id = gtk_signal_connect(GTK_OBJECT(knot->item), "event",
+                                                 GTK_SIGNAL_FUNC(sp_knot_handler), knot);
 
     return knot;
 }
diff --git a/src/knot.h b/src/knot.h
index 795eeb8e3..5693417fd 100644
--- a/src/knot.h
+++ b/src/knot.h
@@ -62,6 +62,8 @@ struct SPKnot {
 
     gchar *tip;
 
+    gulong _event_handler_id;
+
     //TODO: all the members above should eventualle become private, accessible via setters/getters
     inline void setSize (guint i) {size = i;}
     inline void setShape (guint i) {shape = (SPKnotShapeType) i;}
diff --git a/src/knotholder.cpp b/src/knotholder.cpp
index 0ea3ed98a..82d49d223 100644
--- a/src/knotholder.cpp
+++ b/src/knotholder.cpp
@@ -27,6 +27,9 @@ class SPDesktop;
 static void knot_clicked_handler (SPKnot *knot, guint state, gpointer data);
 static void knot_moved_handler(SPKnot *knot, NR::Point const *p, guint state, gpointer data);
 static void knot_ungrabbed_handler (SPKnot *knot, unsigned int state, SPKnotHolder *kh);
+static void sp_knot_holder_class_init(SPKnotHolderClass *klass);
+
+void sp_knot_holder_dispose(GObject *object);
 
 #ifdef KNOT_HOLDER_DEBUG
 
@@ -36,6 +39,41 @@ static void sp_knot_holder_debug(GtkObject *object, gpointer data)
 }
 #endif
 
+static GObjectClass *parent_class;
+
+/**
+ * Registers SPKnotHolder class and returns its type number.
+ */
+GType sp_knot_holder_get_type()
+{
+    static GType type = 0;
+    if (!type) {
+        GTypeInfo info = {
+            sizeof(SPKnotHolderClass),
+            NULL,	/* base_init */
+            NULL,	/* base_finalize */
+            (GClassInitFunc) sp_knot_holder_class_init,
+            NULL,	/* class_finalize */
+            NULL,	/* class_data */
+            sizeof (SPKnotHolder),
+            16,	/* n_preallocs */
+            NULL,
+            NULL
+        };
+        type = g_type_register_static (G_TYPE_OBJECT, "SPKnotHolder", &info, (GTypeFlags) 0);
+    }
+    return type;
+}
+
+/**
+ * SPKnotHolder vtable initialization.
+ */
+static void sp_knot_holder_class_init(SPKnotHolderClass *klass){
+    GObjectClass *object_class = (GObjectClass *) klass;
+    parent_class = (GObjectClass*) g_type_class_peek_parent(klass);
+    object_class->dispose = sp_knot_holder_dispose;
+}
+
 SPKnotHolder *sp_knot_holder_new(SPDesktop *desktop, SPItem *item, SPKnotHolderReleasedFunc relhandler)
 {
     Inkscape::XML::Node *repr = SP_OBJECT(item)->repr;
@@ -44,7 +82,7 @@ SPKnotHolder *sp_knot_holder_new(SPDesktop *desktop, SPItem *item, SPKnotHolderR
     g_return_val_if_fail(item != NULL, NULL);
     g_return_val_if_fail(SP_IS_ITEM(item), NULL);
 
-    SPKnotHolder *knot_holder = g_new(SPKnotHolder, 1);
+    SPKnotHolder *knot_holder = (SPKnotHolder*)g_object_new (SP_TYPE_KNOT_HOLDER, 0);
     knot_holder->desktop = desktop;
     knot_holder->item = item;
     g_object_ref(G_OBJECT(item));
@@ -62,22 +100,25 @@ SPKnotHolder *sp_knot_holder_new(SPDesktop *desktop, SPItem *item, SPKnotHolderR
     return knot_holder;
 }
 
-void sp_knot_holder_destroy(SPKnotHolder *kh)
-{
-    if (kh) {
-        g_object_unref(G_OBJECT(kh->item));
-        while (kh->entity) {
-            SPKnotHolderEntity *e = (SPKnotHolderEntity *) kh->entity->data;
-            /* unref should call destroy */
-            g_object_unref(G_OBJECT(e->knot));
-            g_free(e);
-            kh->entity = g_slist_remove(kh->entity, e);
-        }
-
-        g_free(kh);
+void sp_knot_holder_dispose(GObject *object) {
+    SPKnotHolder *kh = G_TYPE_CHECK_INSTANCE_CAST((object), SP_TYPE_KNOT_HOLDER, SPKnotHolder);
+
+    g_object_unref(G_OBJECT(kh->item));
+    while (kh->entity) {
+        SPKnotHolderEntity *e = (SPKnotHolderEntity *) kh->entity->data;
+        g_signal_handler_disconnect(GTK_OBJECT (e->knot), e->_click_handler_id);
+        g_signal_handler_disconnect(GTK_OBJECT (e->knot), e->_ungrab_handler_id);
+        /* unref should call destroy */
+        g_object_unref(G_OBJECT(e->knot));
+        g_free(e);
+        kh->entity = g_slist_remove(kh->entity, e);
     }
 }
 
+void sp_knot_holder_destroy(SPKnotHolder *kh) {
+    g_object_unref(G_OBJECT(kh));
+    }
+
 void sp_knot_holder_add(
     SPKnotHolder *knot_holder,
     SPKnotHolderSetFunc knot_set,
@@ -130,8 +171,8 @@ void sp_knot_holder_add_full(
     sp_knot_set_position(e->knot, &dp, SP_KNOT_STATE_NORMAL);
 
     e->handler_id = g_signal_connect(G_OBJECT(e->knot), "moved", G_CALLBACK(knot_moved_handler), knot_holder);
-    g_signal_connect(G_OBJECT(e->knot), "clicked", G_CALLBACK(knot_clicked_handler), knot_holder);
-    g_signal_connect(G_OBJECT(e->knot), "ungrabbed", G_CALLBACK(knot_ungrabbed_handler), knot_holder);
+    e->_click_handler_id = g_signal_connect(G_OBJECT(e->knot), "clicked", G_CALLBACK(knot_clicked_handler), knot_holder);
+    e->_ungrab_handler_id = g_signal_connect(G_OBJECT(e->knot), "ungrabbed", G_CALLBACK(knot_ungrabbed_handler), knot_holder);
 
 #ifdef KNOT_HOLDER_DEBUG
     g_signal_connect(ob, "destroy", sp_knot_holder_debug, "SPKnotHolder::knot");
@@ -163,6 +204,7 @@ static void knot_clicked_handler(SPKnot *knot, guint state, gpointer data)
     SPKnotHolder *knot_holder = (SPKnotHolder *) data;
     SPItem *item  = SP_ITEM (knot_holder->item);
 
+    g_object_ref(G_OBJECT(knot_holder));
     for (GSList *el = knot_holder->entity; el; el = el->next) {
         SPKnotHolderEntity *e = (SPKnotHolderEntity *) el->data;
         if (e->knot == knot) {
@@ -178,6 +220,7 @@ static void knot_clicked_handler(SPKnot *knot, guint state, gpointer data)
     }
 
     knotholder_update_knots(knot_holder, item);
+    g_object_unref(G_OBJECT(knot_holder));
 
     // for drag, this is done by ungrabbed_handler, but for click we must do it here
     sp_document_done(SP_OBJECT_DOCUMENT(knot_holder->item));
diff --git a/src/knotholder.h b/src/knotholder.h
index 6980e2fdd..971dae3b0 100644
--- a/src/knotholder.h
+++ b/src/knotholder.h
@@ -32,7 +32,7 @@ typedef NR::Point (* SPKnotHolderGetFunc) (SPItem *item);
 /* fixme: Think how to make callbacks most sensitive (Lauris) */
 typedef void (* SPKnotHolderReleasedFunc) (SPItem *item);
 
-struct SPKnotHolder {
+struct SPKnotHolder : GObject {
     SPDesktop *desktop;
     SPItem *item;
     GSList *entity;
@@ -44,6 +44,8 @@ struct SPKnotHolder {
     gboolean local_change; ///< if true, no need to recreate knotholder if repr was changed.
 };
 
+struct SPKnotHolderClass : GObjectClass {
+};
 
 /* fixme: As a temporary solution, if released is NULL knotholder flushes undo itself (Lauris) */
 SPKnotHolder *sp_knot_holder_new(SPDesktop *desktop, SPItem *item, SPKnotHolderReleasedFunc relhandler);
@@ -64,6 +66,9 @@ void sp_knot_holder_add_full(SPKnotHolder *knot_holder,
                              SPKnotModeType mode,
                              gchar const *tip);
 
+GType sp_knot_holder_get_type();
+
+#define SP_TYPE_KNOT_HOLDER      (sp_knot_holder_get_type())
 
 #endif /* !__SP_KNOTHOLDER_H__ */