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

List:       kmail-devel
Subject:    Re: Reason for crashes when filtering mails
From:       Allen Winter <winter () kde ! org>
Date:       2007-10-10 20:58:29
Message-ID: 200710101658.29854.winter () kde ! org
[Download RAW message or body]

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.

-Allen
_______________________________________________
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]