From f21e3764144a94d3c2b8ed1f475fcd9010203c5b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Matthias=20K=C3=B6fferlein?= <matthias@koefferlein.de>
Date: Wed, 16 Mar 2022 23:27:25 +0100
Subject: [PATCH] Issue 1029 - Crash on QTreeViewItem#setBackgroundColor
 (#1032)

* Fixed issue #1029

The problem happened because in the described case the
tl::Variant used as a intermediate container holds the
Python QBrush object and when it gets deleted, the
QBrush object is deleted too.

* Added tests
---
 src/pya/pya/pyaMarshal.cc    | 12 +++++++++---
 src/rba/rba/rbaMarshal.cc    | 12 +++++++++---
 testdata/python/qtbinding.py |  9 +++++++++
 testdata/ruby/qtbinding.rb   | 12 ++++++++++++
 4 files changed, 39 insertions(+), 6 deletions(-)

diff --git a/src/pya/pya/pyaMarshal.cc b/src/pya/pya/pyaMarshal.cc
index e583d1484..e170cc2fa 100644
--- a/src/pya/pya/pyaMarshal.cc
+++ b/src/pya/pya/pyaMarshal.cc
@@ -114,6 +114,7 @@ public:
 
   virtual tl::Variant var () const;
   virtual void set (const tl::Variant &v);
+  const PythonPtr &ptr () const { return m_var; }
 
 private:
   PythonPtr m_var;
@@ -641,7 +642,7 @@ struct reader<gsi::ByteArrayType>
 };
 
 static
-PyObject *object_from_variant (tl::Variant &var, PYAObjectBase *self, const gsi::ArgType &atype)
+PyObject *object_from_variant (tl::Variant &var, PYAObjectBase *self, const gsi::ArgType &atype, bool transfer = false)
 {
   if (var.is_user()) {
 
@@ -657,7 +658,7 @@ PyObject *object_from_variant (tl::Variant &var, PYAObjectBase *self, const gsi:
     void *obj = var.to_user ();
     const gsi::ClassBase *cls = var.user_cls ()->gsi_cls ();
 
-    if (pass_obj) {
+    if (pass_obj || transfer) {
 
       if (holder) {
 
@@ -711,12 +712,17 @@ struct reader<gsi::VariantType>
       *ret = PythonRef (Py_None, false /*borrowed*/);
     } else {
       gsi::VariantAdaptorImpl<tl::Variant> *aa = dynamic_cast<gsi::VariantAdaptorImpl<tl::Variant> *> (a.get ());
+      PythonBasedVariantAdaptor *pa = dynamic_cast<PythonBasedVariantAdaptor *> (a.get ());
       if (aa) {
         //  A small optimization that saves one variant copy
         *ret = object_from_variant (aa->var_ref_nc (), self, atype);
+      } else if (pa) {
+        //  Optimization for Python to Python transfer
+        *ret = pa->ptr ();
       } else {
         tl::Variant v = a->var ();
-        *ret = object_from_variant (v, self, atype);
+        //  NOTE: as v may hold the object, we need to transfer ownership
+        *ret = object_from_variant (v, self, atype, true);
       }
     }
   }
diff --git a/src/rba/rba/rbaMarshal.cc b/src/rba/rba/rbaMarshal.cc
index 72eb3c570..3486f27f5 100644
--- a/src/rba/rba/rbaMarshal.cc
+++ b/src/rba/rba/rbaMarshal.cc
@@ -125,6 +125,7 @@ public:
 
   virtual tl::Variant var () const;
   virtual void set (const tl::Variant &v);
+  VALUE value () const { return m_var; }
 
 private:
   VALUE m_var;
@@ -662,7 +663,7 @@ struct reader<gsi::ByteArrayType>
   }
 };
 
-static VALUE object_from_variant (tl::Variant &var, Proxy *self, const gsi::ArgType &atype)
+static VALUE object_from_variant (tl::Variant &var, Proxy *self, const gsi::ArgType &atype, bool transfer = false)
 {
   if (var.is_user()) {
 
@@ -678,7 +679,7 @@ static VALUE object_from_variant (tl::Variant &var, Proxy *self, const gsi::ArgT
     void *obj = var.to_user ();
     const gsi::ClassBase *cls = var.user_cls ()->gsi_cls ();
 
-    if (pass_obj) {
+    if (pass_obj || transfer) {
 
       if (holder) {
 
@@ -732,12 +733,17 @@ struct reader<gsi::VariantType>
       *ret = Qnil;
     } else {
       gsi::VariantAdaptorImpl<tl::Variant> *aa = dynamic_cast<gsi::VariantAdaptorImpl<tl::Variant> *> (a.get ());
+      RubyBasedVariantAdaptor *pa = dynamic_cast<RubyBasedVariantAdaptor *> (a.get ());
       if (aa) {
         //  A small optimization that saves one variant copy
         *ret = object_from_variant (aa->var_ref_nc (), self, atype);
+      } else if (pa) {
+        //  Optimization for Ruby to Ruby transfer
+        *ret = pa->value ();
       } else {
         tl::Variant v = a->var ();
-        *ret = object_from_variant (v, self, atype);
+        //  NOTE: as v may hold the object, we need to transfer ownership
+        *ret = object_from_variant (v, self, atype, true);
       }
     }
   }
diff --git a/testdata/python/qtbinding.py b/testdata/python/qtbinding.py
index 78b562687..373bdde86 100644
--- a/testdata/python/qtbinding.py
+++ b/testdata/python/qtbinding.py
@@ -640,6 +640,15 @@ class QtBindingTest(unittest.TestCase):
     else:
       self.assertEqual(str(jsonData), 'b\'{"test":"test"}\'')
 
+  def test_54(self):
+
+    # issue #1029 (Crash for QBrush passed to setData)
+    item = pya.QTreeWidgetItem()
+    item.setBackground(0, pya.QBrush(pya.QColor(0xFF, 0xFF, 0x00)))
+    self.assertEqual(item.background(0).color.red, 255)
+    self.assertEqual(item.background(0).color.green, 255)
+    self.assertEqual(item.background(0).color.blue, 0)
+
 # run unit tests
 if __name__ == '__main__':
   suite = unittest.TestLoader().loadTestsFromTestCase(QtBindingTest)
diff --git a/testdata/ruby/qtbinding.rb b/testdata/ruby/qtbinding.rb
index b503dc553..1bf3d6d1b 100644
--- a/testdata/ruby/qtbinding.rb
+++ b/testdata/ruby/qtbinding.rb
@@ -755,6 +755,18 @@ class QtBinding_TestClass < TestBase
 
   end
 
+  def test_54
+
+    # issue #1029 (Crash for QBrush passed to setData)
+
+    item = RBA::QTreeWidgetItem::new
+    item.setBackground(0, RBA::QBrush::new(RBA::QColor::new(0xFF, 0xFF, 0x00)))
+    assert_equal(item.background(0).color.red, 255)
+    assert_equal(item.background(0).color.green, 255)
+    assert_equal(item.background(0).color.blue, 0)
+
+  end
+
 end 
 
 load("test_epilogue.rb")