Summary: | KExtendableItemDelegate crashes in extendRect() on initial paint | ||
---|---|---|---|
Product: | [Unmaintained] kdelibs | Reporter: | Jonathan Thomas <echidnaman> |
Component: | kdeui | Assignee: | Andreas Hartmetz <ahartmetz> |
Status: | RESOLVED FIXED | ||
Severity: | crash | CC: | ahartmetz, fd.svensson, mirza.dervisevic, rdesfo, stavros.tsigas, vladimir.tuboltsev, yuberion |
Priority: | NOR | ||
Version: | unspecified | ||
Target Milestone: | --- | ||
Platform: | Ubuntu | ||
OS: | Linux | ||
Latest Commit: | Version Fixed In: | ||
Sentry Crash Report: | |||
Attachments: | New crash information added by DrKonqi |
Description
Jonathan Thomas
2010-12-09 15:54:29 UTC
*** Bug 265548 has been marked as a duplicate of this bug. *** Please check what Valgrind has to say. Just that a pointer is non-null doesn't mean that it points to memory containing sensible/initialized/not deleted data. I successfully ran a function of the pointee's right before the extend function was called s, so the widget being extended is indeed initialized right before being extended. void ApplicationDelegate::itemActivated(QModelIndex index) { if (index == m_oldIndex) { return; } if (isExtended(m_oldIndex)) { disconnect(m_extender, SIGNAL(infoButtonClicked(Application *)), this, SIGNAL(infoButtonClicked(Application *))); disconnect(m_extender, SIGNAL(installButtonClicked(Application *)), this, SIGNAL(installButtonClicked(Application *))); disconnect(m_extender, SIGNAL(removeButtonClicked(Application *)), this, SIGNAL(removeButtonClicked(Application *))); disconnect(m_extender, SIGNAL(cancelButtonClicked(Application *)), this, SIGNAL(cancelButtonClicked(Application *))); contractItem(m_oldIndex); m_extender->deleteLater(); m_extender = 0; } Application *app = static_cast<const ApplicationProxyModel*>(index.model())->applicationAt(index); QTreeView *view = static_cast<QTreeView*>(parent()); m_extender = new ApplicationExtender(view, app, m_appBackend); connect(m_extender, SIGNAL(infoButtonClicked(Application *)), this, SIGNAL(infoButtonClicked(Application *))); connect(m_extender, SIGNAL(installButtonClicked(Application *)), this, SIGNAL(installButtonClicked(Application *))); connect(m_extender, SIGNAL(removeButtonClicked(Application *)), this, SIGNAL(removeButtonClicked(Application *))); connect(m_extender, SIGNAL(cancelButtonClicked(Application *)), this, SIGNAL(cancelButtonClicked(Application *))); kDebug() << m_extender->sizeHint().height(); extendItem(m_extender, index); m_oldIndex = index; } In a situation where the crash occurs, the kDebug() in the above function gives a correct output of 33. This is also coincidentally the way in which the extender pointer is used internally in kextendableitemdelegate.cpp at line 313. Since the extender pointer is valid right before KExtendableItemDelegate::extendItem() is called, something inside the KExtendableItemDelegate must mess up the pointer. A clue; line 313 in kextendableitemdelegate.cpp is nested inside this if statement: if (isExtended(index)) {} The extender is most definitely not extended at this point, as this crash happens with a totally new view. Here's the valgrind log for posterity: ==2081== Invalid read of size 4 ==2081== at 0x45568E3: KExtendableItemDelegate::extenderRect(QWidget*, QStyleOptionViewItem const&, QModelIndex const&) const (kextendableitemdelegate.cpp:313) ==2081== by 0x4557BA1: KExtendableItemDelegate::paint(QPainter*, QStyleOptionViewItem const&, QModelIndex const&) const (kextendableitemdelegate.cpp:277) ==2081== by 0x806B9ED: ApplicationDelegate::paint(QPainter*, QStyleOptionViewItem const&, QModelIndex const&) const (ApplicationDelegate.cpp:90) ==2081== by 0x53B1C03: QTreeView::drawRow(QPainter*, QStyleOptionViewItem const&, QModelIndex const&) const (qtreeview.cpp:1678) ==2081== by 0x53B4A71: QTreeView::drawTree(QPainter*, QRegion const&) const (qtreeview.cpp:1441) ==2081== by 0x53B53FA: QTreeView::paintEvent(QPaintEvent*) (qtreeview.cpp:1274) ==2081== by 0x4E5068D: QWidget::event(QEvent*) (qwidget.cpp:8346) ==2081== by 0x5240FB2: QFrame::event(QEvent*) (qframe.cpp:557) ==2081== by 0x52CF861: QAbstractScrollArea::viewportEvent(QEvent*) (qabstractscrollarea.cpp:1043) ==2081== by 0x536E9C6: QAbstractItemView::viewportEvent(QEvent*) (qabstractitemview.cpp:1619) ==2081== by 0x53B6872: QTreeView::viewportEvent(QEvent*) (qtreeview.cpp:1256) ==2081== by 0x52D2154: QAbstractScrollAreaFilter::eventFilter(QObject*, QEvent*) (qabstractscrollarea_p.h:100) ==2081== Address 0x38 is not stack'd, malloc'd or (recently) free'd ==2081== Looking at the isExtended() function: bool KExtendableItemDelegate::isExtended(const QModelIndex &index) const { return d->extenders.value(index); } d->extenders is a QHash of indices and QWidget *'s. QHash docs state that if that the key passed to value() is not in the QHash, it will return a default-constructed value. If this default-constructed pointer happens to not be pointing at 0x0 (say, 0x38 maybe), then the return statement will return true even though 0x38 is not a valid pointer. It would be much safer instead to use QHash::contains(key), as it is guaranteed to return false if the key is not present in the QHash. I am running a kdelibs build with this patch to see if this fixes the bug. Hmm, it still happens when using contains(). The Qt docs also say that a default-constructed value for primitive types such as int and pointers default to 0. This does however mean that somehow the KExtendableItemDelegate's internals mapped an invalid QWidget pointer to the index instead of the QWidget pointer provided. I just had an idea. Check if the index is valid! Maybe there is an extender (or more) with invalid index in the QHash. I might have to add a check for valid index, probably with a kWarning(), to prevent such errors in the future. Generally check if all indices are sane, like they don't change while KExtendableItemDelegate is using them. *** Bug 271201 has been marked as a duplicate of this bug. *** My Muon-updater Work perfectly after some update 19/4-11. *** Bug 280941 has been marked as a duplicate of this bug. *** *** Bug 281286 has been marked as a duplicate of this bug. *** *** Bug 282176 has been marked as a duplicate of this bug. *** Created attachment 63942 [details]
New crash information added by DrKonqi
muon-installer (1.2.1 "Caustic Carrionite") on KDE Platform 4.7.1 (4.7.1) using Qt 4.7.4
- What I was doing when the application crashed:
I install any of application, then I find the other application and click on him, then muon center's crash.
I can repeat this error.
-- Backtrace (Reduced):
#7 0x00007fa013b73148 in KExtendableItemDelegate::extenderRect (this=0x27f51f0, extender=<optimized out>, option=<optimized out>, index=...) at ../../kdeui/itemviews/kextendableitemdelegate.cpp:313
#8 0x00007fa013b7412d in KExtendableItemDelegate::paint (this=0x27f51f0, painter=0x7fffd798d740, option=..., index=...) at ../../kdeui/itemviews/kextendableitemdelegate.cpp:277
#9 0x000000000042ad37 in ApplicationDelegate::paint (this=0x27f51f0, painter=0x7fffd798d740, option=..., index=...) at /build/buildd/muon-1.2.1/installer/ApplicationModel/ApplicationDelegate.cpp:90
#10 0x00007fa012f0baed in QTreeView::drawRow (this=0x1f75b20, painter=0x7fffd798d740, option=..., index=...) at itemviews/qtreeview.cpp:1678
#11 0x00007fa012f0e5b3 in QTreeView::drawTree (this=0x1f75b20, painter=0x7fffd798d740, region=<optimized out>) at itemviews/qtreeview.cpp:1441
Git commit fc923ff98a3d0aaf0fb57ca480ba022e2978c2ac by Andreas Hartmetz. Committed on 26/09/2011 at 21:14. Pushed by ahartmetz into branch 'KDE/4.7'. Don't use "method static" (ahem) variables for per-instance data. When I wrote that code I didn't know that static data in member functions is "global" - all instances use the same data. So if there is more than one KExtendableItemDelegate in an application, it makes Bad Things happen. At least it is binary compatible to remove the bogus variables. This is officially a brown paper bag bug. Hopefully fixes BUG: 259333 M +26 -21 kdeui/itemviews/kextendableitemdelegate.cpp http://commits.kde.org/kdelibs/fc923ff98a3d0aaf0fb57ca480ba022e2978c2ac Seems fixed now with the commit from the last comment, please reopen if necessary. |