--===============0296321153==
Content-type: multipart/signed; boundary=nextPart1712926.hX3qu5uVYe;
 protocol="application/pgp-signature"; micalg=pgp-sha1
Content-transfer-encoding: 7bit

--nextPart1712926.hX3qu5uVYe
Content-Type: multipart/mixed;
  boundary="Boundary-01=_CP24Fcm1Fg0iXaT"
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

--Boundary-01=_CP24Fcm1Fg0iXaT
Content-Type: text/plain;
  charset="iso-8859-15"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline

On Monday 26 February 2007 09:45, Andreas Gungl wrote:
> Am Sunday 25 February 2007 schrieb Ingo Kl=F6cker:
> > On Sunday 25 February 2007 13:39, Andreas Gungl wrote:
> > > This makes me wonder if idx =3D=3D 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 =3D=3D 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 ) >=3D 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 !=3D -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=20
replace a KMMsgInfo instance by a KMMessage instance if the message is=20
loaded. Now the problem is that there's code that still holds a pointer=20
to the deleted KMMsgInfo instance and as soon as it is accessed we have=20
a crash. There are two things that must be done for KDE 4:

=2D Add a proxy class containing pointers to a KMMsgInfo _and_ a KMMessage=
=20
instance. If the message is not loaded all information is taken from=20
the KMMsgInfo object otherwise (as suitable) from the KMMessage object.=20
This way we prevent the problem of invalid KMMsgInfo* pointers. In=20
fact, KMMsgBase could probably become this proxy. Of course, KMMsgInfo=20
and KMMessage would no longer be derived from KMMsgBase.
(Hmm, it seems a message can be any of KMMsgBase, KMMsgInfo or=20
KMMessage. So it's a bit more complicated since the pointers to=20
KMMsgInfo and KMMessage could both be 0.)

=2D Use shared pointers to prevent the deletion of KMMessage objects that=20
some code still holds a pointer to, i.e. derive KMMessage from KShared=20
and use KMMessagePtr (=3D KSharedPtr<KMMessage>) everywhere.
(Hmm, in view of the above comment the same should probably be done for=20
KMMsgInfo.)

Now the latter is probably too much work for KDE 3.5. But I think the=20
former could be feasible.

A more dirty solution for KDE 3.5 (but most likely not less work) would=20
be to derive KMMsgBase from KShared and replace all KMMsgInfo* by=20
KMMsgInfoPtr and all KMMessage* by KMMessagePtr.

A really dirty (and probably quick) solution would be not to delete the=20
KMMsgInfo object when a corresponding KMMessage is created, but to=20
remember the pointer to the KMMsgInfo object in the KMMessage object=20
and then, when the KMMessage is destroyed, to reset the pointer in the=20
message list to the pointer to the KMMsgInfo object.
This would require changes to (at least):
=2D KMMessage (add member variable and getter/setter for KMMsgInfo*)
=2D KMMsgInfo* KMFolderIndex::setIndexEntry( int idx, KMMessage *msg )
    (which replaces a KMMessage by the corresponding KMMsgInfo)
=2D KMMessage* KMFolderMaildir::readMsg(int idx)
=2D KMMessage* KMFolderMbox::readMsg(int idx)
    (which replaces a KMMsgInfo by the corresponding KMMessage)
=2D void KMMsgList::set(unsigned int idx, KMMsgBase* aMsg)
    (which actually deletes the KMMsgInfo/KMMessage objects; FWIW it's=20
really shocking that the documentation of this method reads "[...] If=20
there is already a message at the given index this message is *not*  =20
deleted. [...]"; this is obviously a lie :-( )

The attached patch contains all those required changes. I didn't even=20
check whether this patch compiles, so please use it with care.

Regards,
Ingo

--Boundary-01=_CP24Fcm1Fg0iXaT
Content-Type: text/x-diff;
  charset="iso-8859-15";
  name="keep-kmmsginfo-when-loading-message-2007-02-26.diff"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: attachment;
	filename="keep-kmmsginfo-when-loading-message-2007-02-26.diff"

Index: kmmessage.h
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
=2D-- 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 =3D pos; };
=20
+  /** 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 =3D msgInfo; }
+
 private:
=20
   /** Initialization shared by the ctors. */
