[Kde-pim] Review Request 114341: Fix a bug in kmail that eats all cpu

[Christian Mollekopf]

> On Dec. 7, 2013, 10:30 p.m., Milian Wolff wrote:
> > mailcommon/kernel/mailkernel.cpp, line 206
> > <http://git.reviewboard.kde.org/r/114341/diff/1/?file=223197#file223197line206>
> >
> >     this should to wherever the connect happens, i.e. before you connect, first disconnect. This is much simpler to understand then code-wise.
> 
> Raul Fernandes wrote:
>     Actually, the right way is to make sure that connect() is not called more than one time.
>     There's no point in disconnect one signal to reconnect just after.
>     IMHO there should be 2 functions, one initFolders() called by kmail and another updateFolders() called by slotDefaultCollectionsChanged().
>     This way, disconnect is not used and the code becomes simplier.
>     What do you think??
> 
> Milian Wolff wrote:
>     Sounds good.

I think Qt::UniqueConnecton is just fine (or calling disconnect right before connect, that's just a style issue). The actual problem seem to be that there is a loop, since slotDefaultCollectonsChanged triggers a fetch job that triggers the slot again. This is now avoided by simply disconnecting the signal, but that of course means we no longer get updates for changed defaultCollections (and I think we want that).

Thiago posted a nice analysis in 323068:
1) when KMail starts, I suppose MailCommon::Kernel::initFolders is called to initialise the folder listing
2) MailCommon::Kernel::initFolders creates a Akonadi::SpecialMailCollectionsDiscoveryJob job (derived from Akonadi::SpecialCollectionsDiscoveryJob)
3) when Akonadi::SpecialCollectionsDiscoveryJob finishes, Akonadi::SpecialCollectionsDiscoveryJob::slotResult is called
4) Akonadi::SpecialCollectionsDiscoveryJob::slotResult iterates over the result (201 folders). For two folders, it calls:
68                  d->mSpecialCollections->registerCollection(collection.attribute<SpecialCollectionAttribute>()->collectionType(), collection);
  The two folders have mRemoteId = "outbox" and mRemoteId = "caixa de saída" (don't ask me why one is localised and the other isn't).
5) because of those calls, Akonadi::SpecialCollectionsPrivate::emitChanged gets called
6) since those two folders have resourceId == mDefaultResourceId, the defaultCollectionsChanged() signal gets called, which ends up calling MailCommon::Kernel::initFolders
7) repeat ad nauseam

So slotDefaultCollectonsChanged should just be always connected (avoiding calling connect everytime would be nice but doesn't really matter), we now need to figure out why we end-up in an endless loop (SpecialCollectionsDiscoveryJob should probably not emit it's changed signal every time it finds two collections, and slotDefaultCollectonsChanged() shouldn't issue another SpecialMailCollectionsRequestJob I suppose).


- Christian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/114341/#review45333
-----------------------------------------------------------


On Dec. 9, 2013, 6:03 p.m., Raul Fernandes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/114341/
> -----------------------------------------------------------
> 
> (Updated Dec. 9, 2013, 6:03 p.m.)
> 
> 
> Review request for KDEPIM.
> 
> 
> Bugs: 323929
>     http://bugs.kde.org/show_bug.cgi?id=323929
> 
> 
> Repository: kdepim
> 
> 
> Description
> -------
> 
> The bug is described in https://bugs.kde.org/show_bug.cgi?id=323929.
> The bug triggers several queries to akonadi to get the same information.
> I think it is caused by the repeated connect() calls in initFolders() function (even using the Qt::UniqueConnection).
> So everytime the function slotDefaultCollectionsChanged() is called, disconnect the signal to avoid accumulate multiple signals.
> I think the right way is to rewrite the initFolders to call connect() only once.
> Split initFolders() in 2 functions. One is called by kmail and another (with the connect()) called by slotDefaultCollectionsChanged().
> For now, the disconnect() should be a good workaround for the (critical) bug.
> 
> 
> Diffs
> -----
> 
>   mailcommon/kernel/mailkernel.cpp 5b92778 
> 
> Diff: http://git.reviewboard.kde.org/r/114341/diff/
> 
> 
> Testing
> -------
> 
> Until now, it is ok here.
> 
> 
> File Attachments
> ----------------
> 
> kmail.log.lrz
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/12/07/1fe839f7-aea6-462a-8310-57be1afbb14f__kmail.log.lrz
> Recompressed log file
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/12/08/2502547f-e827-40ae-9fe5-dd1bcaad3168__log.tar.xz
> Log sorted by occurrences
>   http://git.reviewboard.kde.org/media/uploaded/files/2013/12/08/68881267-6582-4dbb-a02b-0ffcc49f0e3c__kmail-sorted.log.gz
> 
> 
> Thanks,
> 
> Raul Fernandes
> 
>

_______________________________________________
KDE PIM mailing list kde-pim at kde.org
https://mail.kde.org/mailman/listinfo/kde-pim
KDE PIM home page at http://pim.kde.org/