D28074: [kstyle] Avoid invalid iterators in qDeleteAll

[Vlad Zahorodnii]

zzag abandoned this revision.
zzag added a comment.


  Abandoned in favor of https://invent.kde.org/plasma/oxygen/-/merge_requests/1

REPOSITORY
  R113 Oxygen Theme

REVISION DETAIL
  https://phabricator.kde.org/D28074

To: zzag, #plasma
Cc: anthonyfieroni, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28074: [kstyle] Avoid invalid iterators in qDeleteAll

[Vlad Zahorodnii]

zzag added a comment.


  Alternatively, we could drop `qDeleteAll( _widgets );` because the style is 
destroyed after all widgets have been deleted.

REPOSITORY
  R113 Oxygen Theme

REVISION DETAIL
  https://phabricator.kde.org/D28074

To: zzag, #plasma
Cc: anthonyfieroni, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D28074: [kstyle] Avoid invalid iterators in qDeleteAll

[Vlad Zahorodnii]

zzag added a comment.


  Another way around is not to use `qDeleteAll`, e.g.
  
for( QWidget* widget : _widgets )
{ unregisterWidget( widget ); } // or uninstallShadows()
  
  Either way, I'm open to suggestions. :-)

REPOSITORY
  R113 Oxygen Theme

REVISION DETAIL
  https://phabricator.kde.org/D28074

To: zzag, #plasma
Cc: anthonyfieroni, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D28074: [kstyle] Avoid invalid iterators in qDeleteAll

[Anthony Fieroni]

anthonyfieroni added a comment.


  In D28074#628309 , @zzag wrote:
  
  > Well, we could do something like this (not sure whether it works though)
  >
  >   for (KWindowShadow *shadow : _shadows)
  >   disconnect(shadow, nullptr, this, nullptr);
  >  
  >   qDeleteAll(_shadows);
  >
  
  
  +1

REPOSITORY
  R113 Oxygen Theme

REVISION DETAIL
  https://phabricator.kde.org/D28074

To: zzag, #plasma
Cc: anthonyfieroni, broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, 
cblack, jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, 
himcesjf, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
ahiemstra, mart

D28074: [kstyle] Avoid invalid iterators in qDeleteAll

[Vlad Zahorodnii]

zzag added a comment.


  In D28074#628308 , @broulik wrote:
  
  > It would be nice not to mutate a list that's being deleted
  
  
  Well, we could do something like this (not sure whether it works though)
  
for (KWindowShadow *shadow : _shadows)
disconnect(shadow, nullptr, this, nullptr);

qDeleteAll(_shadows);
  
  or switch to QPointers

REPOSITORY
  R113 Oxygen Theme

REVISION DETAIL
  https://phabricator.kde.org/D28074

To: zzag, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28074: [kstyle] Avoid invalid iterators in qDeleteAll

[Kai Uwe Broulik]

broulik added a comment.


  It would be nice not to mutate a list that's being deleted

REPOSITORY
  R113 Oxygen Theme

REVISION DETAIL
  https://phabricator.kde.org/D28074

To: zzag, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28074: [kstyle] Avoid invalid iterators in qDeleteAll

[Vlad Zahorodnii]

zzag added a comment.


  it would be nice to have qSafeDeleteAll or something that takes a copy rather 
than a const ref.

REPOSITORY
  R113 Oxygen Theme

REVISION DETAIL
  https://phabricator.kde.org/D28074

To: zzag, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28074: [kstyle] Avoid invalid iterators in qDeleteAll

[Vlad Zahorodnii]

zzag added a comment.


  I would say it's undefined behavior. Either way, we should not call 
qDeleteAll on a container which is being "simultaneously" mutated.

REPOSITORY
  R113 Oxygen Theme

REVISION DETAIL
  https://phabricator.kde.org/D28074

To: zzag, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28074: [kstyle] Avoid invalid iterators in qDeleteAll

[Kai Uwe Broulik]

broulik added a comment.


  Don't you access garbage then if the shadow is destroyed during `qDeleteAll`?

REPOSITORY
  R113 Oxygen Theme

REVISION DETAIL
  https://phabricator.kde.org/D28074

To: zzag, #plasma
Cc: broulik, plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, 
jraleigh, zachus, fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, 
lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart

D28074: [kstyle] Avoid invalid iterators in qDeleteAll

[Vlad Zahorodnii]

zzag created this revision.
zzag added a reviewer: Plasma.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
zzag requested review of this revision.

REVISION SUMMARY
  When a KWindowShadow object is destroyed, it's automatically removed
  from _shadows and therefore iterators become invalid. This may cause
  problems when one is using qDeleteAll because the latter assumes that
  the passed container won't change.

TEST PLAN
  Applications don't crash when switching from Oxygen to
  Breeze widget style.

REPOSITORY
  R113 Oxygen Theme

REVISION DETAIL
  https://phabricator.kde.org/D28074

AFFECTED FILES
  kstyle/oxygenshadowhelper.cpp

To: zzag, #plasma
Cc: plasma-devel, Orage, LeGast00n, The-Feren-OS-Dev, cblack, jraleigh, zachus, 
fbampaloukas, GB_2, ragreen, ZrenBot, ngraham, himcesjf, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, ahiemstra, mart