From kde-core-devel Tue Nov 22 10:43:37 2011 From: =?utf-8?q?Thomas_L=C3=BCbking?= Date: Tue, 22 Nov 2011 10:43:37 +0000 To: kde-core-devel Subject: Re: Review Request: Ensure Plasma Desktop does not crash on exit with Message-Id: <20111122104337.13852.96521 () vidsolbach ! de> X-MARC-Message: https://marc.info/?l=kde-core-devel&m=132195873524241 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============4190736718641618159==" --===============4190736718641618159== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable > On Nov. 22, 2011, 9:24 a.m., Thomas L=C3=BCbking wrote: > > plasma/private/containment_p.h, line 71 > > > > > > ibeg your pardon? > > to me this looks like you delete the same entry forever (what *will= * crash at a point) since you don't remove it - should be "takeFirst", yesn= o? > = > Ben Cooksley wrote: > Other code handles this for us - which is why qDeleteAll() crashes. > = > Thomas L=C3=BCbking wrote: > sorry, but the only way i see this could be correct is that qlist (fu= n with templates) subclasses pointer Types and reimplements delete to autom= atically remove them from the list, what is, nicely spoken, sick. > is there any detailed (official) explanation on this? > = > Aaron J. Seigo wrote: > see ContainmentPrivate::appletDestroyed(Plasma::Applet *applet) in kd= elibs/plasma/containment.cpp > = > Ben Cooksley wrote: > Only information I have is https://git.reviewboard.kde.org/r/102981/ > I haven't checked the code, but i'm guessing that Plasma::Applet must= do something similar to what Akregator does. > In any case, I tested and Plasma does not infinitely loop - it does c= lose properly. okey, thanks - that explains things. qlist needs a mutex (at least as friend for qdeleteall) - and imho this her= e needs another comment since esp. the superfluous clear() makes it look li= ke i bug. (written by touch in a shaky train ;-) - Thomas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103202/#review8384 ----------------------------------------------------------- On Nov. 22, 2011, 7:39 a.m., Ben Cooksley wrote: > = > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103202/ > ----------------------------------------------------------- > = > (Updated Nov. 22, 2011, 7:39 a.m.) > = > = > Review request for kdelibs and Plasma. > = > = > Description > ------- > = > Qt 4.8 contains changes which break qDeleteAll() in certain scenarios. Th= is patch fixes one of those scenarios to ensure that Plasma Desktop does no= t crash on quit or logout. > = > = > Diffs > ----- > = > plasma/private/containment_p.h 4025bf4 = > = > Diff: http://git.reviewboard.kde.org/r/103202/diff/diff > = > = > Testing > ------- > = > Compiled, Plasma Desktop no longer crashes on exit. > = > = > Thanks, > = > Ben Cooksley > = > --===============4190736718641618159== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable
This is an automatically generated e-mail. To reply, visit: http://git.revie= wboard.kde.org/r/103202/

On November 22nd, 2011, 9:24 a.m., Thomas L= =C3=BCbking wrote:

= = =
plasma/private/containment_p.h (Diff revision 1) 
public:
 

    71
    
        while (!applets.isEmpty()) {

  


 




  
ibeg your=
 pardon?
to me this looks like you delete the same entry forever (what *will* crash =
at a point) since you don't remove it - should be "takeFirst"=
, yesno?

 




 
On November 22nd, 2011, 9:27 a.m., Ben Cooksley wrote:

 

  
Other cod=
e handles this for us - which is why qDeleteAll() crashes.

 






 
On November 22nd, 2011, 9:57 a.m., Thomas L=C3=BCbking wrote:

 

  
sorry, bu=
t the only way i see this could be correct is that qlist (fun with template=
s) subclasses pointer Types and reimplements delete to automatically remove=
 them from the list, what is, nicely spoken, sick.
is there any detailed (official) explanation on this?

 






 
On November 22nd, 2011, 10:11 a.m., Aaron J. Seigo wrote:

 

  
see Conta=
inmentPrivate::appletDestroyed(Plasma::Applet *applet) in kdelibs/plasma/co=
ntainment.cpp

 






 
On November 22nd, 2011, 10:12 a.m., Ben Cooksley wrote:

 

  
Only info=
rmation I have is https://git.reviewboard.kde.org/r/102981/
I haven't checked the code, but i'm guessing that Plasma::Applet mu=
st do something similar to what Akregator does.
In any case, I tested and Plasma does not infinitely loop - it does close p=
roperly.

 











okey, thanks - that explains things.
qlist needs a mutex (at least as friend for qdeleteall) - and imho this her=
e needs another comment since esp. the superfluous clear() makes it look li=
ke i bug. (written by touch in a shaky train ;-)








- Thomas






On November 22nd, 2011, 7:39 a.m., Ben Cooksley wrote:









 

  


Review request for kdelibs and Plasma.


By Ben Cooksley.




Updated Nov. 22, 2011, 7:39 a.m.








Descripti=
on 

 

 

  
   
Qt 4.8 contains changes which break qDeleteAll() in certain =
scenarios. This patch fixes one of those scenarios to ensure that Plasma De=
sktop does not crash on quit or logout.

  		
 






Testing <=
/h1>


 

  
   
Compiled, Plasma Desktop no longer crashes on exit.

  		
 








Diffs=
 


    

 
  • plasma/private/containment_p.h (4025bf4)
    • 





View Diff





  		
 











  

 



--===============4190736718641618159==--