[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-bindings
Subject:    [Kde-bindings] [PATCH] Deferred objects and QGraphicsEffects
From:       Paolo Capriotti <p.capriotti () gmail ! com>
Date:       2010-03-17 0:24:56
Message-ID: 87b37d61003161724l5fc24582n380fe45102cab5dc () mail ! gmail ! com
[Download RAW message or body]

Hi all,
the attached patch solves two issues with QGraphicsEffects.
Background: a QGraphicEffect instance can be associated to a
QGraphicsItem by calling QGraphicsItem::setGraphicsEffect. When you do
that, QGraphicsItem takes ownership of the effect instance, and will
delete it in its destructor. The issues are:
1) When an item is deleted (say by the GC), the associated effect is
deleted as well, and Binding::deleted should take care of cleaning up
its smokeruby_object and unmap the pointer. Unfortunately, it may
happen that the effect has also been scheduled for finalization, and
it seems that in this case, ruby changes the type of the object (in
RBasic::flag) to T_DEFERRED, and invalidates the pointer. This implies
that we cannot use the VALUE stored in the pointer map to clean up.
What's currently happening is that the smokeruby_object doesn't get
set as deallocated (ptr = 0) as it should, and is then accessed when
the GC finally decides to call its finalizer => boom!
My proposed solution (in the patch) is to store both the
smokeruby_object _and_ the ruby VALUE in the map. So even if the VALUE
is invalidated, you can still access the smokeruby_object directly and
clean up. This fixes any instance of the crash I've seen.
2) The second issue is more straightforward: QGraphicsItem can
reference a QGraphicsEffect instance, so the latter needs to be marked
(in the mark phase of the GC) whenever the former is.

Please note that the first issue is not related only to
QGraphicsEffects. I think that in theory you could get a crash in any
situation when an object is deferred by ruby, deleted by Qt, and
finally finalized by qtruby, although I've never actually seen such a
crash except for the case with QGraphicsEffect.
To reproduce the crash, you can use the following code:

require 'Qt4'

loop do
  item = Qt::GraphicsItem.new
  effect = Qt::GraphicsEffect.new
  item.set_graphics_effect(effect)
end

Paolo

["qtruby-effects.patch" (text/x-patch)]

Index: ruby/qtruby/src/handlers.cpp
===================================================================
--- ruby/qtruby/src/handlers.cpp	(revision 1103840)
+++ ruby/qtruby/src/handlers.cpp	(working copy)
@@ -333,6 +333,14 @@
 			if (item->parentItem() == 0) {
 				mark_qgraphicsitem_children(item);
 			}
+			if (QGraphicsEffect* effect = item->graphicsEffect()) {
+				obj = getPointerObject(effect);
+				if (obj != Qnil) {
+				  if (do_debug & qtdb_gc) 
+					qWarning("Marking (%s*)%p -> %p", "QGraphicsEffect", effect, (void*)obj);
+					rb_gc_mark(obj);
+				}
+			}
 		}
 
 		if (o->smoke->isDerivedFrom(className, "QGraphicsScene")) {
Index: ruby/qtruby/src/Qt.cpp
===================================================================
--- ruby/qtruby/src/Qt.cpp	(revision 1103840)
+++ ruby/qtruby/src/Qt.cpp	(working copy)
@@ -99,7 +99,7 @@
 int do_debug = qtdb_none;
 #endif
 
-typedef QHash<void *, VALUE *> PointerMap;
+typedef QHash<void *, SmokeValue> PointerMap;
 Q_GLOBAL_STATIC(PointerMap, pointer_map)
 int object_count = 0;
 
@@ -149,6 +149,10 @@
 }
 
 VALUE getPointerObject(void *ptr) {
+  return getSmokeValue(ptr).value;
+}
+
+SmokeValue getSmokeValue(void *ptr) {
 	if (!pointer_map() || !pointer_map()->contains(ptr)) {
 		if (do_debug & qtdb_gc) {
 			qWarning("getPointerObject %p -> nil", ptr);
@@ -156,12 +160,12 @@
 				qWarning("getPointerObject pointer_map deleted");
 			}
 		}
-	    return Qnil;
+	    return SmokeValue();
 	} else {
 		if (do_debug & qtdb_gc) {
-			qWarning("getPointerObject %p -> %p", ptr, (void *) \
*(pointer_map()->operator[](ptr))); +			qWarning("getPointerObject %p -> %p", ptr, \
(void *) pointer_map()->operator[](ptr).value);  }
-		return *(pointer_map()->operator[](ptr));
+		return pointer_map()->operator[](ptr);
 	}
 }
 
