From c37e5abeff6ffa5a436fc01ad2473282358b9e46 Mon Sep 17 00:00:00 2001 From: Ivan Sorokin Date: Fri, 7 Nov 2014 02:30:54 +0300 Subject: [PATCH] Fix torrent removal. Closes #2132 It was reported that qbittorrent crashes on Mac OS X when user deletes torrents from label view when label filter is active. The callstack of the crash is the following: 1 abort + 129 2 __assert_rtn + 321 3 QTorrentHandle::total_size() const + 124 4 TorrentModelItem::data(int, int) const + 307 5 TorrentModel::data(QModelIndex const&, int) const + 255 6 QSortFilterProxyModel::data(QModelIndex const&, int) const + 109 7 QSortFilterProxyModel::data(QModelIndex const&, int) const + 109 8 QSortFilterProxyModel::data(QModelIndex const&, int) const + 109 9 QItemDelegate::rect(QStyleOptionViewItem const&, QModelIndex const&, int) const + 75 10 QItemDelegate::sizeHint(QStyleOptionViewItem const&, QModelIndex const&) const + 172 11 TransferListDelegate::sizeHint(QStyleOptionViewItem const&, QModelIndex const&) const + 14 12 QTreeView::indexRowSizeHint(QModelIndex const&) const + 887 13 QTreeViewPrivate::layout(int, bool, bool) + 462 14 QTreeView::doItemsLayout() + 356 15 QTreeViewPrivate::updateScrollBars() + 109 16 QTreeView::scrollTo(QModelIndex const&, QAbstractItemView::ScrollHint) + 124 17 TransferListWidget::currentChanged(QModelIndex const&, QModelIndex const&) + 548 18 TransferListWidget::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) + 641 19 QMetaObject::activate(QObject*, QMetaObject const*, int, void**) + 2196 20 QItemSelectionModelPrivate::_q_rowsAboutToBeRemoved(QModelIndex const&, int, int) + 3729 21 QItemSelectionModel::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) + 398 22 QMetaObject::activate(QObject*, QMetaObject const*, int, void**) + 2196 23 QAbstractItemModel::rowsAboutToBeRemoved(QModelIndex const&, int, int) + 78 24 QAbstractItemModel::beginRemoveRows(QModelIndex const&, int, int) + 106 25 QSortFilterProxyModelPrivate::remove_proxy_interval(QVector&, QVector&, int, int, QModelIndex const&, Qt::Orientation, bool) + 58 26 QSortFilterProxyModelPrivate::remove_source_items(QVector&, QVector&, QVector const&, QModelIndex const&, Qt::Orientation, bool) + 265 27 QSortFilterProxyModelPrivate::source_items_about_to_be_removed(QModelIndex const&, int, int, Qt::Orientation) + 232 28 QMetaObject::activate(QObject*, QMetaObject const*, int, void**) + 2196 29 QAbstractItemModel::rowsAboutToBeRemoved(QModelIndex const&, int, int) + 78 30 QAbstractItemModel::beginRemoveRows(QModelIndex const&, int, int) + 106 31 QSortFilterProxyModelPrivate::remove_proxy_interval(QVector&, QVector&, int, int, QModelIndex const&, Qt::Orientation, bool) + 58 32 QSortFilterProxyModelPrivate::remove_source_items(QVector&, QVector&, QVector const&, QModelIndex const&, Qt::Orientation, bool) + 265 33 QSortFilterProxyModelPrivate::source_items_about_to_be_removed(QModelIndex const&, int, int, Qt::Orientation) + 232 34 QMetaObject::activate(QObject*, QMetaObject const*, int, void**) + 2196 35 QAbstractItemModel::rowsAboutToBeRemoved(QModelIndex const&, int, int) + 78 36 QAbstractItemModel::beginRemoveRows(QModelIndex const&, int, int) + 106 37 QSortFilterProxyModelPrivate::remove_proxy_interval(QVector&, QVector&, int, int, QModelIndex const&, Qt::Orientation, bool) + 58 38 QSortFilterProxyModelPrivate::remove_source_items(QVector&, QVector&, QVector const&, QModelIndex const&, Qt::Orientation, bool) + 265 39 QSortFilterProxyModelPrivate::source_items_about_to_be_removed(QModelIndex const&, int, int, Qt::Orientation) + 232 40 QMetaObject::activate(QObject*, QMetaObject const*, int, void**) + 2196 41 QAbstractItemModel::rowsAboutToBeRemoved(QModelIndex const&, int, int) + 78 42 QAbstractItemModel::beginRemoveRows(QModelIndex const&, int, int) + 106 43 TorrentModel::removeTorrent(QString const&) + 81 44 TorrentModel::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) + 345 45 QMetaObject::activate(QObject*, QMetaObject const*, int, void**) + 2196 46 QBtSession::deletedTorrent(QString const&) + 56 47 QBtSession::deleteTorrent(QString const&, bool) + 2855 48 TransferListWidget::deleteSelectedTorrents() + 292 49 TransferListWidget::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) + 230 50 QMetaObject::activate(QObject*, QMetaObject const*, int, void**) + 2196 51 QAction::activate(QAction::ActionEvent) + 227 52 QMenuPrivate::activateCausedStack(QList > const&, QAction*, QAction::ActionEvent, bool) + 77 53 QMenuPrivate::activateAction(QAction*, QAction::ActionEvent, bool) + 470 54 QWidget::event(QEvent*) + 687 55 QMenu::event(QEvent*) + 617 56 QApplicationPrivate::notify_helper(QObject*, QEvent*) + 194 57 QApplication::notify(QObject*, QEvent*) + 2716 58 SessionApplication::notify(QObject*, QEvent*) + 21 59 QCoreApplication::notifyInternal(QObject*, QEvent*) + 118 60 QApplicationPrivate::sendMouseEvent(QWidget*, QMouseEvent*, QWidget*, QWidget*, QWidget**, QPointer&, bool) + 448 61 qt_mac_handleMouseEvent(NSEvent*, QEvent::Type, Qt::MouseButton, QWidget*, bool) + 1300 62 -[NSWindow _reallySendEvent:] + 759 63 -[NSWindow sendEvent:] + 368 64 -[QCocoaPanel sendEvent:] + 113 65 -[NSApplication sendEvent:] + 2238 66 -[QNSApplication sendEvent:] + 97 67 -[NSApplication run] + 711 68 QEventDispatcherMac::processEvents(QFlags) + 1522 69 QEventLoop::processEvents(QFlags) + 77 70 QEventLoop::exec(QFlags) + 370 71 QMenu::exec(QPoint const&, QAction*) + 103 72 TransferListWidget::displayListMenu(QPoint const&) + 8741 73 TransferListWidget::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) + 622 74 QMetaObject::activate(QObject*, QMetaObject const*, int, void**) + 2196 75 QWidget::event(QEvent*) + 3082 76 QFrame::event(QEvent*) + 45 77 QAbstractScrollArea::viewportEvent(QEvent*) + 108 78 QAbstractItemView::viewportEvent(QEvent*) + 1390 79 QTreeView::viewportEvent(QEvent*) + 218 80 QAbstractScrollAreaFilter::eventFilter(QObject*, QEvent*) + 37 81 QCoreApplicationPrivate::sendThroughObjectEventFilters(QObject*, QEvent*) + 115 82 QApplicationPrivate::notify_helper(QObject*, QEvent*) + 178 83 QApplication::notify(QObject*, QEvent*) + 5742 84 SessionApplication::notify(QObject*, QEvent*) + 21 85 QCoreApplication::notifyInternal(QObject*, QEvent*) + 118 86 qt_sendSpontaneousEvent(QObject*, QEvent*) + 45 87 qt_mac_handleMouseEvent(NSEvent*, QEvent::Type, Qt::MouseButton, QWidget*, bool) + 1378 88 -[NSWindow _reallySendEvent:] + 5682 89 -[NSWindow sendEvent:] + 368 90 -[QCocoaWindow sendEvent:] + 113 91 -[NSApplication sendEvent:] + 2238 92 -[QNSApplication sendEvent:] + 97 93 -[NSApplication run] + 711 94 QEventDispatcherMac::processEvents(QFlags) + 1522 95 QEventLoop::processEvents(QFlags) + 77 96 QEventLoop::exec(QFlags) + 370 97 QCoreApplication::exec() + 199 98 main + 3415 99 start + 52 As we can see the user deleted some torrent (48). QBtSession deleted the torrent from libtorrent::session (47) and emitted a signal (46), about torrent deletion. In responce to the signal (43) the TorrentModel notifies (42) its views about a change. After a long chain of notifications (42-6) the view requested (5) a value of total_size from TorrentModel. QTorrentHandle is already invalid as the torrent was removed in (47). So we've got a crash in (3). The fix is relatively straightforward: do notify TorrentModel about removal not after, but before torrent is removed from libtorrent::session. This commit does the same thing to TorrentSpeedMonitor. This bug reveals a major flaw in a design: currently we have a several components all subscribed to the torrent removal signal. Signal is delivered to them in arbitrary order, but they access each other in the handlers of this signal. E.g. TorrentModel can access TorrentSpeedMonitor. This doesn't lead to a crash because TorrentSpeedMonitor returns MAX_ETA when ETA is queried for unknown torrent. --- src/qtlibtorrent/qbtsession.cpp | 2 -- src/qtlibtorrent/qbtsession.h | 1 - src/qtlibtorrent/torrentmodel.cpp | 19 ++++++------------- src/qtlibtorrent/torrentmodel.h | 1 - src/qtlibtorrent/torrentspeedmonitor.cpp | 7 +------ src/qtlibtorrent/torrentspeedmonitor.h | 1 - 6 files changed, 7 insertions(+), 24 deletions(-) diff --git a/src/qtlibtorrent/qbtsession.cpp b/src/qtlibtorrent/qbtsession.cpp index 86ac3fdaa..8572c65b6 100755 --- a/src/qtlibtorrent/qbtsession.cpp +++ b/src/qtlibtorrent/qbtsession.cpp @@ -821,8 +821,6 @@ void QBtSession::deleteTorrent(const QString &hash, bool delete_local_files) { else addConsoleMessage(tr("'%1' was removed from transfer list.", "'xxx.avi' was removed...").arg(fileName)); qDebug("Torrent deleted."); - emit deletedTorrent(hash); - qDebug("Deleted signal emitted."); } void QBtSession::pauseAllTorrents() { diff --git a/src/qtlibtorrent/qbtsession.h b/src/qtlibtorrent/qbtsession.h index f6a80535f..c1eec4cc3 100755 --- a/src/qtlibtorrent/qbtsession.h +++ b/src/qtlibtorrent/qbtsession.h @@ -272,7 +272,6 @@ private slots: signals: void addedTorrent(const QTorrentHandle& h); - void deletedTorrent(const QString &hash); void torrentAboutToBeRemoved(const QTorrentHandle &h); void pausedTorrent(const QTorrentHandle& h); void resumedTorrent(const QTorrentHandle& h); diff --git a/src/qtlibtorrent/torrentmodel.cpp b/src/qtlibtorrent/torrentmodel.cpp index eb819ee03..8111f1465 100644 --- a/src/qtlibtorrent/torrentmodel.cpp +++ b/src/qtlibtorrent/torrentmodel.cpp @@ -331,7 +331,6 @@ void TorrentModel::populate() { // Listen for torrent changes connect(QBtSession::instance(), SIGNAL(addedTorrent(QTorrentHandle)), SLOT(addTorrent(QTorrentHandle))); connect(QBtSession::instance(), SIGNAL(torrentAboutToBeRemoved(QTorrentHandle)), SLOT(handleTorrentAboutToBeRemoved(QTorrentHandle))); - connect(QBtSession::instance(), SIGNAL(deletedTorrent(QString)), SLOT(removeTorrent(QString))); connect(QBtSession::instance(), SIGNAL(finishedTorrent(QTorrentHandle)), SLOT(handleFinishedTorrent(QTorrentHandle))); connect(QBtSession::instance(), SIGNAL(metadataReceived(QTorrentHandle)), SLOT(handleTorrentUpdate(QTorrentHandle))); connect(QBtSession::instance(), SIGNAL(resumedTorrent(QTorrentHandle)), SLOT(handleTorrentUpdate(QTorrentHandle))); @@ -440,18 +439,6 @@ void TorrentModel::addTorrent(const QTorrentHandle &h) } } -void TorrentModel::removeTorrent(const QString &hash) -{ - const int row = torrentRow(hash); - qDebug() << Q_FUNC_INFO << hash << row; - if (row >= 0) { - beginRemoveTorrent(row); - delete m_torrents[row]; - m_torrents.removeAt(row); - endRemoveTorrent(); - } -} - void TorrentModel::beginInsertTorrent(int row) { beginInsertRows(QModelIndex(), row, row); @@ -579,8 +566,14 @@ QString TorrentModel::torrentHash(int row) const void TorrentModel::handleTorrentAboutToBeRemoved(const QTorrentHandle &h) { const int row = torrentRow(h.hash()); + qDebug() << Q_FUNC_INFO << row; if (row >= 0) { emit torrentAboutToBeRemoved(m_torrents.at(row)); + + beginRemoveTorrent(row); + delete m_torrents[row]; + m_torrents.removeAt(row); + endRemoveTorrent(); } } diff --git a/src/qtlibtorrent/torrentmodel.h b/src/qtlibtorrent/torrentmodel.h index e62fce743..50b2b20a0 100644 --- a/src/qtlibtorrent/torrentmodel.h +++ b/src/qtlibtorrent/torrentmodel.h @@ -104,7 +104,6 @@ signals: private slots: void addTorrent(const QTorrentHandle& h); - void removeTorrent(const QString &hash); void handleTorrentUpdate(const QTorrentHandle &h); void handleFinishedTorrent(const QTorrentHandle& h); void notifyTorrentChanged(int row); diff --git a/src/qtlibtorrent/torrentspeedmonitor.cpp b/src/qtlibtorrent/torrentspeedmonitor.cpp index 5cdb62d30..9b593ac8b 100644 --- a/src/qtlibtorrent/torrentspeedmonitor.cpp +++ b/src/qtlibtorrent/torrentspeedmonitor.cpp @@ -117,7 +117,7 @@ private: TorrentSpeedMonitor::TorrentSpeedMonitor(QBtSession* session) : m_session(session) { - connect(m_session, SIGNAL(deletedTorrent(QString)), SLOT(removeSamples(QString))); + connect(m_session, SIGNAL(torrentAboutToBeRemoved(QTorrentHandle)), SLOT(removeSamples(QTorrentHandle))); connect(m_session, SIGNAL(pausedTorrent(QTorrentHandle)), SLOT(removeSamples(QTorrentHandle))); connect(m_session, SIGNAL(statsReceived(libtorrent::stats_alert)), SLOT(statsReceived(libtorrent::stats_alert))); } @@ -143,11 +143,6 @@ Sample SpeedSample::average() const return Sample(m_sum) * (1. / m_speedSamples.size()); } -void TorrentSpeedMonitor::removeSamples(const QString &hash) -{ - m_samples.remove(hash); -} - void TorrentSpeedMonitor::removeSamples(const QTorrentHandle& h) { try { m_samples.remove(h.hash()); diff --git a/src/qtlibtorrent/torrentspeedmonitor.h b/src/qtlibtorrent/torrentspeedmonitor.h index 5e0ea1c6a..473a4bdd5 100644 --- a/src/qtlibtorrent/torrentspeedmonitor.h +++ b/src/qtlibtorrent/torrentspeedmonitor.h @@ -52,7 +52,6 @@ public: private slots: void statsReceived(const libtorrent::stats_alert& stats); - void removeSamples(const QString& hash); void removeSamples(const QTorrentHandle& h); private: