Bug 327865

Summary:	Endless fetch loop with favorite collections
Product:	[Applications] kdepimlibs	Reporter:	Christian Mollekopf <mollekopf>
Component:	general	Assignee:	kdepim bugs <kdepim-bugs>
Status:	RESOLVED FIXED
Severity:	grave	CC:	dvratil, rdieter
Priority:	NOR
Version:	4.11.3
Target Milestone:	---
Platform:	unspecified
OS:	Linux
Latest Commit:		Version Fixed In:	4.13 b335be0
Sentry Crash Report:

Description Christian Mollekopf 2013-11-20 14:24:37 UTC

The fixing of the buffering in the ETM exposed a new bug, that result in an endless fetch loop if you have more than 10 favorite collections.

The basic loop is:
* dataChanged get's triggered for all favorite collections
* this results in rootIndexAboutToBeRemoved in KSelectionProxyModel respectively Akonadi::SelectionProxyModel
* Akonadi::SelectionProxyModel dereferences the collection
* since this is done for all collections, the currently selected one falls out of the buffer again => it get's purged
* The selection proxy references the collection again after the dataChange is done
* items are fetched again
* dataChanged is emitted again
=> endless loop

Note that the endless loop would happen in any case with the selection proxy, but if there are less than 10 folders in there, the buffering keeps the currently selected folder from being purged.

I'm not sure yet how to fix this problem, but I'm working on it

Reproducible: Always

Comment 1 Christian Mollekopf 2013-11-20 16:27:48 UTC

Two problems is probably that an itemFetch shouldn't trigger dataChanged for the parent collection (we have rowsAboutToBeInserted signals for that).

There may be more problems though. However, a patch that avoids those unnecessary dataChanged signals seems to fix the endless loop.

Comment 2 Daniel Vrátil 2013-11-20 16:55:50 UTC

Could be fixed by this [0] patch, not sure whether it is still valid after Christian's changes though (or whether it was ever valid at all ;-)

[0] https://git.reviewboard.kde.org/r/112958/

Comment 3 Christian Mollekopf 2013-11-21 09:54:20 UTC

