[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kmail-devel
Subject:    Re: Reason for crashes when filtering mails
From:       Ingo =?utf-8?q?Kl=C3=B6cker?= <kloecker () kde ! org>
Date:       2007-10-11 18:33:00
Message-ID: 200710112033.22739 () erwin ! ingo-kloecker ! de
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


On Thursday 11 October 2007, Allen Winter wrote:
> 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:
> > > >
[snip]
> > > >
> > > > 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.
> >
[snip]
>
> > 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".

This is just a wild guess, but this "No Subject" message might be caused 
by the problem with the invalid value of the parent member variable 
after the message move I mentioned above.


> > 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?

I'm pretty sure my patch was self-contained.


Regards,
Ingo

["signature.asc" (application/pgp-signature)]

_______________________________________________
KMail developers mailing list
KMail-devel@kde.org
https://mail.kde.org/mailman/listinfo/kmail-devel


[prev in list] [next in list] [prev in thread] [next in thread]