[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-10 21:42:14
Message-ID: 200710102342.14764 () erwin ! ingo-kloecker ! de
[Download RAW message or body]

[Attachment #2 (multipart/signed)]


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<KMMessage>) 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? And wasn't this the last 3.5 release? And 
do not most distros use the enterprise branch by now?

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.

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.


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]