(In reply to comment #2)
> Could be fixed by this [0] patch, not sure whether it is still valid after
> Christian's changes though (or whether it was ever valid at all ;-)
> 
> [0] https://git.reviewboard.kde.org/r/112958/

I'm not quite sure if this should be required. But there are a couple of different problems to tackle anyways:
* The first issue is that we get a dataChanged signal for the collection when fetching items, while we should only get rowsInserted.
For this I have a patch ready here:
https://git.reviewboard.kde.org/r/113992/

That already fixes the endless loop.

* The second problem is how the selection proxy reacts to legitimate dataChanged signals. It looks like every dataChanged signal explodes into each selection proxy dereferencing and rereferencing all favorite collections. This of course clears the buffer if you have a couple of favorites.

Comment 4 Christian Mollekopf 2013-11-21 09:57:33 UTC

This is what we get when selecting a new collection without any dataChanged signals:
kmail2(25459)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 659
kmail2(25459)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 677
kmail2(25459)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 677
kmail2(25459)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 677
kmail2(25459)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 677
kmail2(25459)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 677

As you can see there seems to be a bit uncessary back and forth going on, since the refcount always stays above 0 it's not a big problem though.

Comment 5 Christian Mollekopf 2013-11-21 09:59:54 UTC

And this is what happens with dataChanged signals:

kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::monitoredItemChanged: QModelIndex(15,0,0x2e045d0,Akonadi::EntityTreeModel(0x1ff7b70) )
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::dataChanged: QModelIndex(15,0,0x2e045d0,Akonadi::EntityTreeModel(0x1ff7b70) )  QModelIndex(15,0,0x2e045d0,Akonadi::EntityTreeModel(0x1ff7b70) )
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::monitoredItemChanged: QModelIndex(15,0,0x2e045d0,Akonadi::EntityTreeModel(0x1ff7b70) )
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::dataChanged: QModelIndex(15,0,0x2e045d0,Akonadi::EntityTreeModel(0x1ff7b70) )  QModelIndex(15,0,0x2e045d0,Akonadi::EntityTreeModel(0x1ff7b70) )
kmail2(27586) KMKernel::slotProgressItemCompletedOrCanceled: Last resource finished syncing, mail check done
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::monitoredCollectionStatisticsChanged: QModelIndex(12,0,0x2cd2330,Akonadi::EntityTreeModel(0x1ff7b70) )
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::dataChanged: QModelIndex(12,0,0x2cd2330,Akonadi::EntityTreeModel(0x1ff7b70) )  QModelIndex(12,0,0x2cd2330,Akonadi::EntityTreeModel(0x1ff7b70) )
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 659
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 659  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 677
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 677  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 678
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 678  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 685
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 684
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 684  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 665
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 665  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 660
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 660  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 680
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 680  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 683
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 683  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 679
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 679  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 682
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 682  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 659
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 677
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 678
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 685
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 684
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 665
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 660
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 680
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 683
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 679
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 682
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 659
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 659  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 677
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 677  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 678
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 678  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 685
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 684
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 684  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 665
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 665  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 660
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 660  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 680
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 680  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 683
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 683  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 679
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 679  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 682
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 682  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 659
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 677
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 678
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 685
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 684
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 665
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 660
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 680
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 683
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 679
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 682
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 659
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 659  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 677
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 677  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 678
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 678  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 685
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 684
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 684  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 665
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 665  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 660
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 660  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 680
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 680  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 683
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 683  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 679
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 679  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 682
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::deref: 682  moved to buffer
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 659
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 677
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 678
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 685
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 684
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 665
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 660
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 680
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 683
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 679
kmail2(27586)/libakonadi Akonadi::EntityTreeModelPrivate::ref: 682

Comment 6 Christian Mollekopf 2013-11-21 14:58:15 UTC

After some more research it looks like it's not the KSelectionProxyModel that breaks the refcounting. If all favorite folders are removed the referencing works as expected, with always the previously selected collection getting dereferenced, and the newly selected collection getting reference. In this case a dataChanged signal also doesn't cause any selection changes.

However, as soon as favorite folders are added to the mix, we get the reference/dereference cycles as above and a dataChanged signal results in all favorite folders getting wildly referenced and dereferenced as shown above. Since it's neither the favoriteCollectionModel nore the SelectionProxyModel itself, I suspect the responsible code is somewhere in kmail.

Comment 7 Christian Mollekopf 2013-12-02 09:12:57 UTC

The endless fetch loop has been fixed in:
95784bf [4.11]
00fd234 [4.11]

The FavoriteCollectionsModel still breaks the refcounting and needs to be fixed (and tested). Status quo is that the refcounting works without any favorites, but doesn't as soon as favorites are used.

Comment 8 Christian Mollekopf 2013-12-18 13:46:07 UTC

A layoutChanged signal comes from the modelstack in kmail foldertreewidget:

EntityCollectionOrderProxyModel <- expands a dataChanged signal to layoutChanged
FolderTreeWidgetProxyModel
StatisticsProxyModel
QuotaColorProxyModel
ETM

This results in all collections getting deselected and getting reselected again.
Obviously we should avoid the layoutChanged, but it's also not clear why the refcount falls to 0. The Favorite model stack should keep it's refcount....

Comment 9 Christian Mollekopf 2013-12-18 17:21:46 UTC

The reason why everything gets dereferenced to zero, is because the FavortiteCollectionsModel sits on top of the EntityCollectionOrderProxyModel just like the other SelectionProxyModels that are part of StorageModel.

So unless we have a workaround (like delaying the decrease of the refcount), the layoutChanged signal is our real problem.

The layoutChanged signal get's emitted because the QSortFilterProxyModel baseclass of EntityCollectionOrderProxyModel detects that some indexes need to be resorted in it's sourceDataChanged slot, and that results in a layoutChanged signal. The dataChanged signal causing it in this case stems from Akonadi::EntityTreeModelPrivate::changeFetchState.
However, the underlying models shouldn't change the order just because a new folder was selected in the view.

Comment 10 Christian Mollekopf 2013-12-19 15:27:34 UTC

I'm tracking the layoutChanged signal problem here: https://bugs.kde.org/show_bug.cgi?id=329004

It's in fact an optimization to not issue layoutChanged IMO, meaning we just can't rely on the selection for the referencing. I'm investigating if I have to rewrite FavortieCollectionModel entirely, or if I can adapt it reasonably to not rely on the selection for the referencing.

Comment 11 Christian Mollekopf 2014-03-18 09:52:54 UTC

This has been fixed for 4.13 in kdepimlibs [b335be0]