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<int>&, QVector<int>&, int, int, QModelIndex const&, Qt::Orientation, bool) + 58
26  QSortFilterProxyModelPrivate::remove_source_items(QVector<int>&, QVector<int>&, QVector<int> 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<int>&, QVector<int>&, int, int, QModelIndex const&, Qt::Orientation, bool) + 58
32  QSortFilterProxyModelPrivate::remove_source_items(QVector<int>&, QVector<int>&, QVector<int> 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<int>&, QVector<int>&, int, int, QModelIndex const&, Qt::Orientation, bool) + 58
38  QSortFilterProxyModelPrivate::remove_source_items(QVector<int>&, QVector<int>&, QVector<int> 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<QPointer<QWidget> > 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<QWidget>&, 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<QEventLoop::ProcessEventsFlag>) + 1522
69  QEventLoop::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) + 77
70  QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) + 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<QEventLoop::ProcessEventsFlag>) + 1522
95  QEventLoop::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) + 77
96  QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) + 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

@@ -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() {

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);

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();
   }
 }
 

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);

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<qreal> SpeedSample::average() const
   return Sample<qreal>(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());

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: