D10691: [kactivities-stats] Fix plasmashell crash when database is broken

[Kamil Piwowarski]

kpiwowarski abandoned this revision.
kpiwowarski added a comment.


  Fixed by R159:9b47babb6c41 


REPOSITORY
  R159 KActivities Statistics

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

To: kpiwowarski, #plasma, #frameworks, hein, ivan
Cc: ivan, plasma-devel, #frameworks, ragreen, Pitel, michaelh, ZrenBot, 
ngraham, bruns, lesliezhai, ali-mohamed, jensreuterberg, abetts, sebas, apol, 
mart

D10691: [kactivities-stats] Fix plasmashell crash when database is broken

[Ivan Čukić]

ivan added a comment.


  The fix needs to be a bit more complex. We can not really rely on the user to 
fix the database.
  
  The current plan is to have kamd backup the important parts of the database 
from time to time, and to recreate the database when needed.
  
  I'll see whether I can roll out a crash=>fail-with-a-message patch for the 
LTS.
  
  The current implementation relied on my (wrong) assumption that sqlite is 
very stable and that it can not go berserk and corrupt the database. :)

REPOSITORY
  R159 KActivities Statistics

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

To: kpiwowarski, #plasma, #frameworks, hein, ivan
Cc: ivan, plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10691: [kactivities-stats] Fix plasmashell crash when database is broken

[Kamil Piwowarski]

kpiwowarski added a comment.


  @ivan When database is broken additionaly kactivitymanagerd is in endless 
loop of creating and closing processes. Each KF5 application prints to console 
message: "KActivities: FATAL ERROR: Failed to contact the activity manager 
daemon"
  
  Maybe it would be good to display dialog from kactivitymanagerd and ask user 
to fix (remove and create again) database instead of creating new processes 
infinitialy? It would fix the source of the problem which is broken database (I 
only wonder why the database is broken).
  
  I think that for now it's more important to fix crash and then help user to 
fix database. Additionally at this moment I think I don't have good C++ skills 
to fix that another way :(

REPOSITORY
  R159 KActivities Statistics

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

To: kpiwowarski, #plasma, #frameworks, hein, ivan
Cc: ivan, plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10691: [kactivities-stats] Fix plasmashell crash when database is broken

[Ivan Čukić]

ivan requested changes to this revision.
ivan added a comment.
This revision now requires changes to proceed.


  Well, in a library that 99% relies on a database, having no database is not 
something that can be gracefully handled (the returned nullptr is asserted on 
as well in the function that calls ::instance in ResultSet :) - because of 
this, I'm changing to 'Request Changes').
  
  Graceful handling instead of qFatal/Q_ASSERT could be implemented so that all 
models and queries return empty sets. Which would be useless, but as I said, I 
don't have anything against that.
  
  When we get the bug reports about empty favourites, there will be an 
additional test step "Hi, do you have this message in the plasmashell output 
'Database can not be opened...'" :)

REPOSITORY
  R159 KActivities Statistics

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

To: kpiwowarski, #plasma, #frameworks, hein, ivan
Cc: ivan, plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10691: [kactivities-stats] Fix plasmashell crash when database is broken

[Eike Hein]

hein added a comment.


  @ivan So you wrote a method that can return null, but the call sites aren't 
actually prepared to handle that gracefully? :) But I was waiting for your 
review for that context.

REPOSITORY
  R159 KActivities Statistics

BRANCH
  master

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

To: kpiwowarski, #plasma, #frameworks, hein
Cc: ivan, plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10691: [kactivities-stats] Fix plasmashell crash when database is broken

[Ivan Čukić]

ivan added a comment.


  @hein
  
  This is not really as simple as 'accept without a comment'.
  
  I don't mind this patch to be merged, but keep in mind that it just turns 
users for which plasma crashes (for which we get an explicit message as to why) 
into users who do not have certain parts of Plasma working where the error 
message will get lost in the Plasma's log. Now, as a maintainer of said parts 
of Plasma, that is your call.

REPOSITORY
  R159 KActivities Statistics

BRANCH
  master

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

To: kpiwowarski, #plasma, #frameworks, hein
Cc: ivan, plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D10691: [kactivities-stats] Fix plasmashell crash when database is broken

[Eike Hein]

hein accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R159 KActivities Statistics

BRANCH
  master

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

To: kpiwowarski, #plasma, #frameworks, hein
Cc: plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D10691: [kactivities-stats] Fix plasmashell crash when database is broken

[Kamil Piwowarski]

kpiwowarski retitled this revision from "Fix plasmashell crash when database is 
broken" to "[kactivities-stats] Fix plasmashell crash when database is broken".

REPOSITORY
  R159 KActivities Statistics

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

To: kpiwowarski, #plasma, #frameworks
Cc: plasma-devel, #frameworks, michaelh, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart