From kmail-devel Wed Oct 10 22:20:54 2007 From: Allen Winter Date: Wed, 10 Oct 2007 22:20:54 +0000 To: kmail-devel Subject: Re: Reason for crashes when filtering mails Message-Id: <200710101820.54389.winter () kde ! org> X-MARC-Message: https://marc.info/?l=kmail-devel&m=119205491411285 On Wednesday 10 October 2007 5:42:14 pm Ingo Klöcker wrote: > On Wednesday 10 October 2007, Allen "The Elephant Who Never Forgets > Anything" Winter wrote: > > On Monday 26 February 2007 5:48:30 pm Ingo Klöcker wrote: > > > On Monday 26 February 2007 09:45, Andreas Gungl wrote: > > > > Am Sunday 25 February 2007 schrieb Ingo Klöcker: > > > > > On Sunday 25 February 2007 13:39, Andreas Gungl wrote: > > > > > > This makes me wonder if idx == 0 is a valid index and how the > > > > > > assert is meant to work. Of course, the real problem is > > > > > > mStorage being not defined, but that's a bit beyond my > > > > > > knowledge of the folder stuff in KMail. > > > > > > > > > > 0 is definitely a valid index, so KMFolder::find() should > > > > > return -1 in case of mStorage == NULL. > > > > > > > > Fine. I was fooled by the current code in the 3.5 branch. I've > > > > seen Allen fixed that in kpim+, but such changes should be > > > > applied to the 3.5 branch immediately. > > > > > > > > > msgBase->parent()->find( msgBase ) >= 0 always has to be true > > > > > because obviously msgBase must be in its parent (i.e. its > > > > > folder) and thus it must be found in the folder. mStorage being > > > > > NULL is of course a serious problem and I have no idea how that > > > > > could ever happen because mStorage is set in the c'tor of > > > > > KMFolder and it's never set to 0 anywhere. > > > > > > > > > > I'm sorry, but I don't see what could possibly go wrong. To > > > > > prevent the crash I suggest adding a check for idx != -1 after > > > > > the assert and disabling the assert in the release build. > > > > > > > > > > It would be great if you could debug this a bit more in order > > > > > to find out why mStorage is NULL. > > > > > > > > The problem is in the access of msgBase->parent(). I've slightly > > > > modified the code from the 3.5 branch. I've added the changes > > > > made by Allen plus some additional code to get an idea what's > > > > going wrong. See the attached diff. > > > > > > > > Here's what valgrind gave me when triggering the crash: > > > > > > [snip] > > > > > > > Somehow it seems to be related to the message being selected > > > > before I use Ctrl-A Ctrl-J, but I'm not really sure about that. > > > > > > Yes, this explains the problem. It is due to the not-so-clever idea > > > to replace a KMMsgInfo instance by a KMMessage instance if the > > > message is loaded. Now the problem is that there's code that still > > > holds a pointer to the deleted KMMsgInfo instance and as soon as it > > > is accessed we have a crash. There are two things that must be done > > > for KDE 4: > > > > > > - Add a proxy class containing pointers to a KMMsgInfo _and_ a > > > KMMessage instance. If the message is not loaded all information is > > > taken from the KMMsgInfo object otherwise (as suitable) from the > > > KMMessage object. This way we prevent the problem of invalid > > > KMMsgInfo* pointers. In fact, KMMsgBase could probably become this > > > proxy. Of course, KMMsgInfo and KMMessage would no longer be > > > derived from KMMsgBase. > > > (Hmm, it seems a message can be any of KMMsgBase, KMMsgInfo or > > > KMMessage. So it's a bit more complicated since the pointers to > > > KMMsgInfo and KMMessage could both be 0.) > > > > > > - Use shared pointers to prevent the deletion of KMMessage objects > > > that some code still holds a pointer to, i.e. derive KMMessage from > > > KShared and use KMMessagePtr (= KSharedPtr) everywhere. > > > (Hmm, in view of the above comment the same should probably be done > > > for KMMsgInfo.) > > > > > > Now the latter is probably too much work for KDE 3.5. But I think > > > the former could be feasible. > > > > > > A more dirty solution for KDE 3.5 (but most likely not less work) > > > would be to derive KMMsgBase from KShared and replace all > > > KMMsgInfo* by KMMsgInfoPtr and all KMMessage* by KMMessagePtr. > > > > > > A really dirty (and probably quick) solution would be not to delete > > > the KMMsgInfo object when a corresponding KMMessage is created, but > > > to remember the pointer to the KMMsgInfo object in the KMMessage > > > object and then, when the KMMessage is destroyed, to reset the > > > pointer in the message list to the pointer to the KMMsgInfo object. > > > This would require changes to (at least): > > > - KMMessage (add member variable and getter/setter for KMMsgInfo*) > > > - KMMsgInfo* KMFolderIndex::setIndexEntry( int idx, KMMessage *msg > > > ) (which replaces a KMMessage by the corresponding KMMsgInfo) - > > > KMMessage* KMFolderMaildir::readMsg(int idx) > > > - KMMessage* KMFolderMbox::readMsg(int idx) > > > (which replaces a KMMsgInfo by the corresponding KMMessage) > > > - void KMMsgList::set(unsigned int idx, KMMsgBase* aMsg) > > > (which actually deletes the KMMsgInfo/KMMessage objects; FWIW > > > it's really shocking that the documentation of this method reads > > > "[...] If there is already a message at the given index this > > > message is *not* deleted. [...]"; this is obviously a lie :-( ) > > > > > > The attached patch contains all those required changes. I didn't > > > even check whether this patch compiles, so please use it with care. > > > > Since this patch has been committed to the enterprise branch > > and seems to work there, I will commit it also to the 3.5 branch and > > trunk. > > > > Bug http://bugs.kde.org/show_bug.cgi?id=135376 has 241 votes > > and really needs some solution. Even if it is a dirty hack. > > Isn't it too late for 3.5.8? Yes > And wasn't this the last 3.5 release? No, not to my knowledge. > And do not most distros use the enterprise branch by now? Many do, yes. But not all. > > Anyway, AFAIR later I came up with some issues with my patch (possible > introduction of other crashes under specific circumstances IIRC; I > think it had to do with moving of messages which changes the parent > member variable of the KMMsgInfo object but not the corresponding > member variable of the KMMessage object; or was it the other way > around?; anyway, there is definitely a great risk for inconsistencies, > but I have no idea what impact those inconsistencies might have) and > therefore proposed not to commit it. Unfortunately, this does not seem > to be documented in this thread and quickly glancing over other > slightly more recent threads I didn't find another thread dealing with > this. > Umm.. testing this locally... ctrl-a ctrl-j moves all messages into my trash folder. But I don't think any of my filters have that action.. strange. And, 1 message is left behind with a "No Subject". > If the patch is good enough for the enterprise branch then I guess it's > also good enough for 3.5 and trunk. But at least in trunk this must be > fixed correctly at some point. > Yes, it should be fixed correctly. Doesn't look like it works by itself in the 3.5 branch. Maybe I'm missing part of the patch? Anyway, the more I try to fix and debug the more crashes I encounter and the more icky things become. Bleech. -Allen _______________________________________________ KMail developers mailing list KMail-devel@kde.org https://mail.kde.org/mailman/listinfo/kmail-devel