Bug 1362

Created attachment 1071 [details]
Fix patch

The patch fix the buf for me.

(In reply to comment #1)
> Created attachment 1071 [details]
> Fix patch
> The patch fix the buf for me.
This have not fixed. But possible QT3 updating fix that.
The bug is strange crash into QString::isNull() function for KJobViewer context.

(In reply to comment #2)
> (In reply to comment #1)
> > Created attachment 1071 [details] [details]
> > Fix patch
> > The patch fix the buf for me.
> This have not fixed. But possible QT3 updating fix that.
> The bug is strange crash into QString::isNull() function for KJobViewer
> context.
The bug still meet. For that I have drop all my fix attempts and will continue trace.

Created attachment 1342 [details]
KJobViewer crash

KJobViewer continue crash on 2.5.13.2 and I have include more full back trace.

Is this report still valid?

Created attachment 1643 [details]
Crash report

Yes, the proplem is actual for now and I get the crash today.

Do you only get the crash on 3.5.13.x or have you also observed it on R14.0.0?

I tried testing this with R14. I am unable to keep Kjobviewer in the system tray. When I select Close rather than Quit, the app nonetheless quits and disappears from the system tray. Running ps confirms the app has stopped running despite selecting Close rather than Quit.

In R14, when I leave Kjobviewer minimized to the panel, I do not experience a crash when exiting Trinity. I do not have 3.5.13.2 to test.

(In reply to comment #7)
> Do you only get the crash on 3.5.13.x or have you also observed it on R14.0.0?
I have not R14.0.0 builds for test the problem.

(In reply to comment #8)
> I tried testing this with R14. I am unable to keep Kjobviewer in the system
> tray. When I select Close rather than Quit, the app nonetheless quits and
> disappears from the system tray. Running ps confirms the app has stopped
> running despite selecting Close rather than Quit.
For get that behavior you have to start KJobViewer, set flag "Keep window permanent", Select all printers' jobs display by menu "Filter"->"Select printer"->"All". And next close main window, this will stay put into the system tray.
But the crash mostly reproduced for more printers into the printers list and after several tries.

(In reply to comment #9)
> In R14, when I leave Kjobviewer minimized to the panel, I do not experience a
> crash when exiting Trinity. I do not have 3.5.13.2 to test.
This by you did not set the target for self for get permanent print jobs observing into the system tray. :)

Okay, I now see the small "Keep window permanent" check box at the lower left of the window. When that check box is enabled, closing the window with the title bar Close button minimizes the app to the system tray.

When selecting File->Close from the menu, the app terminates the same as using File->Quit.

Although I don't yet see the crashes, I notice two bugs:

* Setting "Keep window permanent" is not saved. Users should not have to manually reset that option with each usage.

* File->Close does not function the same as the title bar Close button. Either remove File->Close or fix the bug.

An oddity:

main.cpp:30 has:

"noshow", I18N_NOOP("Show job viewer at startup"), 0},

kjobviewer.cpp:112 has:

bool 	showIt = args->isSet("show");

Seems "noshow" in main.cpp should be "show"?

(In reply to comment #11)
> When selecting File->Close from the menu, the app terminates the same as using
> File->Quit.
Yes, I have seen that also.

> Although I don't yet see the crashes, 
Do you really close TDE session on KJobViewer on tray and all printers jobs selected here?

> I notice two bugs:
> * Setting "Keep window permanent" is not saved. Users should not have to
> manually reset that option with each usage.
Yes, it is

> main.cpp:30 has:
> "noshow", I18N_NOOP("Show job viewer at startup"), 0},
> kjobviewer.cpp:112 has:
> bool     showIt = args->isSet("show");
> Seems "noshow" in main.cpp should be "show"?
I have also set that locally for allow start from Autostart by command: "kjobviewer --all --noshow %i %m" and next remove this but the flag is saved somewhere. :)

>Do you really close TDE session on KJobViewer on tray and all printers jobs
>selected here?
I need exact instructions how to duplicate the bug.

I strongly suspect this is fixed in R14.0.0 due to the threading enhancements in TQt3.  However, I will hold off on marking this SRUONLY until exact details on how to trigger the bug in 3.5.13.x are given and R14 is verified not to crash under the same conditions.

(In reply to comment #14)
> >Do you really close TDE session on KJobViewer on tray and all printers jobs
> >selected here?
> I need exact instructions how to duplicate the bug.
I already tell you that the bug is not exactly but probability reproduced at time TDE session closing where more threads conflict and can use main resources. Then for rise the crash probability you:
- Ensure about 5-10 printers into the printers list.
- Start KJobViewer.
- Set flag "Keep window permanent".
- Select all printers' jobs display by menu "Filter"->"Select
printer"->"All".
- Close main window of KJobViewer.
- Exit the TDE session after some time pass, I have the exit on may work day end.
- At the TDE session exit you can get the DrKonqi crash dialog, without frames on black background.

Thanks for the detailed test plan. That should help others debug.

>Ensure about 5-10 printers into the printers list.
Oh, my! Who has that many printers available except enterprise users? :-) I wonder whether this bug is a corner case?

I have only one printer in my network. Likely I could create additional "dummy" printers to test. Before I do that I'll instead leave kjobviewer in the system tray each session and see how that goes for several days.

Created attachment 1672 [details]
Crash after printer's tasks queue clear

(In reply to comment #17)
> I have only one printer in my network. Likely I could create additional "dummy"
> printers to test. Before I do that I'll instead leave kjobviewer in the system
> tray each session and see how that goes for several days.
Yes, mostly from my 10 printers are "dummy". :)
And really active printer is only one.

I have seen new crashes not only at TDE session closing but one time after manual closing from tray and one after manual print queue clear when KJobViewer itself close. For second case KJobViewer was started at TDE start by some tasks into the printer queue left.

Created attachment 1733 [details]
KJobViewer crash fix pacth

(In reply to comment #15)
> I strongly suspect this is fixed in R14.0.0 due to the threading enhancements
> in TQt3.  However, I will hold off on marking this SRUONLY until exact details
> on how to trigger the bug in 3.5.13.x are given and R14 is verified not to
> crash under the same conditions.
Most likely no!

After observing source code into kmjobviewer.cpp (kdeligs) I have seen bad printer's objects (KMPrinter) storing into m_printers as pointers to the objects and real management and storing ones behind function it(*(KMFactory::self()->manager()->printerList(false))); then as result of some situation possible actuality printers list broken into m_printers and access try to already freed object and next crash.

After more tests I have found out this problem fix reloading the actual printers list by loadPrinters(); before this removing try into KMJobViewer::removeFromManager().

Also, for prevent access to the printers list from Jobs update thread I have append force the timer freeing into ~KJobViewerApp() destructor into file kjobviewer.cpp (kdebase).

With the fixes I have not any crashes into KJobViewer for week.

Created attachment 1734 [details]
KJobViewer crash fix pacth (kdebase)

Created attachment 2539 [details]
KJobViewer (tdelibs part) crash fix and autostart appending to the system tray

KJobViewer start at the TDE starting is added by new config parameter "Jobs/KeepWindow", bound to the window's "Hold permanent the window".

Created attachment 2540 [details]
KJobViewer (tdebase part) crash fix and autostart appending to the system tray

KJobViewer start at the TDE starting is added by new config parameter "Jobs/KeepWindow", bound to the window's "Hold permanent the window".

Includes new file kjobviewer-autostart.desktop for place that to autostart folder /opt/trinity/share/autostart like by line into file tdeprint-trinity.install:
debian/tmp/opt/trinity/share/autostart/kjobviewer-autostart.desktop

Set to PATCHAVAIL.

The patches fine work now on version R14.0.1.

Hi Roman, I take the bug is still valid, right?

(In reply to Michele Calgaro from comment #25)
> Hi Roman, I take the bug is still valid, right?
No, I have not the crashes with this patch, since I use only my builds and with him: http://oscada.org/wiki/Special:MyLanguage/Sub-projects/Automation_Linux_distributive

Changing the way the window is kept open across sessions seems like a good idea.

However, the solution to the primary problem, adding loadPrinters() in the destructor, looks more like a hack than solving a real problem. I think there is still some work needed to solve a real problem.

Roman, Slavek, thanks for the additional feedback!

(In reply to Slávek Banko from comment #27)
> Changing the way the window is kept open across sessions seems like a good
> idea.
> 
> However, the solution to the primary problem, adding loadPrinters() in the
> destructor, looks more like a hack than solving a real problem. I think
> there is still some work needed to solve a real problem.
I have no problem in adding loadPrinters() to the destructor and I think this solution is true relating the crashing situation when some data there can be not initiated yet.

So, if in your service you have an author of this deprecated code which can explain this behavior, you can wait for the true relating to the crashing situation. :)

And I will continue to build this code with this just working patch.

If you so dislike this part, I can prepare a patch only with the code of KJobViewer starting and holding in the system tray, with changing the file debian/tdeprint-trinity.install for adding "autostart/kjobviewer-autostart.desktop". But just after that you and other users will start to regular observe this crashing at exit, due to KJobViewer now will work before each exit!

First patch (except the 'loadPrinters()' thing) pushed to master as commit 9dc23ede. My apologies Roman: I signed it off with your name in the comment but I mistakenly use my account as author of the patch and I noticed that too late to rectify it :-(

Regarding the 'loadPrinter()', I think it is not the correct way to fix the problem. I am going to prepare a PR on TGW with a different solution and will need your help to test it since I can't reproduce the problem here.

First patch backported to R14.0.x in commit 41cceb16.

Second patch adjusted and pushed to master in commit 28771943 and r14.0.x in commit 27b56d1f. This time properly authored :-)

The problem is easy to reproduce. Follow steps at comment 16 and after closing the kjobviewer window make sure to delete one printer from TDE printer configuration. Then right click the kjobviewer icon and quit the application. The crash happens because kjobviewer is still trying to remove a no longer existing printer from the manager.

(In reply to Michele Calgaro from comment #30)
> Regarding the 'loadPrinter()', I think it is not the correct way to fix the
> problem.
I think if it works long time, it is correct!

> The crash happens because kjobviewer is still trying to remove a no longer 
> existing printer from the manager.
And what loadPrinter() prevents.

Do not forget about "autostart/kjobviewer-autostart.desktop", see comment 29!

> First patch backported to R14.0.x in commit 41cceb16.
OK, I am going to see about changing the my patch set applying now: http://ftp.oscada.org/Debian/src/

Hi Roman,
the autostart desktop file has already been pushed to TGW as mentioned in comment 32.

>I think if it works long time, it is correct!
Not necessarily. If it works for long time for a user, it means it works. Doesn't mean it is necessarily the correct way to fix it, it could be a quick work-around.
I am looking into it, we should have a fix (perhaps yours) within this week :-)

(In reply to Michele Calgaro from comment #35)
> the autostart desktop file has already been pushed to TGW as mentioned in
> comment 32.

Yes, but it is wrong in the line:
  X-TDE-autostart-condition=tdeprintrc:Jobs:KeepWindow:false

Since it must be auto-started at keeping the window! So the correct line is:
  X-TDE-autostart-condition=tdeprintrc:Jobs:KeepWindow:true

And the file is missed in the autostart folder finally, but the file debian/tdeprint-trinity.install was not changed for appending the line:
  debian/tmp/opt/trinity/share/autostart/kjobviewer-autostart.desktop

> >I think if it works long time, it is correct!
> Not necessarily. If it works for long time for a user, it means it works.

Ok, let get you started to catch the occasional crashes from the beginning, and I am going to send you the crash reports, if you have many time and you don't appreciate my time in the problem catching and fixing! :)

Hi Roman,

> Yes, but it is wrong in the line:
>  X-TDE-autostart-condition=tdeprintrc:Jobs:KeepWindow:false
> Since it must be auto-started at keeping the window! So the correct line is:
>  X-TDE-autostart-condition=tdeprintrc:Jobs:KeepWindow:true
true and false only affect the behaviour on start up if the KeepWindow option is not set in the user config. Once it is set, it is obeyed and KJobViewer is correctly started.
The reason why it is false and not true, is that most of the users don't need/use KJobViewer, so the default is to not autostart it. For you use case, you just need to run KJobViewer once, toggle the Keep Window checkbox off-on once (important !) and you should be ok. I tested on my computer and at the next reboot it will autostart. 
If this is not what you see, write here and we can check together

> but the file debian/tdeprint-trinity.install was not changed
Ups! That a mistake on my side. My apologies. Will rectify but unfortunately it is too late to make it for R14.0.8, official packages already frozen.

Re the remaining missing bit of the patch, Slavek and I are currently disagreeing between us on whether to merge it as is or looking for a different solution, which is why this bug is still open. We have convene to look at this again later this year and make a decision on that.

> if you have many time and you don't appreciate my time in the problem catching and 
> fixing! :)
Lastly, we do appreciate your effort although we may look at your work sometimes with great delay, I admit. Not trying to convince you if you think otherwise, just saying  what I think :-) Keep it up!

(In reply to Michele Calgaro from comment #37)
> > Yes, but it is wrong in the line:
> >  X-TDE-autostart-condition=tdeprintrc:Jobs:KeepWindow:false
> > Since it must be auto-started at keeping the window! So the correct line is:
> >  X-TDE-autostart-condition=tdeprintrc:Jobs:KeepWindow:true
> true and false only affect the behaviour on start up if the KeepWindow
> option is not set in the user config. Once it is set, it is obeyed and
> KJobViewer is correctly started.

No, this option points for what value of the configuration tdeprintrc:Jobs:KeepWindow the program must be started and for false KJobViewer is never started, but for KeepWindow false the program exits just the window closed what is also the starting to the system tray. And it made specially for the KeepWindow original behaviour!

> The reason why it is false and not true, is that most of the users don't
> need/use KJobViewer, so the default is to not autostart it. 

Really, most users the DE have not any printer? I think most users of any DE have one at the least printer but very few users have neither, to make such behaviour by default. Anyway the few users may uncheck the flag KeepWindow and will not see it more but what must do me now? Other point, it is you need just to set KeepWindow to false directly, but do not change the autostart control file!

> If this is not what you see, write here and we can check together

This is not, I use this patch 4 years in very different environments and printers number!

(In reply to Roman Savochenko from comment #38)
> (In reply to Michele Calgaro from comment #37)
> > > Yes, but it is wrong in the line:
> > >  X-TDE-autostart-condition=tdeprintrc:Jobs:KeepWindow:false
> > > Since it must be auto-started at keeping the window! So the correct line is:
> > >  X-TDE-autostart-condition=tdeprintrc:Jobs:KeepWindow:true
> > true and false only affect the behaviour on start up if the KeepWindow
> > option is not set in the user config. Once it is set, it is obeyed and
> > KJobViewer is correctly started.
> 
> No, this option points for what value of the configuration
> tdeprintrc:Jobs:KeepWindow the program must be started and for false
> KJobViewer is never started, but for KeepWindow false the program exits just
> the window closed what is also the starting to the system tray. And it made
> specially for the KeepWindow original behaviour!
> 

Here's what we want: By default, we want kjobviewer to be shown only if there are any queued jobs => so KeepWindow is off by default and kjobviewer isn't started automatically. If the current state in the final R14.0.8 is not like that, then it's sad, but it doesn't change before R14.0.9~pre.

> > The reason why it is false and not true, is that most of the users don't
> > need/use KJobViewer, so the default is to not autostart it. 
> 
> Really, most users the DE have not any printer? I think most users of any DE
> have one at the least printer but very few users have neither, to make such
> behaviour by default. Anyway the few users may uncheck the flag KeepWindow
> and will not see it more but what must do me now? Other point, it is you
> need just to set KeepWindow to false directly, but do not change the
> autostart control file!
> 

Many users certainly have a printer, but few who want a kjobviewer to take up space in the tray if it doesn't provide useful information => if there are no jobs in the queue. Therefore, we want KeepWindow off by default, as well as not start kjobviewer automatically.

> > If this is not what you see, write here and we can check together
> 
> This is not, I use this patch 4 years in very different environments and
> printers number!

The fact that a "hack works" does not mean that it is a "correct fix". Together with Michele, we have noticed other cases where an crash occurred simply because the cause of problem is not fixed, but only in some cases masked by such a hack.

That is why I want to find the cause instead of adding more and more hacks to all the places where the consequences of an unresolved cause occurs.

(In reply to Slávek Banko from comment #39)
> (In reply to Roman Savochenko from comment #38)
> > (In reply to Michele Calgaro from comment #37)
> > > > Yes, but it is wrong in the line:
> > > >  X-TDE-autostart-condition=tdeprintrc:Jobs:KeepWindow:false
> > > > Since it must be auto-started at keeping the window! So the correct line is:
> > > >  X-TDE-autostart-condition=tdeprintrc:Jobs:KeepWindow:true
> > > true and false only affect the behaviour on start up if the KeepWindow
> > > option is not set in the user config. Once it is set, it is obeyed and
> > > KJobViewer is correctly started.
> > 
> > No, this option points for what value of the configuration
> > tdeprintrc:Jobs:KeepWindow the program must be started and for false
> > KJobViewer is never started, but for KeepWindow false the program exits just
> > the window closed what is also the starting to the system tray. And it made
> > specially for the KeepWindow original behaviour!
> > 
> 
> Here's what we want: By default, we want kjobviewer to be shown only if
> there are any queued jobs => so KeepWindow is off by default and kjobviewer
> isn't started automatically. 

So, you will see kjobviewer only at print commands from TDE applications but not from other ones, for what I done this patch primarily!

And why have you at all used the patch, the original behaviour is the same?!

Created attachment 2952 [details]
kjobviewer 18.04.2020.kcrash

(In reply to Slávek Banko from comment #39)
> That is why I want to find the cause instead of adding more and more hacks
> to all the places where the consequences of an unresolved cause occurs.

OK, let's start the fresh crashes, as you wish!!!

And I am going to return to my "hacks"...

Hi Roman,

first of all I tested the functionality before committing your changes, so the current code was working already, except for forgetting to include the autostart file in the package.
FYI, the kjobviewer-autostart.desktop has made it into R14.0.8 since the packaging repo hadn't yet been tagged. Lucky :-)


> No, this option points for what value of the configuration tdeprintrc:Jobs:KeepWindow
> the program must be started and for false KJobViewer is never started

1) tdeprintrc:Jobs:KeepWindow is the option to decide whether to autostart or not. 
2) true or false is the default value to use if the KeepWindow option is not found in tdeprintrc, as I already written in my previous comment.
3) if the KeepWindow option is found in tdeprintrc, it is obeyed and KJobViewer started or not, depending on its value
4) due to a small bug, KJobviewer was not started automatically with "false" in "tdeprintrc:Jobs:KeepWindow:false" if the checkbox "Keep window permanent" was NEVER toggled in the KJobViewer GUI. In fact in my previous comment I pointed this out clearly:

"For you use case, you just need to run KJobViewer once, toggle the Keep Window checkbox off-on once (important !) and you should be ok."

Anyhow this small bug has now been resolved in master branch and will be backported to R14.0.9 soon. All you need to do is to make sure the checkbox is set. 
NOTE: in R14.0.8 make sure you toggle it once, so the KeepWindow setting is stored in tdeprintrc.

> Anyhow this small bug has now been resolved in master branch and will be backported to 
> R14.0.9 soon. 
This has now been backported to R14.0.9.
Only thing left to do here is looking at that "loadPrinters()" thing :-)

(In reply to Michele Calgaro from comment #43)
> This has now been backported to R14.0.9.
> Only thing left to do here is looking at that "loadPrinters()" thing :-)

So, I have seen the auto start file:
roman@debian10:~$ dpkg-query -l | grep tdepr
ii  tdeprint-trinity                      4:14.0.8-0debian10.0.0+1            amd64        print system for TDE
roman@debian10:~$ dpkg-query -L tdeprint-trinity | grep kjobviewer-autostart.desktop
/opt/trinity/share/autostart/kjobviewer-autostart.desktop

(In reply to Michele Calgaro from comment #42)
> first of all I tested the functionality before committing your changes, so the 

Really, and I use that and have not tested yet of course? :)

> current code was working already, except for forgetting to include the autostart file in the package.

So, let's trace this original installation, where:
roman@debian10:~$ cat /opt/trinity/share/autostart/kjobviewer-autostart.desktop | grep X-TDE-autostart-condition
X-TDE-autostart-condition=tdeprintrc:Jobs:KeepWindow:false

Steps:
* Initial start: KJobViewer is missed in the system tray just after start, with the keeping option checking.
* Start KJobViewer manually and uncheck the keeping option.
* Restarting the system: KJobViewer is missed in the system tray again.

So I repeat again, What must I now to do for get the KJobViewer in the system tray, for what I designed the patch primarily???
And why have you at all used the patch, since the original behaviour is the same?!

Hi Roman,
thanks for your feedback, appreciated :-)

> Start KJobViewer manually and uncheck the keeping option.
You have to uncheck and then check again. At the next login KJobViewer will start.

Hi Roman,
I have prepared a fix for the crash. It works fine here, KJobViewer does not crash anymore. Would you be able to test on your environment too?
Basically now KJobViewer gets notified when the printer list is modified.
See the patch here:

https://mirror.git.trinitydesktop.org/gitea/TDE/tdelibs/pulls/82

> Hi Roman, Would you be able to test on your environment too?
Reminder if you could test the patch below. This is more advanced than the original use of "loadPrinters()" and makes sure the menus are always sync (not just after the next polling time).

https://mirror.git.trinitydesktop.org/gitea/TDE/tdelibs/pulls/82

Thanks!!

(In reply to Michele Calgaro from comment #47)
> > Hi Roman, Would you be able to test on your environment too?
> Reminder if you could test the patch below. This is more advanced than the
> original use of "loadPrinters()" and makes sure the menus are always sync
> (not just after the next polling time).
Yes, that is right in the direct processing the printer changes.

I applied this patch in 05.05 and that yet works and have not crashed!

(In reply to Michele Calgaro from comment #45)
> Hi Roman,
> thanks for your feedback, appreciated :-)
> 
> > Start KJobViewer manually and uncheck the keeping option.
> You have to uncheck and then check again. At the next login KJobViewer will
> start.

So, you should disable this option default value directly in KJobViewer also!
--- a/tdeprint/management/kmjobviewer.cpp
+++ b/tdeprint/management/kmjobviewer.cpp
@@ -335,7 +335,7 @@

                TDEConfig *conf = KMFactory::self()->printConfig();
                conf->setGroup("Jobs");
-               m_stickybox->setChecked(conf->readBoolEntry("KeepWindow",true));
+               m_stickybox->setChecked(conf->readBoolEntry("KeepWindow",false));
                connect(m_stickybox, TQT_SIGNAL(toggled(bool)), TQT_SLOT(slotKeepWindowChange(bool)));
                statusbar->addWidget( m_stickybox, 1, false );
                statusbar->insertItem(" " + i18n("Max.: %1").arg(i18n("Unlimited"))+ " ", 0, 0, true);

Thanks for testing Roman! PR merged to master and backported to R14.0.9.

> So, you should disable this option default value directly in KJobViewer also!
Thanks for highlighting this. It had already been done in commit a57ae64c (R14.1) and 3505e323 (R14.0) although it didn't make it in R14.0.8.