Improve the 'Value' type wrapper registration

This commit works around some bugs where multiple/incorrect
wrappers were registered for some types:

- In cases where the first field of a class was itself a Value
  type instance (instead of pointer), both the parent
  and child wrappers had same cptr address, causing confusion
  in retrieveWrapper. Previously, this was worked around by always
  creating a new wrapper for all Value type fields on every access,
  causing leaks. We now check for colocated child wrappers and
  return that instead of creating new wrapper, so each Value type
  subfield need only have one wrapper.
- Some cases of incorrect wrapper registration due to an existing
  wrapper which shiboken could not figure being deleted are fixed,
  specifically cases where the newly registered wrapper is from
  object created in python or owns its wrapper.
- Do not release incorrect wrapper in case of address reuse
  by checking that the registered wrapper is indeed the wrapper
  being released.

Task-number: PYSIDE-217
Task-number: PYSIDE-224
Change-Id: I019c078566c4b9b90e63c5d991e2e372d39c632a
Reviewed-by: Alexandru Croitor <alexandru.croitor@qt.io>
Reviewed-by: Christian Tismer <tismer@stackless.com>

7 files changed, 152 insertions, 17 deletions

diff --git a/generator/shiboken2/cppgenerator.cpp b/generator/shiboken2/cppgenerator.cpp
index c8e919c..f0c2fe4 100644
--- a/generator/shiboken2/cppgenerator.cpp
+++ b/generator/shiboken2/cppgenerator.cpp
@@ -1577,6 +1577,14 @@ void CppGenerator::writeConstructorWrapper(QTextStream &s, const AbstractMetaFun
     // Python owns it and C++ wrapper is false.
     if (shouldGenerateCppWrapper(overloads.first()->ownerClass()))
         s << INDENT << "Shiboken::Object::setHasCppWrapper(sbkSelf, true);" << endl;
+    // Need to check if a wrapper for same pointer is already registered
+    // Caused by bug PYSIDE-217, where deleted objects' wrappers are not released
+    s << INDENT << "if (Shiboken::BindingManager::instance().hasWrapper(cptr)) {" << endl;
+    {
+        Indentation indent(INDENT);
+        s << INDENT << "Shiboken::BindingManager::instance().releaseWrapper(Shiboken::BindingManager::instance().retrieveWrapper(cptr));" << endl;
+    }
+    s << INDENT << "}" << endl;
     s << INDENT << "Shiboken::BindingManager::instance().registerWrapper(sbkSelf, cptr);" << endl;
 
     // Create metaObject and register signal/slot
@@ -4011,12 +4019,38 @@ void CppGenerator::writeGetterFunction(QTextStream &s,
         cppField = QLatin1String("fieldValue");
     }
 
-    s << INDENT << "PyObject* pyOut = ";
+    s << INDENT << "PyObject* pyOut = 0;\n";
     if (newWrapperSameObject) {
+        // Special case colocated field with same address (first field in a struct)
+        s << INDENT << "if (reinterpret_cast<void *>("
+                    << cppField
+                    << ") == reinterpret_cast<void *>("
+                    << CPP_SELF_VAR << ")) {\n";
+        {
+            Indentation indent(INDENT);
+            s << INDENT << "pyOut = (PyObject*)Shiboken::Object::findColocatedChild("
+                        << "(SbkObject*)self , (SbkObjectType*)"
+                        << cpythonTypeNameExt(fieldType)
+                        << ");" << "\n";
+            s << INDENT << "if (pyOut) {Py_IncRef(pyOut); return pyOut;}\n";
+        }
+        s << INDENT << "}\n";
+        // Check if field wrapper has already been created.
+        s << INDENT << "else if (Shiboken::BindingManager::instance().hasWrapper(" << cppField << ")) {" << "\n";
+        {
+            Indentation indent(INDENT);
+            s << INDENT << "pyOut = (PyObject*)Shiboken::BindingManager::instance().retrieveWrapper(" << cppField << ");" << "\n";
+            s << INDENT << "Py_IncRef(pyOut);" << "\n";
+            s << INDENT << "return pyOut;" << "\n";
+        }
+        s << INDENT << "}\n";
+        // Create and register new wrapper
+        s << INDENT << "pyOut = ";
         s << "Shiboken::Object::newObject((SbkObjectType*)" << cpythonTypeNameExt(fieldType);
         s << ", " << cppField << ", false, true);" << endl;
         s << INDENT << "Shiboken::Object::setParent(" PYTHON_SELF_VAR ", pyOut)";
     } else {
+        s << INDENT << "pyOut = ";
         writeToPythonConversion(s, fieldType, metaField->enclosingClass(), cppField);
     }
     s << ';' << endl;
diff --git a/libshiboken/basewrapper.cpp b/libshiboken/basewrapper.cpp
index 71abbb6..d82297d 100644
--- a/libshiboken/basewrapper.cpp
+++ b/libshiboken/basewrapper.cpp
@@ -1105,7 +1105,38 @@ bool isValid(PyObject* pyObj, bool throwPyError)
     return isValid(reinterpret_cast<SbkObject*>(pyObj), throwPyError);
 }
 
