[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 =?iso-8859-15?q?Kl=F6cker?= <kloecker () kde ! org>
Date: 2007-02-26 22:48:30
Message-ID: 200702262348.44293 () erwin ! ingo-kloecker ! de
[Download RAW message or body]
[Attachment #2 (multipart/signed)]
[Attachment #4 (multipart/mixed)]
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.
Regards,
Ingo
["keep-kmmsginfo-when-loading-message-2007-02-26.diff" (text/x-diff)]
Index: kmmessage.h
===================================================================
--- kmmessage.h (revision 637576)
+++ kmmessage.h (working copy)
@@ -855,6 +855,11 @@
/** Set cursor position as offset from message start */
void setCursorPos(int pos) { mCursorPos = pos; };
+ /** Get the KMMsgInfo object that was set with setMsgInfo(). */
+ KMMsgInfo* msgInfo() { return mMsgInfo; }
+ /** Set the KMMsgInfo object corresponding to this message. */
+ void setMsgInfo( KMMsgInfo* msgInfo ) { mMsgInfo = msgInfo; }
+
private:
/** Initialization shared by the ctors. */
@@ -883,6 +888,7 @@
KMMessage* mUnencryptedMsg;
DwBodyPart* mLastUpdated;
int mCursorPos;
+ KMMsgInfo* mMsgInfo; // used to remember the KMMsgInfo object this KMMessage \
replaced in the KMMsgList };
Index: kmfoldermaildir.cpp
===================================================================
--- kmfoldermaildir.cpp (revision 637576)
+++ kmfoldermaildir.cpp (working copy)
@@ -558,7 +558,8 @@
KMMessage* KMFolderMaildir::readMsg(int idx)
{
KMMsgInfo* mi = (KMMsgInfo*)mMsgList[idx];
- KMMessage *msg = new KMMessage(*mi); // note that mi is deleted by the line below
+ KMMessage *msg = new KMMessage(*mi);
+ msg->setMsgInfo( mi ); // remember the KMMsgInfo object to that we can restore it \
when the KMMessage object is no longer needed mMsgList.set(idx,&msg->toMsgBase()); \
// done now so that the serial number can be computed msg->setComplete( true );
msg->fromDwString(getDwString(idx));
Index: kmmessage.cpp
===================================================================
--- kmmessage.cpp (revision 637576)
+++ kmmessage.cpp (working copy)
@@ -152,6 +152,7 @@
mUnencryptedMsg = 0;
mLastUpdated = 0;
mCursorPos = 0;
+ mMsgInfo = 0;
}
void KMMessage::assign( const KMMessage& other )
Index: kmmsglist.cpp
===================================================================
--- kmmsglist.cpp (revision 637576)
+++ kmmsglist.cpp (working copy)
@@ -97,8 +97,6 @@
if (!at(idx) && aMsg) mCount++;
else if (at(idx) && !aMsg) mCount--;
- delete at(idx);
-
at(idx) = aMsg;
if (!aMsg || idx >= mHigh) rethinkHigh();
Index: kmfoldermbox.cpp
===================================================================
--- kmfoldermbox.cpp (revision 637576)
+++ kmfoldermbox.cpp (working copy)
@@ -826,7 +826,8 @@
assert(mi!=0 && !mi->isMessage());
assert(mStream != 0);
- KMMessage *msg = new KMMessage(*mi); // note that mi is deleted by the line below
+ KMMessage *msg = new KMMessage(*mi);
+ msg->setMsgInfo( mi ); // remember the KMMsgInfo object to that we can restore it \
when the KMMessage object is no longer needed mMsgList.set(idx,&msg->toMsgBase()); \
// done now so that the serial number can be computed \
msg->fromDwString(getDwString(idx)); return msg;
Index: kmfolderindex.cpp
===================================================================
--- kmfolderindex.cpp (revision 637576)
+++ kmfolderindex.cpp (working copy)
@@ -473,9 +473,10 @@
KMMsgInfo* KMFolderIndex::setIndexEntry( int idx, KMMessage *msg )
{
- KMMsgInfo *msgInfo = new KMMsgInfo( folder() );
+ KMMsgInfo *msgInfo = msg->msgInfo();
*msgInfo = *msg;
mMsgList.set( idx, msgInfo );
+ delete msg;
return msgInfo;
}
[Attachment #8 (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]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic