[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 22:20:54
Message-ID: 200710101820.54389.winter () kde ! org
[Download RAW message or body]

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

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