@@ -883,6 +888,7 @@
   KMMessage* mUnencryptedMsg;
   DwBodyPart* mLastUpdated;
   int mCursorPos;
+  KMMsgInfo* mMsgInfo; // used to remember the KMMsgInfo object this KMMes=
sage replaced in the KMMsgList
 };
=20
=20
Index: kmfoldermaildir.cpp
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
=2D-- kmfoldermaildir.cpp	(revision 637576)
+++ kmfoldermaildir.cpp	(working copy)
@@ -558,7 +558,8 @@
 KMMessage* KMFolderMaildir::readMsg(int idx)
 {
   KMMsgInfo* mi =3D (KMMsgInfo*)mMsgList[idx];
=2D  KMMessage *msg =3D new KMMessage(*mi); // note that mi is deleted by t=
he line below
+  KMMessage *msg =3D new KMMessage(*mi);
+  msg->setMsgInfo( mi ); // remember the KMMsgInfo object to that we can r=
estore it when the KMMessage object is no longer needed
   mMsgList.set(idx,&msg->toMsgBase()); // done now so that the serial numb=
er can be computed
   msg->setComplete( true );
   msg->fromDwString(getDwString(idx));
Index: kmmessage.cpp
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
=2D-- kmmessage.cpp	(revision 637576)
+++ kmmessage.cpp	(working copy)
@@ -152,6 +152,7 @@
   mUnencryptedMsg =3D 0;
   mLastUpdated =3D 0;
   mCursorPos =3D 0;
+  mMsgInfo =3D 0;
 }
=20
 void KMMessage::assign( const KMMessage& other )
Index: kmmsglist.cpp
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
=2D-- kmmsglist.cpp	(revision 637576)
+++ kmmsglist.cpp	(working copy)
@@ -97,8 +97,6 @@
   if (!at(idx) && aMsg) mCount++;
   else if (at(idx) && !aMsg) mCount--;
=20
=2D  delete at(idx);
=2D
   at(idx) =3D aMsg;
=20
   if (!aMsg || idx >=3D mHigh) rethinkHigh();
Index: kmfoldermbox.cpp
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
=2D-- kmfoldermbox.cpp	(revision 637576)
+++ kmfoldermbox.cpp	(working copy)
@@ -826,7 +826,8 @@
   assert(mi!=3D0 && !mi->isMessage());
   assert(mStream !=3D 0);
=20
=2D  KMMessage *msg =3D new KMMessage(*mi); // note that mi is deleted by t=
he line below
+  KMMessage *msg =3D new KMMessage(*mi);
+  msg->setMsgInfo( mi ); // remember the KMMsgInfo object to that we can r=
estore it when the KMMessage object is no longer needed
   mMsgList.set(idx,&msg->toMsgBase()); // done now so that the serial numb=
er can be computed
   msg->fromDwString(getDwString(idx));
   return msg;
Index: kmfolderindex.cpp
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
=2D-- kmfolderindex.cpp	(revision 637576)
+++ kmfolderindex.cpp	(working copy)
@@ -473,9 +473,10 @@
=20
 KMMsgInfo* KMFolderIndex::setIndexEntry( int idx, KMMessage *msg )
 {
=2D  KMMsgInfo *msgInfo =3D new KMMsgInfo( folder() );
+  KMMsgInfo *msgInfo =3D msg->msgInfo();
   *msgInfo =3D *msg;
   mMsgList.set( idx, msgInfo );
+  delete msg;
   return msgInfo;
 }
=20

--Boundary-01=_CP24Fcm1Fg0iXaT--

--nextPart1712926.hX3qu5uVYe
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2 (GNU/Linux)

iD8DBQBF42PMGnR+RTDgudgRAmpyAJ9xBrAMGcCiXBbBDEzCGA5ZLJIFSQCdFcPi
k82PiZFOQ+zAA0BEGF30Qls=
=C8fJ
-----END PGP SIGNATURE-----

--nextPart1712926.hX3qu5uVYe--


--===============0296321153==
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Content-Disposition: inline

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

--===============0296321153==--