[Differential] [Updated, 31 lines] D2613: Fix some status notifier items not appearing

[davidedmundson (David Edmundson)]

davidedmundson updated this revision to Diff 6333.
davidedmundson added a comment.


  minor fix

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D2613?vs=6332&id=6333

BRANCH
  master

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

AFFECTED FILES
  dataengines/statusnotifieritem/statusnotifieritemsource.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: davidedmundson, #plasma
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Request, 31 lines] D2613: Fix some status notifier items not appearing

[davidedmundson (David Edmundson)]

davidedmundson created this revision.
davidedmundson added a reviewer: Plasma.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  Set the initial values for all the things
  This is important as Plasma::DataModel has an unsolvable bug
  When it gets data with a new key it tries to update the  QAIM roleNames
  From QML this achieves absolutely nothing as there is no signal to tell
  QQmlDelegateModel to reload the roleNames in QQmlAdapatorModel
  No matter if the row changes or the model refreshes
  This means it does not re-evaluate what bindings exist (watchedRoleIds)
  and we get properties that don't bind for this or any future SNI.
  
  The main source of this problem is syncStatus can occur before
  refreshCallback; but whilst that's easy to guard against, there's still
  multiple if{} blocks that don't set various keys in refreshCallback()
  which would all need changing too. This seemed cleaner and easier to
  manage.
  
  BUG: 366283
  BUG: 367756

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

BRANCH
  master

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

AFFECTED FILES
  dataengines/statusnotifieritem/statusnotifieritemsource.cpp

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: davidedmundson, #plasma
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

Jenkins-kde-ci: khotkeys Plasma-5.7 stable-kf5-qt5 » Linux,gcc - Build # 9 - Fixed!

[no-reply]

GENERAL INFO

BUILD SUCCESS
Build URL: 
https://build.kde.org/job/khotkeys%20Plasma-5.7%20stable-kf5-qt5/PLATFORM=Linux,compiler=gcc/9/
Project: PLATFORM=Linux,compiler=gcc
Date of build: Sat, 27 Aug 2016 20:06:17 +
Build duration: 9 min 9 sec

CHANGE SET
No changes


JUNIT RESULTS

Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 
test(s)

COBERTURA RESULTS

Cobertura Coverage Report
  

By packages
  

Jenkins-kde-ci: khotkeys Plasma-5.7 stable-kf5-qt5 » Linux,gcc - Build # 9 - Fixed!

[no-reply]

GENERAL INFO

BUILD SUCCESS
Build URL: 
https://build.kde.org/job/khotkeys%20Plasma-5.7%20stable-kf5-qt5/PLATFORM=Linux,compiler=gcc/9/
Project: PLATFORM=Linux,compiler=gcc
Date of build: Sat, 27 Aug 2016 20:06:17 +
Build duration: 9 min 9 sec

CHANGE SET
No changes


JUNIT RESULTS

Name: (root) Failed: 0 test(s), Passed: 1 test(s), Skipped: 0 test(s), Total: 1 
test(s)

COBERTURA RESULTS

Cobertura Coverage Report
  

By packages
  

[Differential] [Changed Subscribers] D2610: Make icon follow styleHints

[broulik (Kai Uwe Broulik)]

broulik added inline comments.

INLINE COMMENTS

> main.qml:21
>  
> +import QtQml 2.2
>  import QtQuick 2.0

I don't think this import is neccessary, "styleHints object is only available 
when using the Qt Quick module." anyway.

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: davidedmundson, #plasma
Cc: broulik, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, 
sebas

[Differential] [Accepted] D2612: [Logout Dialog] Set avatarPath

[davidedmundson (David Edmundson)]

davidedmundson accepted this revision.
davidedmundson added a reviewer: davidedmundson.
This revision is now accepted and ready to land.

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma, davidedmundson
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Request, 3 lines] D2612: [Logout Dialog] Set avatarPath

[broulik (Kai Uwe Broulik)]

broulik created this revision.
broulik added a reviewer: Plasma.
broulik set the repository for this revision to rPLASMAWORKSPACE Plasma 
Workspace.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  Set avatarPath to the user avatar and iconSource to the fallback icon.
  With the different margins it became apparent that it was rendering the 
avatar through IconItem.

TEST PLAN
  My avatar fills the circle fully again (and looks less pixelated)

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

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

AFFECTED FILES
  lookandfeel/contents/logout/Logout.qml

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Accepted] D2607: [System Tray] Fix warning

[davidedmundson (David Edmundson)]

davidedmundson accepted this revision.
davidedmundson added a reviewer: davidedmundson.
davidedmundson added a comment.
This revision is now accepted and ready to land.


  > No more warning on startup
  
  Well that's a bold claim :P

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma, davidedmundson
Cc: davidedmundson, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas

[Differential] [Request, 4 lines] D2611: Make trash follow styleHints

[davidedmundson (David Edmundson)]

davidedmundson created this revision.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  BUG: 348960
  
Test Plan:
Set mouse to double click. Added icon item
Clicked

Reviewers: #plasma

Subscribers: plasma-devel

Tags: #plasma

REPOSITORY
  rPLASMADESKTOP Plasma Desktop

BRANCH
  master

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

AFFECTED FILES
  applets/trash/package/contents/ui/main.qml

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: davidedmundson
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Request, 5 lines] D2610: Make icon follow styleHints

[davidedmundson (David Edmundson)]

davidedmundson created this revision.
davidedmundson added a reviewer: Plasma.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  CCBUG: 348960

TEST PLAN
  Set mouse to double click. Added icon item
  Clicked

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

BRANCH
  master

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

AFFECTED FILES
  applets/icon/package/contents/ui/main.qml

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: davidedmundson, #plasma
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Request, 123 lines] D2609: Drop custom systemsettings import, move to QtQml.styleHints

[davidedmundson (David Edmundson)]

davidedmundson created this revision.
davidedmundson added a reviewer: Plasma.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  Cleaner code

TEST PLAN
  grep all QML for any other users of SystemSettings
  Test applet handles still appear
  Test single/double click in folderview
  Test drag isn't super instant on folderview items

REPOSITORY
  rPLASMADESKTOP Plasma Desktop

BRANCH
  master

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

AFFECTED FILES
  containments/desktop/package/contents/ui/AppletAppearance.qml
  containments/desktop/package/contents/ui/ConfigIcons.qml
  containments/desktop/package/contents/ui/FolderItemDelegate.qml
  containments/desktop/package/contents/ui/FolderView.qml
  containments/desktop/package/contents/ui/main.qml
  containments/desktop/plugins/desktop/CMakeLists.txt
  containments/desktop/plugins/desktop/desktopplugin.cpp
  containments/desktop/plugins/desktop/systemsettings.cpp
  containments/desktop/plugins/desktop/systemsettings.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: davidedmundson, #plasma
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Closed] D2594: Add new session button to SessionsModel

[davidedmundson (David Edmundson)]

This revision was automatically updated to reflect the committed changes.
Closed by commit rPLASMAWORKSPACE82b0aa2ada43: Add new session button to 
SessionsModel (authored by davidedmundson).

CHANGED PRIOR TO COMMIT
  https://phabricator.kde.org/D2594?vs=6305&id=6326#toc

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D2594?vs=6305&id=6326

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

AFFECTED FILES
  components/CMakeLists.txt
  components/Messages.sh
  components/sessionsprivate/CMakeLists.txt
  components/sessionsprivate/sessionsmodel.cpp
  components/sessionsprivate/sessionsmodel.h
  components/shellprivate/CMakeLists.txt
  components/shellprivate/Messages.sh
  lookandfeel/contents/components/UserList.qml
  lookandfeel/contents/lockscreen/LockScreenUi.qml
  lookandfeel/contents/userswitcher/UserSwitcher.qml

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: davidedmundson, #plasma, mart
Cc: mart, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Request, 4 lines] D2608: [System Tray] Silence warning

[broulik (Kai Uwe Broulik)]

broulik created this revision.
broulik added a reviewer: Plasma.
broulik set the repository for this revision to rPLASMAWORKSPACE Plasma 
Workspace.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  internalSystray is (for some reason) only set in Component.onCompleted. This 
fixes the warning this causes.

TEST PLAN
  I guess not binding directly is due to some native interface initialization 
order foo?

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

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

AFFECTED FILES
  applets/systemtray/container/package/contents/ui/main.qml

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Request, 2 lines] D2607: [System Tray] Fix warning

[broulik (Kai Uwe Broulik)]

broulik created this revision.
broulik added a reviewer: Plasma.
broulik set the repository for this revision to rPLASMAWORKSPACE Plasma 
Workspace.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  It's PlasmaCore.Types.UnknownStatus, not PlasmaCore.Types.NoStatus

TEST PLAN
  No more warning on startup

REPOSITORY
  rPLASMAWORKSPACE Plasma Workspace

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

AFFECTED FILES
  applets/systemtray/package/contents/ui/items/PlasmoidItem.qml

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Abandoned] D2606: Use QGuiApplication::styleHints to check for singleClickActivation

[davidedmundson (David Edmundson)]

davidedmundson abandoned this revision.

REPOSITORY
  rPLASMADESKTOP Plasma Desktop

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: davidedmundson, #plasma
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Request, 13 lines] D2606: Use QGuiApplication::styleHints to check for singleClickActivation

[davidedmundson (David Edmundson)]

davidedmundson created this revision.
davidedmundson added a reviewer: Plasma.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REPOSITORY
  rPLASMADESKTOP Plasma Desktop

BRANCH
  master

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

AFFECTED FILES
  containments/desktop/plugins/desktop/systemsettings.cpp
  containments/desktop/plugins/desktop/systemsettings.h

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: davidedmundson, #plasma
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Accepted] D2605: [Desktop Containment] Create AppletAppearance component only once

[davidedmundson (David Edmundson)]

davidedmundson accepted this revision.
davidedmundson added a reviewer: davidedmundson.
This revision is now accepted and ready to land.

REPOSITORY
  rPLASMADESKTOP Plasma Desktop

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma, davidedmundson
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Request, 21 lines] D2605: [Desktop Containment] Create AppletAppearance component only once

[broulik (Kai Uwe Broulik)]

broulik created this revision.
broulik added a reviewer: Plasma.
broulik set the repository for this revision to rPLASMADESKTOP Plasma Desktop.
Restricted Application added a project: Plasma.
Restricted Application added a subscriber: plasma-devel.

REVISION SUMMARY
  Instead cache the created component. Also check its status rather than doing 
a string comparison on errorString and only call it if it actually failed.
  Moreover, initialize AppletAppearnce already with category set.

TEST PLAN
  Still works.
  
  QML Engine has a component cache but still I think calling Qt.createComponent 
repeatedly (especially since gc will collect it later on when it falls out of 
scope) should be avoided?

REPOSITORY
  rPLASMADESKTOP Plasma Desktop

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

AFFECTED FILES
  containments/desktop/package/contents/ui/main.qml

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: broulik, #plasma
Cc: plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas

[Differential] [Commented On] D2595: Fix comic applet appearance

[davidedmundson (David Edmundson)]

davidedmundson added a comment.


  Edit:
  
  I was wrong about this being a regression. It seems this has always been here.
  
  the switch between showing compact and full is:
   Math.max(minimumWidth, Math.min(centerLayout.comicData.image.nativeWidth * 
0.6, implicitWidth));  or implicitWidth
  
  both minimumWidth and implicitWidth are fricking enormous.
  
  Which means by default we'll show the compact on startup, and until the 
plasmoid is made even bigger than the already huge default size.
  (and in fact dragging it larger in both directions does reveal the components 
correctly)
  
  Your patch works by breaking the binding on minimumWidth used in the 
switchWidth, so that it now always shows the full version.
  Which is replacing it by a different bug.

INLINE COMMENTS

