Thanks, this one I understand. :-D Prioritizing P3.

*** Bug 620 has been marked as a duplicate of this bug. ***

I would add that bug 620 contains a very nice overview of the bug.

Unfortunately, I don't think an easy solution is feasible. This is because due
to the fact that QModelIndex has a (non-virtual) copy constructor, when PySide
creates a QModelIndex and passes it to C++ code, it has no idea when the
reference count of QModelIndex.internalPointer() should be incremented or
decremented, and so it cannot know if the object should be destroyed or not.

Without cooperation from Qt, I'm not optimistic about the possibility of fixing
this bug.

Although I agree about the general issue of QModelIndex in fact not knowing
whether it stores a pointer or not, I believe a solution should be possible
even without altering the QModelIndex class.

The only way to create a QModelIndex with a custom ID (->internalId()) or a
pointer to a PyObject (->internalPointer()) is through the
QAbstractModelItem::createIndex method. The Shiboken wrapper checks for exactly
that overload, which is why it knows that a pointer is intended to be set in
the QModelIndex. At this point, the refcount can safely be incremented (see
#620) on the object.

Now it would be necessary to mark the fact that we actually stored a pointer,
to allow properly decrement it during destruction of the python wrapper. It
should be possible to define a member on the wrapper itself - the wrapped
QModelIndex doesn't have any field which could be used for this (and probably
it shouldn't as it is not its problem at all).

The copying within python appears to be handled through the ___copy__ function,
there such an additional member on the source wrapper could be checked to
assure the refcount is updated too.

The question is whether there is a nice way to define such an additional member
on the QModelIndex wrapper using shiboken. Currently I would try to do it using
its code-insertion feature, but I must admit that I don't know much about its
actual capabilities.

If you once again come to the conclusion that a fix is not feasible, a valid
fix should be to update the documentation of the QModelIndex wrapper as well as
to to add a new entry in the pitfalls area. 
If people know, they can more or less easily work around the issue - I used the
internalId() feature of the QModelIndex for instance, which maps to a list
which in turn contains the items I would have wanted to store using the
internalPointer().

Kind Regards, 
Sebastian

I think your solution would work only if Python-created QModelIndex were
manipulated only by Python code.

Take this example:
1. a C++ view calls your QAbstractItemModel's index() method to obtain a
QModelIndex
2. your Python code calls the createIndex method, which creates a QModelIndex
wrapped in a Shiboken structure specifying it was created with Python
3. your Python code returns, deleting the wrapping structure because its
reference code has dropped to zero
4. C++ code obtains the unwrapped QModelIndex
5. C++ code stores the QModelIndex in a variable, therefore calling its copy
constructor
6. C++ code destroys its temporary QModelIndex
7. at some point, C++ code changes the content of the variable, destroying the
last remaining instance of the QModelIndex

The issue is that the Shiboken wrapping code has no way to be aware of steps
5-7, so it can't know whether it's OK to destroy the internalPointer().

Worse, since QModelIndex's copy constructor is inlined, there is absolutely no
way of fixing this without changing Qt's ABI, so this can't possibly happen
before Qt 5.

Qt does give the guarantee that a QModelIndex shouldn't be kept for too long,
that is, they can be invalidated when the model changes, but it's not clear how
that constraint would translate to PySide. It's also of no help if the model is
used in a read-only way... And even if we had a reasonable bound for
QModelIndex lifetime, it would only shift the issue to QPersistentModelIndex,
although it is possible to change QPersistentModelIndex's behavior without
breaking the ABI due to usage of a private data structure and non-inline
constructor.


In the short term, I agree with you that it should be documented. Also, it
should be easy to make a few sanity checks on internalPointer(): make sure the
Python headers don't look awfully corrupted, make sure the reference count is
not 0. None of these will change the fact that memory safety is broken and that
it's possible to have segfaults, but at least it will catch part of these
errors and help the programmer understand them. It shouldn't be a normal
exception, since it would be very dangerous for a program to depend on being
able to catch them.

Thanks for your detailed explanation, indeed the chances are quite high that I
would run into the false assumption that the QModelIndex is never used except
for through its original wrapper. If a copy of the QModelIndex's instance is
ever copied within c++ and rewrapped, the additional information would
inevitably be lost.

According to some voices in forums, PyQT suffers from exactly the same issue
(http://www.mail-archive.com/pyqt@riverbankcomputing.com/msg16040.html), and
judging from the documentation
(http://www.riverbankcomputing.co.uk/static/Docs/PyQt4/html/qmodelindex.html)
it doesn't point it out either.

I'd be glad if PySide could point this issue out in its documentation, as using
internalPointer() is very tempting for many model programmers, some sort of
error checking to help finding the issue would of course be a great addition to
that.

Thank you, 
Sebastian

A warning about this problem was inserted on API documentation, to avoid future
mistakes.

Fixed on pyside commit:

commit a44fcc6dbacdc56438360dcaed00ed25021e45f0
Author: Renato Araujo Oliveira Filho <renato.filho@openbossa.org>
Date:   Tue Feb 1 15:48:42 2011 -0300

released on beta 5