@@ -170,15 +174,14 @@
 	if (ptr != lastptr) {
 		lastptr = ptr;
 		if (pointer_map() && pointer_map()->contains(ptr)) {
-			VALUE * obj_ptr = pointer_map()->operator[](ptr);
+			VALUE obj_ptr = pointer_map()->operator[](ptr).value;
 		
 			if (do_debug & qtdb_gc) {
 				const char *className = o->smoke->classes[o->classId].className;
-				qWarning("unmapPointer (%s*)%p -> %p size: %d", className, ptr, obj_ptr, \
pointer_map()->size() - 1); +				qWarning("unmapPointer (%s*)%p -> %p size: %d", \
className, ptr, (void*)&obj_ptr, pointer_map()->size() - 1);  }
 	    
 			pointer_map()->remove(ptr);
-			xfree((void*) obj_ptr);
 		}
     }
 
@@ -195,15 +198,14 @@
 	
     if (ptr != lastptr) {
 		lastptr = ptr;
-		VALUE * obj_ptr = ALLOC(VALUE);
-		memcpy(obj_ptr, &obj, sizeof(VALUE));
 		
 		if (do_debug & qtdb_gc) {
 			const char *className = o->smoke->classes[o->classId].className;
 			qWarning("mapPointer (%s*)%p -> %p size: %d", className, ptr, (void*)obj, \
pointer_map()->size() + 1);  }
 	
-		pointer_map()->insert(ptr, obj_ptr);
+		SmokeValue value(obj, o);
+		pointer_map()->insert(ptr, value);
     }
 	
 	for (Smoke::Index *i = o->smoke->inheritanceList + \
o->smoke->classes[classId].parents; *i; i++) { @@ -223,9 +225,9 @@
 	if (!pointer_map()) {
 	return;
 	}
-	VALUE obj = getPointerObject(ptr);
-	smokeruby_object *o = value_obj_info(obj);
+	smokeruby_object *o = getSmokeValue(ptr).o;
 	if (do_debug & qtdb_gc) {
+	  	qWarning("unmapping: o = %p, ptr = %p\n", o, ptr);
     	qWarning("%p->~%s()", ptr, smoke->className(classId));
     }
 	if (!o || !o->ptr) {
Index: ruby/qtruby/src/qtruby.h
===================================================================
--- ruby/qtruby/src/qtruby.h	(revision 1103840)
+++ ruby/qtruby/src/qtruby.h	(working copy)
@@ -70,6 +70,20 @@
     int classId;
 };
 
+struct SmokeValue
+{
+  VALUE value;
+  smokeruby_object* o;
+  
+  SmokeValue()
+  : value(Qnil)
+  , o(0) { }
+  
+  SmokeValue(VALUE value, smokeruby_object* o)
+  : value(value)
+  , o(o) { }
+};
+
 struct TypeHandler {
     const char *name;
     Marshall::HandlerFn fn;
@@ -140,6 +154,7 @@
 extern Q_DECL_EXPORT void *value_to_ptr(VALUE ruby_value); // ptr on success, null \
on fail  
 extern Q_DECL_EXPORT VALUE getPointerObject(void *ptr);
+extern Q_DECL_EXPORT SmokeValue getSmokeValue(void *ptr);
 extern Q_DECL_EXPORT void mapPointer(VALUE obj, smokeruby_object *o, Smoke::Index \
classId, void *lastptr);  extern Q_DECL_EXPORT void unmapPointer(smokeruby_object *, \
Smoke::Index, void*);  



_______________________________________________
Kde-bindings mailing list
Kde-bindings@kde.org
https://mail.kde.org/mailman/listinfo/kde-bindings


[prev in list] [next in list] [prev in thread] [next in thread]