> main.qml:33
>  if (centerLayout.comicData.image) {
>  return Math.max(minimumWidth, 
> Math.min(centerLayout.comicData.image.nativeWidth * 0.6, implicitWidth));
>  } else {

you've broken this binding. minimumWidth is no longer declared within this 
scope.

though this whole line makes little sense anwyay - see comment below.

REPOSITORY
  rKDEPLASMAADDONS Plasma Addons

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: gladhorn
Cc: davidedmundson, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas

[Differential] [Commented On] D2595: Fix comic applet appearance

[davidedmundson (David Edmundson)]

davidedmundson added a comment.


  Thanks, but this is isn't the right fix
  
  comic applet hasn't changed which means it's a change in plasma-framework 
(probably) fixing it here will still leave potentially 3rd party applets 
broken, and if it is plasma-framework will still leave it broken for users of 
Plasma 5.6 or older.
  
  In theory what should happen is the root item will be used as the full 
representation if no fullRepresentation exists.
  
  Once fixed, we can merge this patch anyway, as I hate that undocumented magic 
above, and this is a lot more readable.

REPOSITORY
  rKDEPLASMAADDONS Plasma Addons

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: gladhorn
Cc: davidedmundson, plasma-devel, lesliezhai, ali-mohamed, jensreuterberg, 
abetts, sebas

Re: Review Request 128761: Fix crash on exit

[Martin Tobias Holmedahl Sandsmark]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128761/#review98718
---



Calling exit() before deleting the QApplication is explicitly not supported by 
Qt, it will lead to random crashes all over the place. See e. g. 
https://quickgit.kde.org/?p=konsole.git&a=commit&h=fe334292b5402ad0fd4b934291160ece9a12d953
 and https://bugreports.qt.io/browse/QTBUG-48709

- Martin Tobias Holmedahl Sandsmark


On Aug. 27, 2016, 9:12 a.m., Peter Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128761/
> ---
> 
> (Updated Aug. 27, 2016, 9:12 a.m.)
> 
> 
> Review request for Plasma, David Edmundson, David Faure, and Hugo Pereira Da 
> Costa.
> 
> 
> Bugs: 356940
> https://bugs.kde.org/show_bug.cgi?id=356940
> 
> 
> Repository: oxygen
> 
> 
> Description
> ---
> 
> Since Qt 5.6.0, Qt5 applications started crashing on exit. All signs
> point to this delete-on-destroy hack which was added to avoid outliving
> the plugin lifetime.
> 
> This method is wrong because the returned style is owned by the caller
> (QApplication, QProxyStyle, etc) and will cleaned up when those users
> are destructed.
> 
> Copied from breeze patch https://git.reviewboard.kde.org/r/128760/
> 
> 
> Diffs
> -
> 
>   kstyle/oxygenstyleplugin.cpp 70b90d9 
> 
> Diff: https://git.reviewboard.kde.org/r/128761/diff/
> 
> 
> Testing
> ---
> 
> Started `QT_STYLE_OVERRIDE=oxygen LD_LIBRARY_PATH=... QT_PLUGIN_PATH=... 
> wireshark -o` (an invalid option that triggers `exit(1)`) and observe a 
> heap-use-after free similar to the one reported in the bug. Apply this patch, 
> rebuild oxygen and notice that the crash is fixed. Also tested with "Testcase 
> (ASAN)" from bug 356940, crash is also gone.
> 
> 
> Thanks,
> 
> Peter Wu
> 
>

Jenkins-kde-ci: plasma-desktop master kf5-qt5 » Linux,gcc - Build # 288 - Still Unstable!

[no-reply]

GENERAL INFO

BUILD UNSTABLE
Build URL: 
https://build.kde.org/job/plasma-desktop%20master%20kf5-qt5/PLATFORM=Linux,compiler=gcc/288/
Project: PLATFORM=Linux,compiler=gcc
Date of build: Sat, 27 Aug 2016 11:46:46 +
Build duration: 16 min

CHANGE SET
Revision 1010a511e0473b0ff7b1648d706cf41ad23c6529 by yurchor: (Fix the 
statement in docs (thanks to Free de Kruijf))
  change: edit doc/kcontrol/baloo/index.docbook


JUNIT RESULTS

Name: (root) Failed: 1 test(s), Passed: 6 test(s), Skipped: 0 test(s), Total: 7 
test(s)Failed: TestSuite.appstreamtest

COBERTURA RESULTS

Cobertura Coverage Report
  PACKAGES 7/7 (100%)FILES 36/39 (92%)CLASSES 36/39 (92%)LINE 2294/3386 
(68%)CONDITIONAL 1537/3733 (41%)

By packages
  
kcms.cursortheme.xcursor
FILES 4/4 (100%)CLASSES 4/4 (100%)LINE 99/192 (52%)CONDITIONAL 
22/98 (22%)
kcms.keyboard
FILES 20/23 (87%)CLASSES 20/23 (87%)LINE 762/1511 
(50%)CONDITIONAL 605/1672 (36%)
kcms.keyboard.preview
FILES 4/4 (100%)CLASSES 4/4 (100%)LINE 500/582 (86%)CONDITIONAL 
431/1110 (39%)
kcms.keyboard.tests
FILES 5/5 (100%)CLASSES 5/5 (100%)LINE 229/231 (99%)CONDITIONAL 
236/358 (66%)
kcms.krdb
FILES 1/1 (100%)CLASSES 1/1 (100%)LINE 348/401 (87%)CONDITIONAL 
108/196 (55%)
kcms.lookandfeel
FILES 1/1 (100%)CLASSES 1/1 (100%)LINE 256/369 (69%)CONDITIONAL 
83/195 (43%)
kcms.lookandfeel.autotests
FILES 1/1 (100%)CLASSES 1/1 (100%)LINE 100/100 
(100%)CONDITIONAL 52/104 (50%)

Re: task manager lines instead of blocks

[Kai Uwe Broulik]

I was under the impression that this was already well under way...

Note that plasma frameworks freeze is next Saturday (given the recent breaking 
changes in the theme I don't feel comfortable with theme changes that late in 
the cycle) and the Frameworks release thereafter will be tight for Plasma 5.8 
(and also won't be a dependency) so I would think it's too late for 5.8 now? 
Especially given how easy it is to get margins wrong in tasks.svgz and 5.8 is 
LTS.

Re: Review Request 128761: Fix crash on exit

[Hugo Pereira Da Costa]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128761/#review98712
---


Ship it!




Ship It!

- Hugo Pereira Da Costa


On Aug. 27, 2016, 9:12 a.m., Peter Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128761/
> ---
> 
> (Updated Aug. 27, 2016, 9:12 a.m.)
> 
> 
> Review request for Plasma, David Edmundson, David Faure, and Hugo Pereira Da 
> Costa.
> 
> 
> Bugs: 356940
> https://bugs.kde.org/show_bug.cgi?id=356940
> 
> 
> Repository: oxygen
> 
> 
> Description
> ---
> 
> Since Qt 5.6.0, Qt5 applications started crashing on exit. All signs
> point to this delete-on-destroy hack which was added to avoid outliving
> the plugin lifetime.
> 
> This method is wrong because the returned style is owned by the caller
> (QApplication, QProxyStyle, etc) and will cleaned up when those users
> are destructed.
> 
> Copied from breeze patch https://git.reviewboard.kde.org/r/128760/
> 
> 
> Diffs
> -
> 
>   kstyle/oxygenstyleplugin.cpp 70b90d9 
> 
> Diff: https://git.reviewboard.kde.org/r/128761/diff/
> 
> 
> Testing
> ---
> 
> Started `QT_STYLE_OVERRIDE=oxygen LD_LIBRARY_PATH=... QT_PLUGIN_PATH=... 
> wireshark -o` (an invalid option that triggers `exit(1)`) and observe a 
> heap-use-after free similar to the one reported in the bug. Apply this patch, 
> rebuild oxygen and notice that the crash is fixed. Also tested with "Testcase 
> (ASAN)" from bug 356940, crash is also gone.
> 
> 
> Thanks,
> 
> Peter Wu
> 
>

Re: Review Request 128761: Fix crash on exit

[Hugo Pereira Da Costa]

> On Aug. 27, 2016, 9:14 a.m., Hugo Pereira Da Costa wrote:
> > mmm. But then i think it is better (for commit history etc), to just revert 
> > the incriminated commit (and in oxygen as well), with a possible link to 
> > this RB. makes sense ?
> 
> Peter Wu wrote:
> I tested the reverts for breeze and oxygen and that also works on Qt 
> 5.7.0 (tested with `wireshark -o` (`exit()`s), `wireshark` + quit and the 
> testcase).
> 
> Reverted:
> breeze 1430dd22ae74a09ba289dafe2be15628a0eddc15
> oxygen e0376e1597f6e6b49e6343874e141b439565b749
> 
> Due to `delete qApp->style()` however, it is in theory still possible to 
> reach a use-after-free since QApplication owns it.
> 
> I'll leave it up to you whether you take this patch or revert it, I'm 
> fine with either.

i agree, obviously delete qApp->Style() must go too, and in fact that was the 
original patch introduced by D Faure, to oxygen (commit 
2ffe20e1bfe93c921c5372b4d21447b1de308d4b), which i then copied to breeze with 
many other changes and wich was later on changed to the signal/slot delta 
function by D Edmundson.
So in the end, maybe it is indeed better to clean it all up in one go, possibly 
adding the relevant effectively reverted commits in the comments.


- Hugo


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128761/#review98709
---


On Aug. 27, 2016, 9:12 a.m., Peter Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128761/
> ---
> 
> (Updated Aug. 27, 2016, 9:12 a.m.)
> 
> 
> Review request for Plasma, David Edmundson, David Faure, and Hugo Pereira Da 
> Costa.
> 
> 
> Bugs: 356940
> https://bugs.kde.org/show_bug.cgi?id=356940
> 
> 
> Repository: oxygen
> 
> 
> Description
> ---
> 
> Since Qt 5.6.0, Qt5 applications started crashing on exit. All signs
> point to this delete-on-destroy hack which was added to avoid outliving
> the plugin lifetime.
> 
> This method is wrong because the returned style is owned by the caller
> (QApplication, QProxyStyle, etc) and will cleaned up when those users
> are destructed.
> 
> Copied from breeze patch https://git.reviewboard.kde.org/r/128760/
> 
> 
> Diffs
> -
> 
>   kstyle/oxygenstyleplugin.cpp 70b90d9 
> 
> Diff: https://git.reviewboard.kde.org/r/128761/diff/
> 
> 
> Testing
> ---
> 
> Started `QT_STYLE_OVERRIDE=oxygen LD_LIBRARY_PATH=... QT_PLUGIN_PATH=... 
> wireshark -o` (an invalid option that triggers `exit(1)`) and observe a 
> heap-use-after free similar to the one reported in the bug. Apply this patch, 
> rebuild oxygen and notice that the crash is fixed. Also tested with "Testcase 
> (ASAN)" from bug 356940, crash is also gone.
> 
> 
> Thanks,
> 
> Peter Wu
> 
>

Re: Review Request 128761: Fix crash on exit

[Peter Wu]

> On Aug. 27, 2016, 11:14 a.m., Hugo Pereira Da Costa wrote:
> > mmm. But then i think it is better (for commit history etc), to just revert 
> > the incriminated commit (and in oxygen as well), with a possible link to 
> > this RB. makes sense ?

I tested the reverts for breeze and oxygen and that also works on Qt 5.7.0 
(tested with `wireshark -o` (`exit()`s), `wireshark` + quit and the testcase).

Reverted:
breeze 1430dd22ae74a09ba289dafe2be15628a0eddc15
oxygen e0376e1597f6e6b49e6343874e141b439565b749

Due to `delete qApp->style()` however, it is in theory still possible to reach 
a use-after-free since QApplication owns it.

I'll leave it up to you whether you take this patch or revert it, I'm fine with 
either.


- Peter


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128761/#review98709
---


On Aug. 27, 2016, 11:12 a.m., Peter Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128761/
> ---
> 
> (Updated Aug. 27, 2016, 11:12 a.m.)
> 
> 
> Review request for Plasma, David Edmundson, David Faure, and Hugo Pereira Da 
> Costa.
> 
> 
> Bugs: 356940
> https://bugs.kde.org/show_bug.cgi?id=356940
> 
> 
> Repository: oxygen
> 
> 
> Description
> ---
> 
> Since Qt 5.6.0, Qt5 applications started crashing on exit. All signs
> point to this delete-on-destroy hack which was added to avoid outliving
> the plugin lifetime.
> 
> This method is wrong because the returned style is owned by the caller
> (QApplication, QProxyStyle, etc) and will cleaned up when those users
> are destructed.
> 
> Copied from breeze patch https://git.reviewboard.kde.org/r/128760/
> 
> 
> Diffs
> -
> 
>   kstyle/oxygenstyleplugin.cpp 70b90d9 
> 
> Diff: https://git.reviewboard.kde.org/r/128761/diff/
> 
> 
> Testing
> ---
> 
> Started `QT_STYLE_OVERRIDE=oxygen LD_LIBRARY_PATH=... QT_PLUGIN_PATH=... 
> wireshark -o` (an invalid option that triggers `exit(1)`) and observe a 
> heap-use-after free similar to the one reported in the bug. Apply this patch, 
> rebuild oxygen and notice that the crash is fixed. Also tested with "Testcase 
> (ASAN)" from bug 356940, crash is also gone.
> 
> 
> Thanks,
> 
> Peter Wu
> 
>

Re: Review Request 128761: Fix crash on exit

[Hugo Pereira Da Costa]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128761/#review98709
---



mmm. But then i think it is better (for commit history etc), to just revert the 
incriminated commit (and in oxygen as well), with a possible link to this RB. 
makes sense ?

- Hugo Pereira Da Costa


On Aug. 27, 2016, 9:12 a.m., Peter Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128761/
> ---
> 
> (Updated Aug. 27, 2016, 9:12 a.m.)
> 
> 
> Review request for Plasma, David Edmundson, David Faure, and Hugo Pereira Da 
> Costa.
> 
> 
> Bugs: 356940
> https://bugs.kde.org/show_bug.cgi?id=356940
> 
> 
> Repository: oxygen
> 
> 
> Description
> ---
> 
> Since Qt 5.6.0, Qt5 applications started crashing on exit. All signs
> point to this delete-on-destroy hack which was added to avoid outliving
> the plugin lifetime.
> 
> This method is wrong because the returned style is owned by the caller
> (QApplication, QProxyStyle, etc) and will cleaned up when those users
> are destructed.
> 
> Copied from breeze patch https://git.reviewboard.kde.org/r/128760/
> 
> 
> Diffs
> -
> 
>   kstyle/oxygenstyleplugin.cpp 70b90d9 
> 
> Diff: https://git.reviewboard.kde.org/r/128761/diff/
> 
> 
> Testing
> ---
> 
> Started `QT_STYLE_OVERRIDE=oxygen LD_LIBRARY_PATH=... QT_PLUGIN_PATH=... 
> wireshark -o` (an invalid option that triggers `exit(1)`) and observe a 
> heap-use-after free similar to the one reported in the bug. Apply this patch, 
> rebuild oxygen and notice that the crash is fixed. Also tested with "Testcase 
> (ASAN)" from bug 356940, crash is also gone.
> 
> 
> Thanks,
> 
> Peter Wu
> 
>

Re: Review Request 128761: Fix crash on exit

[Peter Wu]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128761/
---

(Updated Aug. 27, 2016, 11:12 a.m.)


Review request for Plasma, David Edmundson, David Faure, and Hugo Pereira Da 
Costa.


Changes
---

Just remove delete hack completely


Summary (updated)
-

Fix crash on exit


Bugs: 356940
https://bugs.kde.org/show_bug.cgi?id=356940


Repository: oxygen


Description (updated)
---

Since Qt 5.6.0, Qt5 applications started crashing on exit. All signs
point to this delete-on-destroy hack which was added to avoid outliving
the plugin lifetime.

This method is wrong because the returned style is owned by the caller
(QApplication, QProxyStyle, etc) and will cleaned up when those users
are destructed.

Copied from breeze patch https://git.reviewboard.kde.org/r/128760/


Diffs (updated)
-

  kstyle/oxygenstyleplugin.cpp 70b90d9 

Diff: https://git.reviewboard.kde.org/r/128761/diff/


Testing (updated)
---

Started `QT_STYLE_OVERRIDE=oxygen LD_LIBRARY_PATH=... QT_PLUGIN_PATH=... 
wireshark -o` (an invalid option that triggers `exit(1)`) and observe a 
heap-use-after free similar to the one reported in the bug. Apply this patch, 
rebuild oxygen and notice that the crash is fixed. Also tested with "Testcase 
(ASAN)" from bug 356940, crash is also gone.


Thanks,

Peter Wu

Re: Review Request 128760: Fix crash on exit

[Peter Wu]

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128760/
---

(Updated Aug. 27, 2016, 11:04 a.m.)


Review request for Plasma, David Edmundson, David Faure, and Hugo Pereira Da 
Costa.


Changes
---

Remove delete hack completely


Summary (updated)
-

Fix crash on exit


Bugs: 356940
https://bugs.kde.org/show_bug.cgi?id=356940


Repository: breeze


Description (updated)
---

Since Qt 5.6.0, Qt5 applications started crashing on exit. All signs
point to this delete-on-destroy hack which was added to avoid outliving
the plugin lifetime.

This method is wrong because the returned style is owned by the caller
(QApplication, QProxyStyle, etc) and will cleaned up when those users
are destructed.


Diffs (updated)
-

  kstyle/breezestyleplugin.cpp 083100e 

Diff: https://git.reviewboard.kde.org/r/128760/diff/


Testing (updated)
---

Ran the updated test.sh from "Testcase (ASAN) with normal QApplication::quit 
and exit()" from bug https://bugs.kde.org/show_bug.cgi?id=356940, no longer 
crashes. Tested with Qt 5.7.0.


Thanks,

Peter Wu

task manager lines instead of blocks

[kainz.a]

Should I work on it?

Andreas

Re: Review Request 128760: Fix crash when using QProxyStyle and exit()

[Hugo Pereira Da Costa]

> On Aug. 26, 2016, 4:22 a.m., Anthony Fieroni wrote:
> > kstyle/breezestyleplugin.cpp, line 44
> > 
> >
> > This must be not (!inited).
> > However this is not proper fix. Correct and test patch in this way
> > 
> > QPointer style = new Style;
> > 
> > Below unchanged, so when QPointer got delete it hold nullptr by itself 
> > and delete will be safe.
> 
> Peter Wu wrote:
> This does not work since the interface requires a QStyle pointer. If I 
> use this, I guess the caller unwraps it into a raw pointer and the issue is 
> still triggered.
> 
> Good point about `if (!inited)`, forgot to change this while renaming. 
> I'll fix it for the next version. Do you know the root cause of the original 
> issue that required the use of this? It is not documented by Qt.
> 
> Anthony Fieroni wrote:
> QPointer track QObject and he knows when it's life or not, so issue must 
> be fixed, at end
> 
> return style.data();
> 
> Peter Wu wrote:
> `style.data()` should not be needed because the cast operator of QPointer 
> does this implicitly. (I did test it though and it makes no difference.)
> 
> Digging further, I am not convinced that this patch (or the previous 
> code) is correct. While the result of `QApplication::style()` is ignored, its 
> parent is set to a `QApplication` instance. So when a program exits normally, 
> QApplication will be taking care of the QStyle instance. When `exit()` is 
> invoked, the QApplication destructor does not seem to be called which 
> probably led to the code in the first place.
> 
> I am tempted to remove the delete code completely, it seems a 
> hack/workaround at the wrong level. Thoughts?
> 
> Anthony Fieroni wrote:
> You can test to remove manual deleting with style->setParent(this)
> 
> Peter Wu wrote:
> Setting `style->setParent(0)` in the two places (the initial 
> `QApplication::style()` call and the internal `QProxyStyle` code from the 
> testcase) prevents the crash on `exit()`. What about removing the `delete 
> style` code, what are the risks for that?

I would vote for removing the "delete code". 
I know it was introduce to fix a crash in e.g. kioclient5, but i don't think 
this is the right patch. It basically results in ambiguous ownership of the 
Style objects (QApp, and/or QStylePlugin), which is wrong (and creates all sort 
of problems. not only this one).
The issue of QApplication not being destroyed on exit(), which was the reason 
why this code was introduced, should be fixed upstream.

I don't think deleting only the "first" QStyle is a proper fix either. (does 
not solve the ambiguous ownership, and has no guarantee to fix the original 
issue either, basically you get the worse of both worlds :))


- Hugo


---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128760/#review98667
---


On Aug. 25, 2016, 9:56 p.m., Peter Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128760/
> ---
> 
> (Updated Aug. 25, 2016, 9:56 p.m.)
> 
> 
> Review request for Plasma, David Edmundson, David Faure, and Hugo Pereira Da 
> Costa.
> 
> 
> Bugs: 356940
> https://bugs.kde.org/show_bug.cgi?id=356940
> 
> 
> Repository: breeze
> 
> 
> Description
> ---
> 
> Do not delete all style instances which we create, restrict ourselves to
> the first instance. I have no idea if the delete hack is still needed,
> but let's keep it until it is certain that it is unneeded.
> 
> 
> Diffs
> -
> 
>   kstyle/breezestyleplugin.cpp 083100e 
> 
> Diff: https://git.reviewboard.kde.org/r/128760/diff/
> 
> 
> Testing
> ---
> 
> Used "Testcase (with ASAN)" from bug 
> https://bugs.kde.org/show_bug.cgi?id=356940. Run directly, no more crashes. 
> Double-checked with a breakpoint on Breeze::StylePlugin::create that the 
> second instance is called through QProxyStyle.
> 
> 
> Thanks,
> 
> Peter Wu
> 
>