-PyObject* newObject(SbkObjectType* instanceType,
+SbkObject *findColocatedChild(SbkObject* wrapper,
+                              SbkObjectType* instanceType)
+{
+    // Degenerate case, wrapper is the correct wrapper.
+    if (reinterpret_cast<const void *>(Py_TYPE(wrapper)) == reinterpret_cast<const void *>(instanceType))
+        return wrapper;
+
+    if (!(wrapper->d && wrapper->d->cptr))
+        return 0;
+
+    ParentInfo* pInfo = wrapper->d->parentInfo;
+    if (!pInfo)
+        return 0;
+
+    ChildrenList& children = pInfo->children;
+
+    ChildrenList::iterator childrenEnd = children.end();
+    for (ChildrenList::iterator iChild = children.begin(); iChild != childrenEnd; ++iChild) {
+        if (!((*iChild)->d && (*iChild)->d->cptr))
+            continue;
+        if ((*iChild)->d->cptr[0] == wrapper->d->cptr[0]) {
+            if (reinterpret_cast<const void *>(Py_TYPE(*iChild)) == reinterpret_cast<const void *>(instanceType))
+                return const_cast<SbkObject *>((*iChild));
+            else
+                return findColocatedChild(const_cast<SbkObject *>(*iChild), instanceType);
+        }
+    }
+    return 0;
+}
+
+
+PyObject *newObject(SbkObjectType* instanceType,
                     void* cptr,
                     bool hasOwnership,
                     bool isExactType,
@@ -1123,11 +1154,45 @@ PyObject* newObject(SbkObjectType* instanceType,
             instanceType = BindingManager::instance().resolveType(&cptr, instanceType);
     }
 
-    SbkObject* self = reinterpret_cast<SbkObject*>(SbkObjectTpNew(reinterpret_cast<PyTypeObject*>(instanceType), 0, 0));
-    self->d->cptr[0] = cptr;
-    self->d->hasOwnership = hasOwnership;
-    self->d->validCppObject = 1;
-    BindingManager::instance().registerWrapper(self, cptr);
+    bool shouldCreate = true;
+    bool shouldRegister = true;
+    SbkObject* self = 0;
+
+    // Some logic to ensure that colocated child field does not overwrite the parent
+    if (BindingManager::instance().hasWrapper(cptr)) {
+        SbkObject* existingWrapper = BindingManager::instance().retrieveWrapper(cptr);
+
+        self = findColocatedChild(existingWrapper, instanceType);
+        if (self) {
+            // Wrapper already registered for cptr.
+            // This should not ideally happen, binding code should know when a wrapper
+            // already exists and retrieve it instead.
+            shouldRegister = shouldCreate = false;
+        } else if (hasOwnership &&
+                  (!(Shiboken::Object::hasCppWrapper(existingWrapper) ||
+                     Shiboken::Object::hasOwnership(existingWrapper)))) {
+            // Old wrapper is likely junk, since we have ownership and it doesn't.
+            BindingManager::instance().releaseWrapper(existingWrapper);
+        } else {
+            // Old wrapper may be junk caused by some bug in identifying object deletion
+            // but it may not be junk when a colocated field is accessed for an
+            // object which was not created by python (returned from c++ factory function).
+            // Hence we cannot release the wrapper confidently so we do not register.
+            shouldRegister = false;
+        }
+    }
+
+    if (shouldCreate) {
+        self = reinterpret_cast<SbkObject*>(SbkObjectTpNew(reinterpret_cast<PyTypeObject*>(instanceType), 0, 0));
+        self->d->cptr[0] = cptr;
+        self->d->hasOwnership = hasOwnership;
+        self->d->validCppObject = 1;
+        if (shouldRegister) {
+            BindingManager::instance().registerWrapper(self, cptr);
+        }
+    } else {
+        Py_IncRef(reinterpret_cast<PyObject*>(self));
+    }
     return reinterpret_cast<PyObject*>(self);
 }
 
diff --git a/libshiboken/basewrapper.h b/libshiboken/basewrapper.h
index 8152f98..3404198 100644
--- a/libshiboken/basewrapper.h
+++ b/libshiboken/basewrapper.h
@@ -252,6 +252,12 @@ LIBSHIBOKEN_API bool        isUserType(PyObject* pyObj);
 LIBSHIBOKEN_API Py_hash_t   hash(PyObject* pyObj);
 
 /**
+ * Find a child of given wrapper having same address having the specified type.
+ */
+LIBSHIBOKEN_API SbkObject *findColocatedChild(SbkObject* wrapper,
+                                              SbkObjectType* instanceType);
+
+/**
  *  Bind a C++ object to Python.
  * \param instanceType equivalent Python type for the C++ object.
  * \param hasOwnership if true, Python will try to delete the underlying C++ object when there's no more refs.
diff --git a/libshiboken/bindingmanager.cpp b/libshiboken/bindingmanager.cpp
index 5fbd11d..d7e122c 100644
--- a/libshiboken/bindingmanager.cpp
+++ b/libshiboken/bindingmanager.cpp
@@ -143,16 +143,22 @@ struct BindingManager::BindingManagerPrivate {
     bool destroying;
 
     BindingManagerPrivate() : destroying(false) {}
-    void releaseWrapper(void* cptr);
+    bool releaseWrapper(void* cptr, SbkObject* wrapper);
     void assignWrapper(SbkObject* wrapper, const void* cptr);
 
 };
 
-void BindingManager::BindingManagerPrivate::releaseWrapper(void* cptr)
+bool BindingManager::BindingManagerPrivate::releaseWrapper(void* cptr, SbkObject* wrapper)
 {
+    // The wrapper argument is checked to ensure that the correct wrapper is released.
+    // Returns true if the correct wrapper is found and released.
+    // If wrapper argument is NULL, no such check is performed.
     WrapperMap::iterator iter = wrapperMapper.find(cptr);
-    if (iter != wrapperMapper.end())
+    if (iter != wrapperMapper.end() && (wrapper == 0 || iter->second == wrapper)) {
         wrapperMapper.erase(iter);
+        return true;
+    }
+    return false;
 }
 
 void BindingManager::BindingManagerPrivate::assignWrapper(SbkObject* wrapper, const void* cptr)
@@ -234,12 +240,12 @@ void BindingManager::releaseWrapper(SbkObject* sbkObj)
     void** cptrs = reinterpret_cast<SbkObject*>(sbkObj)->d->cptr;
     for (int i = 0; i < numBases; ++i) {
         unsigned char *cptr = reinterpret_cast<unsigned char *>(cptrs[i]);
-        m_d->releaseWrapper(cptr);
+        m_d->releaseWrapper(cptr, sbkObj);
         if (d && d->mi_offsets) {
             int* offset = d->mi_offsets;
             while (*offset != -1) {
                 if (*offset > 0)
-                    m_d->releaseWrapper(cptr + *offset);
+                    m_d->releaseWrapper(reinterpret_cast<void *>((std::size_t) cptr + (*offset)), sbkObj);
                 offset++;
             }
         }
diff --git a/tests/libsample/protected.h b/tests/libsample/protected.h
index fe1d40f..c33bdf4 100644
--- a/tests/libsample/protected.h
+++ b/tests/libsample/protected.h
@@ -127,19 +127,25 @@ class LIBSAMPLE_API ProtectedProperty
 {
 public:
     ProtectedProperty()
-        : protectedProperty(0),
+        : protectedValueTypeProperty(Point(0, 0)),
+          protectedProperty(0),
           protectedEnumProperty(Event::NO_EVENT),
-          protectedValueTypeProperty(Point(0, 0)),
           protectedValueTypePointerProperty(0),
           protectedObjectTypeProperty(0)
     {}
 protected:
+    // This is deliberately the first member to test wrapper registration
+    // for value type members sharing the same memory address.
+    Point protectedValueTypeProperty;
     int protectedProperty;
     std::list<int> protectedContainerProperty;
     Event::EventType protectedEnumProperty;
-    Point protectedValueTypeProperty;
     Point* protectedValueTypePointerProperty;
     ObjectType* protectedObjectTypeProperty;
 };
 
+LIBSAMPLE_API inline ProtectedProperty* createProtectedProperty() {
+    return new ProtectedProperty;
+}
+
 #endif // PROTECTED_H
diff --git a/tests/samplebinding/protected_test.py b/tests/samplebinding/protected_test.py
index 43b7640..9201a63 100644
--- a/tests/samplebinding/protected_test.py
+++ b/tests/samplebinding/protected_test.py
@@ -36,7 +36,7 @@ import unittest
 from sample import cacheSize
 from sample import ProtectedNonPolymorphic, ProtectedVirtualDestructor
 from sample import ProtectedPolymorphic, ProtectedPolymorphicDaughter, ProtectedPolymorphicGrandDaughter
-from sample import ProtectedProperty, ProtectedEnumClass
+from sample import createProtectedProperty, ProtectedProperty, ProtectedEnumClass
 from sample import PrivateDtor
 from sample import Event, ObjectType, Point
 
@@ -300,7 +300,23 @@ class ProtectedPropertyTest(unittest.TestCase):
         self.assertEqual(self.obj.protectedValueTypeProperty, point)
         self.assertFalse(self.obj.protectedValueTypeProperty is point)
         pointProperty = self.obj.protectedValueTypeProperty
-        self.assertFalse(self.obj.protectedValueTypeProperty is pointProperty)
+        self.assertTrue(self.obj.protectedValueTypeProperty is pointProperty)
+
+    def testProtectedValueTypePropertyWrapperRegistration(self):
+        '''Access colocated protected value type property.'''
+        cache_size = cacheSize()
+        point = Point(12, 34)
+        obj = createProtectedProperty()
+        obj.protectedValueTypeProperty
+        self.assertEqual(obj.protectedValueTypeProperty.copy(),
+                         obj.protectedValueTypeProperty)
+        obj.protectedValueTypeProperty = point
+        self.assertEqual(obj.protectedValueTypeProperty, point)
+        self.assertFalse(obj.protectedValueTypeProperty is point)
+        pointProperty = obj.protectedValueTypeProperty
+        self.assertTrue(obj.protectedValueTypeProperty is pointProperty)
+        del obj, point, pointProperty
+        self.assertEqual(cacheSize(), cache_size)
 
     def testProtectedValueTypePointerProperty(self):
         '''Writes and reads a protected value type pointer property.'''
diff --git a/tests/samplebinding/typesystem_sample.xml b/tests/samplebinding/typesystem_sample.xml
index 601b4b0..089f835 100644
--- a/tests/samplebinding/typesystem_sample.xml
+++ b/tests/samplebinding/typesystem_sample.xml
@@ -878,6 +878,8 @@
 
     <value-type name="ProtectedProperty" />
 
+    <function signature="createProtectedProperty()" />
+
     <template name="boolptr_at_end_fix_beginning">
     bool __ok__;
     %RETURN_TYPE %0 = %CPPSELF.%TYPE::%FUNCTION_NAME(%ARGUMENT_NAMES, &amp;__ok__);