Practicing what I preach

Two days ago I came across a Qt bug about adding extra columns in proxy models.

Gabriel de Dietrich is right to think QSortFilterProxyModel is not right for the job of adding extra columns. There are a few dangerous corners in using it for that purpose, such as proper handling of ranges in dataChanged signals, added complications from possible reordering of columns, and the one I mentioned in the bug report – that is – when the source model changes its layout, the QSortFilterProxyModel will only change the layout of the columns that the source model knows about, but not your extra columns, which could lead to a crash in various situations. I also mentioned that although it’s a bad idea, we in Akonadi also have a proxy which does such a thing for adding statistics columns in kmail (unread count, size etc). We also didn’t handle the crash-case I mentioned in the bug report.

Then yesterday I started working on a crash in akonadiconsole, a development tool which uses the same statistics proxy model. The crash happened when moving items between different folders. That in turn caused an intermediate proxy to perform a layout change, which was signaled to our statistics proxy, which then lead to a crash with this backtrace:

Thread 1 (Thread 0xb2b33700 (LWP 16733)):
[KCrash Handler]
#7  0xb60a5a37 in QSortFilterProxyModelPrivate::index_to_iterator (this=0x9eb3be0, proxy_index=...) at /home/kde-devel/kde/src/qt47/src/gui/itemviews/qsortfilterproxymodel.cpp:193
#8  0xb60a1dce in QSortFilterProxyModel::parent (this=0x9eb4028, child=...) at /home/kde-devel/kde/src/qt47/src/gui/itemviews/qsortfilterproxymodel.cpp:1654
#9  0xb7569834 in QModelIndex::parent (this=0x49455467) at /home/kde-devel/kde/qt47/include/QtCore/qabstractitemmodel.h:389
#10 0xb7565e96 in Future::KIdentityProxyModel::parent (this=0x9eb31c0, child=...) at /home/kde-devel/kde/src/KDE/kdepim/akonadi_next/kidentityproxymodel.cpp:353
#11 0xb5409448 in QModelIndex::parent (this=0xa9df368) at /home/kde-devel/kde/src/qt47/src/corelib/kernel/qabstractitemmodel.h:389
#12 0xb54025a2 in QPersistentModelIndex::parent (this=0xa933cf0) at /home/kde-devel/kde/src/qt47/src/corelib/kernel/qabstractitemmodel.cpp:347
#13 0xb600f93b in QItemSelectionRange::parent (this=0xa933cf0) at ../../include/QtGui/../../../../src/qt47/src/gui/itemviews/qitemselectionmodel.h:78
#14 0xb606871e in QItemSelectionRange::contains (this=0xa933cf0, index=...) at /home/kde-devel/kde/src/qt47/src/gui/itemviews/qitemselectionmodel.h:85
#15 0xb6060d93 in QItemSelection::contains (this=0x9eb7890, index=...) at /home/kde-devel/kde/src/qt47/src/gui/itemviews/qitemselectionmodel.cpp:423
#16 0xb6065fed in QItemSelectionModel::isSelected (this=0x9eb7828, index=...) at /home/kde-devel/kde/src/qt47/src/gui/itemviews/qitemselectionmodel.cpp:1200
#17 0xb604b48c in QTreeView::drawRow (this=0x9e26310, painter=0xbfcbd7c8, option=..., index=...) at /home/kde-devel/kde/src/qt47/src/gui/itemviews/qtreeview.cpp:1602
#18 0xb604a861 in QTreeView::drawTree (this=0x9e26310, painter=0xbfcbd7c8, region=...) at /home/kde-devel/kde/src/qt47/src/gui/itemviews/qtreeview.cpp:1441
#19 0xb6049ee3 in QTreeView::paintEvent (this=0x9e26310, event=0xbfcbe0e8) at /home/kde-devel/kde/src/qt47/src/gui/itemviews/qtreeview.cpp:1274
#20 0xb5a150e9 in QWidget::event (this=0x9e26310, event=0xbfcbe0e8) at /home/kde-devel/kde/src/qt47/src/gui/kernel/qwidget.cpp:8346

It’s not obvious where the bug is because it comes from some random paint event rather than something directly related to the move action. After some investigation I found that I was being hit by the exact issue I was describing in the bug report. A QPersistentModelIndex was corrupted and then some time later the QTreeView tried to draw the row for it. The fix was as described and only took a little time to write.

Another lesson that this nicely illustrates is that you need to Q_ASSERT as much as possible when using Qt model-view. Constructs like if (index.model() != this) return hide bugs and will result in you reading backtraces unrelated to the code causing the bug because a segfault or assert will occur later. Although KIdentityProxyModel appeared in the above backtrace it was unrelated to the problem. Assert as early as possible if you expect something from a QModelIndex.

About these ads

7 Responses to “Practicing what I preach”

  1. Andreas Says:

    I would extend the advice to Q_ASSERT() / assert() as much as possible to all other tricky situations where unittests can’t help you or are too much work for the benefit they’d provide.
    In at least one instance I’ve even added extra data structures and methods that do nothing but runtime conistency checking – only in debug builds, of course. If users had inexplicable bugs or crashes one could always send them a debug build to find out more. In the FOSS world that is possible most of the time.
    There is also the old advice that a program should “fail as early as possible”.

  2. steveire Says:

    Indeed, I also add assertion methods sometimes such as here:

    http://websvn.kde.org/trunk/KDE/kdelibs/kdeui/itemviews/kmodelindexproxymapper.cpp?r1=1174860&r2=1174859&pathrev=1174860

    So Q_ASSERT(assertSelectionValid(selection)); is properly compiled out in release mode.

    It would be nice to make that a file-static method but that means that it the unused method would generate a warning at compile time in release mode with gcc. I guess a dummy call is a possibility then, but it’s a bit ugly.

  3. Andreas Says:

    You can put the function body inside the braces into #ifdefs and maybe add some Q_UNUSED for the arguments. The call itself should be optimized out by a reasonably smart compiler. I haven’t checked, though.

  4. Tobias Says:

    Will this get changed in the Qt library? Any idea? I just stumbled over this bug and it took me a while to find the problem.

  5. steveire Says:

    Tobias,

    I’m not sure there’s anything to fix in Qt. What was the problem you found?

  6. Tobias Says:

    I had exactly this crash, with an invalid (already destroyed) iterator in QSortFilterProxyModelPrivate::index_to_iterator. I was using a unmodified QSortFilterProxyModel, with a static number of columns. I could reproduce the crash every time, but because it happened in the paint method, the trace what lead to the destroyed internal iterator was already lost.

    So I found your post, and because you mentioned the bug is related to the layout change, I simple removed the layoutChanged signals from my model to avoid this crash. This “solved” my immediate problem with the crash, but it seems there is anyway a serious bug in the QSortFilterProxyModel. And I’m using the latest 4.7.1 release version of Qt.

    In fact this caused the bug:
    void CommonItemGroupManagerModel::onInsertedItems( int groupId, int, int )
    {
    if( ! hasItemGroupManager() )
    return;

    endInsertRows();

    emit layoutChanged(); // <<<<<<< — caused the crash
    }

    Probably my fault, because the layoutAboutToBeChanged() signal was misplaced, but it should still not lead to a crash.

  7. steveire Says:

    I don’t think anything can be done in Qt to hide issues like that in your code. You just need to use valgrind to find and fix the issues in your code.

    Misplaced calls like the one you point out should be expected to cause crashes I think. You can’t do just anything in a model implementation and expect it to not break. That’s what makes it hard in the real world.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s


Follow

Get every new post delivered to your Inbox.

%d bloggers like this: