--===============1463379029==
Content-type: multipart/signed; boundary=nextPart1463871.d8K8y6AOG1;
 protocol="application/pgp-signature"; micalg=pgp-sha1
Content-transfer-encoding: 7bit

--nextPart1463871.d8K8y6AOG1
Content-Type: text/plain;
  charset="utf-8"
Content-Transfer-Encoding: quoted-printable
Content-Disposition: inline

On Wednesday 10 October 2007, Allen "The Elephant Who Never Forgets=20
Anything" Winter wrote:
> On Monday 26 February 2007 5:48:30 pm Ingo Kl=C3=B6cker wrote:
> > On Monday 26 February 2007 09:45, Andreas Gungl wrote:
> > > Am Sunday 25 February 2007 schrieb Ingo Kl=C3=B6cker:
> > > > 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 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 (=3D 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=3D135376 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=20
do not most distros use the enterprise branch by now?

Anyway, AFAIR later I came up with some issues with my patch (possible=20
introduction of other crashes under specific circumstances IIRC; I=20
think it had to do with moving of messages which changes the parent=20
member variable of the KMMsgInfo object but not the corresponding=20
member variable of the KMMessage object; or was it the other way=20
around?; anyway, there is definitely a great risk for inconsistencies,=20
but I have no idea what impact those inconsistencies might have) and=20
therefore proposed not to commit it. Unfortunately, this does not seem=20
to be documented in this thread and quickly glancing over other=20
slightly more recent threads I didn't find another thread dealing with=20
this.

If the patch is good enough for the enterprise branch then I guess it's=20
also good enough for 3.5 and trunk. But at least in trunk this must be=20
fixed correctly at some point.


Regards,
Ingo

--nextPart1463871.d8K8y6AOG1
Content-Type: application/pgp-signature; name=signature.asc 
Content-Description: This is a digitally signed message part.

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.4-svn0 (GNU/Linux)

iD8DBQBHDUc2GnR+RTDgudgRAlD6AKCdRLXkAhCsgfV6uiQEz0/3EksCwgCgyYAG
P84FN5Hki5tu9YSfqMVCpTk=
=gdSL
-----END PGP SIGNATURE-----

--nextPart1463871.d8K8y6AOG1--

--===============1463379029==
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

--===============1463379029==--