From kde-core-devel Mon Dec 05 16:50:46 2005 From: Michael Brade Date: Mon, 05 Dec 2005 16:50:46 +0000 To: kde-core-devel Subject: Re: branches/KDE/3.5/kdelibs/kio/kio Message-Id: <200512051345.48550.brade () kde ! org> X-MARC-Message: https://marc.info/?l=kde-core-devel&m=113380144632019 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--nextPart3848116.pPfKAy3fJ3" --nextPart3848116.pPfKAy3fJ3 Content-Type: multipart/mixed; boundary="Boundary-01=_MzHlDv31v+jOF3z" Content-Transfer-Encoding: 7bit Content-Disposition: inline --Boundary-01=_MzHlDv31v+jOF3z Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline On Thursday 01 December 2005 20:12, Aaron J. Seigo wrote: > UNBELIEVABLE. > > i discussed this with brade, alright. but it was NOT to revert this chang= e. > this change should not in practice affect anyone IMHO (since the problem > was ONLY trigged in situations where something went wrong anyways and this > simply protected as well as possible against it). the times when this > affected anyone would be in situations such as kicker. Well gee ey? What's going on with you, you're writing this almost two weeks after I reverted, which was still *before* our discussion, we have exchange= d=20 about 6 emails each since then, discussing the real cause and the proper fi= x!=20 Now pretending you didn't get my (pseudo-)patch I sent with my second last= =20 mail? And ranting exactly at the time I left for Venezuela, right after my= =20 last diploma examination, about two days before I actually wanted to start= =20 debugging KLibLoader, which I did tell you more than a week ago already? I don't understand you anymore, honestly. Luckily I've got an internet=20 connection now so that I can reply at all. > and NO, kicker can't fix this because it is out of kicker's control. it's > the confluence of various kdelibs classes each individually doing The Rig= ht > Thing for themselves, but not working together (static object deletion > based cleanup on app exit). in this case, particularly klibloader and > kdirlister. we don't actually know for sure yet. > and NOW we hae a 3.5.0 where kicker crashes on logout EVERY TIME in the > DEFAULT CONFIGURATION. this is beyond stupid. Mind you, it was not THIS commit that did that, this is KDE_3_5_BRANCH and = we=20 still have more than enough time to fix it. Properly. Two things: - this is my code (sadly there are not many people yet who understand the big picture of KDirListerCache) and you did not tell me anything about your problem before committing the flawed fix, you did not even send it= to core-devel, and you committed it during the freeze, so I don't think it= 's ok for you to complain *NOW* that Kicker crashes and that I don't want a really bad hack in my code that *I* need to debug later on. - your fix would have made it impossibly difficult to debug the crashes t= hat are still left in KDirListerCache (probably about 1-2 of them) by violating one of KDirListerCache's core invariants. All future KDE 3.5.0 reports would be a lot closer to "useless" with your patch in. > i spent a fair amount of time working this through and had a long > conversation with michael about this via private email. and now, to cover= a > theoretical issue, we have a very real practical issue. Ok, to make this clear for the public: Aaron spend this time *before* our=20 discussion and I told him why his patch was completely wrong. I reverted th= is=20 before our main discussion as well, after telling him that the fix won't be= =20 in KDE 3.5.0 (suggested by Coolo), so it's not like I said something to Aar= on=20 and did it the other way around behind his back. I even sent a patch that would change KDirListerCache *properly*. Here's th= e=20 simple explanation: something (maybe KLibLoader) deletes KDirListerCache=20 before deleting the KDirListers connected to it. Then when deleting the=20 KDirListers they try to deregister form from KDirListerCache, failing while= =20 causing a new static KDirListerCache object to be created that can't do muc= h=20 but crash sooner or later.=20 The obvious patch is to check in the KDirLister DTOR if the cache is still= =20 there, no need to spend days just to hack one of the core methods in=20 KDirListerCache with the purpose to delay the crash long enough so that=20 Kicker can quit before it. It took me about 2 minutes to find the problem a= nd=20 the patch---after updating, freeing space on my hd and recompiling my KDE=20 (-libs) with debugging symbols, which took a lot longer. I'll attach the 2 last mail I sent to Aaron for public reference. They=20 included the proper patch. > i'm not going to simply re-commit, because i think that kind of commit > fighting is not useful. i'd like to request permission to recommit my > changes so that 3.5.1 might not suck as much in such an obviously stupid > way. Absolutely no. I sent you the proper patch for KDirListerCache already and = I=20 told you why I didn't commit it yet (see attachment). I'll commit it soon.= =20 Soon enough for KDE 3.5.1, but not before I have debugged the problem to=20 maybe find a fix for the real cause, a fix for KLibLoader, or Kicker, or=20 whatever. The least understandable part about your reaction is that you actually want= to=20 put your hack back in KDirListerCache right now, instead of forcing me to p= ut=20 my new patch in as soon as possible. The reasoning behind the latter I woul= d=20 at least have been able to follow. Please let's get back to sanity. Cheers, =2D-=20 Michael Brade; KDE Developer, Student of Computer Science |-mail: echo brade !#|tr -d "c oh"|s\e\d 's/e/\@/2;s/$/.org/;s/bra/k/2' =B0--web: http://www.kde.org/people/michaelb.html KDE 3: The Next Generation in Desktop Experience --Boundary-01=_MzHlDv31v+jOF3z Content-Type: message/rfc822; name="forwarded message" Content-Transfer-Encoding: 7bit Content-Description: Michael Brade : Re: [Bug 113242] the logout crash Content-Disposition: inline From: Michael Brade Organization: K Desktop Environment To: "Aaron J. Seigo" Subject: Re: [Bug 113242] the logout crash Date: Thu, 24 Nov 2005 11:36:59 +0100 User-Agent: KMail/1.9 References: <20051120104659.14024.qmail@ktown.kde.org> <200511221958.03220.brade@kde.org> <200511231540.27533.aseigo@kde.org> In-Reply-To: <200511231540.27533.aseigo@kde.org> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart4006872.BHdeXmtyEQ"; protocol="application/pgp-signature"; micalg=pgp-sha1 Content-Transfer-Encoding: 7bit Message-Id: <200511241137.04217.brade@kde.org> --nextPart4006872.BHdeXmtyEQ Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline On Wednesday 23 November 2005 23:40, you wrote: > no, i'm not. if you rely on automagic static object deletion you can't > assume that static object A gets deleted before or after any other static > object B. so you want to say that KLibLoader is static then? And that the deletion or= der=20 of KLibLoader and KDirListerCache is not defined? I'm still not sure, becau= se=20 in my code there is exactly _one_ static object only and I'm not sure which= =20 order you think would work (even if this order isn't guaranteed). > > I have a static server object holding file items > > and several non-static clients relaying the file items to the applicati= on > > and you say the whole thing should work properly when randomly exchangi= ng > > the server, deleting the file items behind all KDirListers' backs?!? > > no, that's not at all what i'm saying. look at the bug, what caused it, a= nd > then consider order of deletion issues. well...? honestly, I don't understand what you want to get at? Maybe you mi= ss=20 the possibility that right at the time of application-quit there could stil= l=20 the user be clicking on something triggering a KDirLister action, which in= =20 turn will access (the just deleted) KDirListerCache? > > > > Now you're probably thinking "what the hell does this mean". > > > > > > actually, i know exacxtly what that means, as it's exactly what i > > > suggested the problem was in my last email. > > > > hm? you never told me that the cache was deleted while the rest was sti= ll > > alive. Anyway, that's not the point anymore. > > i assumed that was obvious given the patch i committed? Then you have a serious problem with coding, eh? =3D;-)) To me, something like this would have been obvious and I was thinking about= it=20 two days ago: KDirLister::~KDirLister() { kdDebug(7003) << "-KDirLister" << endl; + if ( KDirListerCache::s_pSelf ) + { // Stop all running jobs stop(); s_pCache->forgetDirs( this ); + } delete d; } Which actually is an optimization that I will put in regardless of the Kick= er=20 crash because I realized that you can create a KDirLister and then delete i= t=20 again without doing anything else and only at the point of deletion the=20 KDirListerCache is created, unnecessarily. It will most probably fix the Kicker crash as well in your case but this is= =20 not the proper fix IMHO and I fear that when I put this in with the words "= it=20 fixes the Kicker crash" then nobody will care about KLibLoader anymore :-/ > if one of those items loaded via klibloader uses kdirlister, and if that > item is loaded after any other kdirlister is set up (thereby setting up t= he > later deletion-based disaster), then we get the crash. I really don't understand this, sorry... if you set up a KDirLister and use= =20 it, then there will already be the KDLCache, so if you load another lib=20 _afterwards_, why should that be the reason for the crash? It could only be= =20 if a newly loaded lib doesn't see the static objects that were created up=20 until then, and in this case it would also crash if no KDirLister was set u= p=20 before loading the lib. > this is because the kdirlistercache gets cleaned up and then klibloader > cleans up and things go bad. So right here is the problem: *who* cleans up KDirListerCache if not=20 KLibLoader? That has to be fixed. > i don't know what you are doing for reference counting in kdirlistercache, > but it seems that it gets messed up somewhere along the way. nothing. Absolutely nothing. I like optimizing things and refcounting is=20 unnecessary in this design :-) And it's also not the problem here because t= he=20 Cache that is supposed to do the refcounting is deleted! > they used to be double deleted due to klibloader. this would indeed cause > frequent log-out crashing. i then stopped cleaning these up on application > quit, allowing klibloader to do so and voila ... the kdirlister crash > instead. fun times. tell me about it... we really ought to fix this KLL thing. > one may wish to fix klibloader (who knows what else that would break, > though) but it would still leave kdirlister with an undocumented contract > with the application developer. no. The app developer has absolutely no access to KDLCache. =2D-=20 Michael Brade; KDE Developer, Student of Computer Science |-mail: echo brade !#|tr -d "c oh"|s\e\d 's/e/\@/2;s/$/.org/;s/bra/k/2' =B0--web: http://www.kde.org/people/michaelb.html KDE 3: The Next Generation in Desktop Experience --nextPart4006872.BHdeXmtyEQ Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (GNU/Linux) iD8DBQBDhZfQdK2tAWD5bo0RAt4VAKDrD9IoacWuV1R9CzlSOjOklB7ouQCeLCoq 1acSs4TfjFAsdVeGz6mQ7SY= =qdgn -----END PGP SIGNATURE----- --nextPart4006872.BHdeXmtyEQ-- --Boundary-01=_MzHlDv31v+jOF3z Content-Type: message/rfc822; name="forwarded message" Content-Transfer-Encoding: 7bit Content-Description: Michael Brade : Re: [Bug 113242] the logout crash Content-Disposition: inline From: Michael Brade Organization: K Desktop Environment To: "Aaron J. Seigo" Subject: Re: [Bug 113242] the logout crash Date: Thu, 24 Nov 2005 14:28:04 +0100 User-Agent: KMail/1.9 References: <20051120104659.14024.qmail@ktown.kde.org> <200511231540.27533.aseigo@kde.org> <200511241137.04217.brade@kde.org> In-Reply-To: <200511241137.04217.brade@kde.org> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart2251640.U4y3hhgNQN"; protocol="application/pgp-signature"; micalg=pgp-sha1 Content-Transfer-Encoding: 7bit Message-Id: <200511241428.06421.brade@kde.org> --nextPart2251640.U4y3hhgNQN Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: quoted-printable Content-Disposition: inline On Thursday 24 November 2005 11:36, Michael Brade wrote: > > this is because the kdirlistercache gets cleaned up and then klibloader > > cleans up and things go bad. > > So right here is the problem: *who* cleans up KDirListerCache if not > KLibLoader? That has to be fixed. shit, this is going up and down in my head although I should be learning...= =20 anyway, could it be that when loading a library, which uses KDirLister, tha= t=20 the KDLC is created statically _next to_ the klibloader, so that when exiti= ng=20 the app the order of deletion between KLL and KDLC is not defined. So it=20 deletes KDLC (which *should* be deleted by KLL), then KLL, which in turn=20 deletes only then the KDL? If so, we know what to fix. If not, I'm still le= ft=20 out in the rain. I will really stop thinking about it for now and resume debugging it in=20 Venezuela in December. =2D-=20 Michael Brade; KDE Developer, Student of Computer Science |-mail: echo brade !#|tr -d "c oh"|s\e\d 's/e/\@/2;s/$/.org/;s/bra/k/2' =B0--web: http://www.kde.org/people/michaelb.html KDE 3: The Next Generation in Desktop Experience --nextPart2251640.U4y3hhgNQN Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (GNU/Linux) iD8DBQBDhb/mdK2tAWD5bo0RAo+YAKDVvgzGkJXnViu4vjIlmKr1dJoTEgCgrqWg FYC6lJ0STTKrWEt2ovhMgds= =oj8o -----END PGP SIGNATURE----- --nextPart2251640.U4y3hhgNQN-- --Boundary-01=_MzHlDv31v+jOF3z-- --nextPart3848116.pPfKAy3fJ3 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.2 (GNU/Linux) iD8DBQBDlHzMdK2tAWD5bo0RAo9HAJ0Yt73dTLPKprOFvPqPJF19ZxA9rACglT5g mAsNKBOZKCfKEdLw5lJZAew= =UuBR -----END PGP SIGNATURE----- --nextPart3848116.pPfKAy3fJ3--