Uploaded image for project: 'Qt for Python'
  1. Qt for Python
  2. PYSIDE-1251

QTreeWidget clear() sets parents to null incorrectly

    XMLWordPrintable

Details

    • Bug
    • Resolution: Done
    • P2: Important
    • None
    • 5.14.1
    • PySide
    • None
    • 502dfcf352b0094e2b1e8b495af1cdeb3e7874ed (pyside/pyside-setup/5.14)

    Description

      In pyside-setup/sources/pyside2/PySide2/glue/qtwidgets.cpp, there is this:

      // @snippet qtreewidget-clear
      QTreeWidgetItem *rootItem = %CPPSELF.invisibleRootItem();
      Shiboken::BindingManager &bm = Shiboken::BindingManager::instance();
      for (int i = 0, i_count = rootItem->childCount(); i < i_count; ++i) {
          QTreeWidgetItem *item = rootItem->child(i);
          if (SbkObject *wrapper = bm.retrieveWrapper(item))
              Shiboken::Object::setParent(nullptr, reinterpret_cast<PyObject *>(wrapper));
      }
      // @snippet qtreewidget-clear
      

      The problem is that calling setParent can result in the item being deleted, if the only reference to the item is from the tree itself (ie, the parent.) If the item is deleted (keep in mind that it may or may not be deleted - depends on the ref count), then the childCount will change and the index shouldn't be incremented at the bottom of the loop. The bottom line is that we're iterating over the list of children and potentially modifying that list as we go.

      This snippet gets inserted into the overridden clear() function for the QTreeWidgetWrapper class that derives from QTreeWidget. It does the above, then calls QTreeWidget's clear() function.

      The thing is that QTreeWidget's clear() function deletes all of the tree's items. Why bother setting the parents of the children to null, when those children are about to be deleted in any case?

      So rather than somehow fixing the above code, I would propose simply removing it. Just use QTreeWidget's clear() function and don't bother setting the children's parents before the base clear() is called.

      For what it's worth, I'm seeing a crash that goes away when I remove the above code. But I don't know exactly how the crash is happening. Memory is corrupted somehow and I don't know where. The above loop doesn't directly crash. But if I remove the above snippet, then the crash goes away. This crash occurs for me in a complex application. If I try to make a small test program that crashes, I can't do it. But if I put in printfs, I can see that the loop is definitely behaving incorrectly. For example, if the children are items with text "one", "two" and "three", then I can see that only "one" and "three" have their parents set to null (assuming that there are no additional references to the items.)

      By the way - if there's a reason why setParent really needs to be called, in spite of the fact that the children are about to be deleted by QTreeWidget's clear() function, I would be curious to know what that reason is! In that case, the loop needs to be changed to account for the fact that calling setParent may (or may not!) cause the item to be deleted.

      Attachments

        No reviews matched the request. Check your Options in the drop-down menu of this sections header.

        Activity

          People

            crmaurei Cristian Maureira-Fredes
            jyost Jeffery Yost
            Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

              Created:
              Updated:
              Resolved:

              Gerrit Reviews

                There are no open Gerrit changes