Bug 367746 - konsole-16.08.0 closes active screen (or tmux) window on exit
Summary: konsole-16.08.0 closes active screen (or tmux) window on exit
Status: RESOLVED FIXED
Alias: None
Product: konsole
Classification: Applications
Component: general (show other bugs)
Version: master
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Martin Sandsmark
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-24 06:47 UTC by Markus Trippelsdorf
Modified: 2016-09-13 10:15 UTC (History)
2 users (show)

See Also:
Latest Commit:
Version Fixed In:
Sentry Crash Report:


Attachments
Only send EOF if no foreground process (910 bytes, patch)
2016-08-27 20:40 UTC, Martin Sandsmark
Details
Use proper methods to check for active PID (1.25 KB, patch)
2016-08-28 12:42 UTC, Martin Sandsmark
Details
Patch #3 (2.17 KB, patch)
2016-08-28 13:29 UTC, Martin Sandsmark
Details
Patch to only send EOF to known shells (963 bytes, patch)
2016-08-28 17:31 UTC, Martin Sandsmark
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Trippelsdorf 2016-08-24 06:47:17 UTC
konsole-16.08.0 closes the currently active screen (or tmux) window on exit.


Reproducible: Always

Steps to Reproduce:
1. Start screen or tmux and create several windows
2. Close konsole
3. re-attach the screen or tmux session

Actual Results:  
The last active window is now gone.

Expected Results:  
The last active window should still be there.

This is a regression from the previous version.
Comment 1 Markus Trippelsdorf 2016-08-24 15:37:43 UTC
Probably started with commit 2f1c8cf397be6c91f and commit c6c09ce7a38fccd.
Please revert.
Thanks.
Comment 2 Martin Sandsmark 2016-08-27 20:29:44 UTC
The alternative is data loss because the active process terminates its children, as explained in https://bugs.kde.org/show_bug.cgi?id=185140

As the bash maintainer explained EOF is the correct way for a terminal emulator to end a session.

If you have a better solution that doesn't lead to data loss I'm all ears.
Comment 3 Martin Sandsmark 2016-08-27 20:40:25 UTC
Created attachment 100811 [details]
Only send EOF if no foreground process

Can you test this patch and see if it helps for your usecase?
Comment 4 Markus Trippelsdorf 2016-08-28 07:29:55 UTC
(In reply to Martin Sandsmark from comment #2)
> The alternative is data loss because the active process terminates its
> children, as explained in https://bugs.kde.org/show_bug.cgi?id=185140
> 
> As the bash maintainer explained EOF is the correct way for a terminal
> emulator to end a session.
> 
> If you have a better solution that doesn't lead to data loss I'm all ears.

One could argue that this isn't the job of an terminal emulator.
The user can specify the desired behavior in his shell config. So there
really is no need to change the way Konsole behaves.

Anyway your patch seems to fix the issue for me.
Thanks.
Comment 5 Markus Trippelsdorf 2016-08-28 07:47:00 UTC
(In reply to Markus Trippelsdorf from comment #4)
> (In reply to Martin Sandsmark from comment #2)
> > The alternative is data loss because the active process terminates its
> > children, as explained in https://bugs.kde.org/show_bug.cgi?id=185140
> > 
> > As the bash maintainer explained EOF is the correct way for a terminal
> > emulator to end a session.
> > 
> > If you have a better solution that doesn't lead to data loss I'm all ears.
> 
> One could argue that this isn't the job of an terminal emulator.
> The user can specify the desired behavior in his shell config. So there
> really is no need to change the way Konsole behaves.
> 
> Anyway your patch seems to fix the issue for me.
> Thanks.

No, I spoke too soon. The issue still happens (but not always) with your patch.
Comment 6 Martin Sandsmark 2016-08-28 10:10:43 UTC
Ok, then I'll have to track the shell process in some other way, probably by PID. The idea is to only send EOF if the only process running is the initially launched shell, not if e. g. screen or top is running. screen broke it somewhat because I just tracked the current process by name, and obviously bash is the active process in the screen.

I think that should be a good solution for everyone.
Comment 7 Markus Trippelsdorf 2016-08-28 11:47:20 UTC
I'm not sure how this is supposed to work.

After boot I have, e.g.:  
  |-login,221 -f       
  |   `-zsh,227
  |       `-startx,228 /usr/bin/startx
  |           `-xinit,244 /home/markus/.xinitrc -- /etc/X11/xinit/xserverrc :0 -auth /home/markus/.serverauth.228
  |               |-X,245 -nolisten tcp :0 -auth /home/markus/.serverauth.228
  |               |  `-{radeon_cs:0},248
  |               `-xmonad-x86_64-l,250
  |                   |-konsole,278 -e /home/markus/start_tmux
  |                   |   |-start_tmux,300 /home/markus/start_tmux
  |                   |   |   `-tmux,302 attach-session
  |                   |   |-{QDBusConnection},293
  |                   |   |-{QXcbEventReader},292
  |                   |   `-{radeon_cs:0},299

After closing and re-attaching this gets:
  ├─login,221 -f       
  │   └─zsh,227
  │       └─startx,228 /usr/bin/startx
  │           └─xinit,244 /home/markus/.xinitrc -- /etc/X11/xinit/xserverrc :0 -auth /home/markus/.serverauth.228
  │               ├─X,245 -nolisten tcp :0 -auth /home/markus/.serverauth.228
  │               │  └─{radeon_cs:0},248
  │               └─xmonad-x86_64-l,250
  │                   ├─konsole,435
  │                   │   ├─zsh,440
  │                   │   │   └─tmux,445 attach-session
  │                   │   ├─zsh,505
  │                   │   │   └─ssh,510 trippels@gcc112.fsffrance.org
  │                   │   ├─{QDBusConnection},437
  │                   │   ├─{QXcbEventReader},436
  │                   │   └─{radeon_cs:0},439
  │                   ├─unclutter,280 -noevents -idle 1
  │                   ├─xmobar,282
  │                   └─xscreensaver,274

Where "ssh,510 trippels@gcc112.fsffrance.org" is a connection to a remote tmux session
in a new konsole window.

"tmux,445 attach-session" is the local tmux session:

  ├─tmux,304 attach-session
  │   ├─mutt,316
  │   ├─ncmpcpp,314
  │   ├─sh,309 /home/markus/multitail
  │   │   └─sudo,374 multitail --no-mark-change -M 0 -csn /var/log/messages -cS kernel /var/log/kern.log
  │   │       └─multitail,375 --no-mark-change -M 0 -csn /var/log/messages -cS kernel /var/log/kern.log
  │   │           ├─tail,376 --follow=name -n 157 /var/log/messages
  │   │           └─tail,377 --follow=name -n 50 /var/log/kern.log
  │   ├─sudo,305 su
  │   │   └─su,312
  │   │       └─zsh,332
  │   ├─sudo,307 su
  │   │   └─su,313
  │   │       └─zsh,333
  │   ├─zsh,318
  │   ├─zsh,320
  │   ├─zsh,324
  │   ├─zsh,326
  │   └─zsh,329
  │       └─pstree,1325 -apl
Comment 8 Martin Sandsmark 2016-08-28 12:42:28 UTC
Created attachment 100817 [details]
Use proper methods to check for active PID

This works in my testing.
Comment 9 Markus Trippelsdorf 2016-08-28 13:02:52 UTC
(In reply to Martin Sandsmark from comment #8)
> Created attachment 100817 [details]
> Use proper methods to check for active PID
> 
> This works in my testing.

It crashes when re-attaching a tmux session:

Thread 1 "konsole" received signal SIGSEGV, Segmentation fault.
Konsole::ProcessInfo::name (this=this@entry=0x0, ok=ok@entry=0x7fffffffd9af) at /var/tmp/konsole/src/ProcessInfo.cpp:231
231         *ok = _fields.testFlag(NAME);
(gdb) bt
#0  Konsole::ProcessInfo::name (this=this@entry=0x0, ok=ok@entry=0x7fffffffd9af) at /var/tmp/konsole/src/ProcessInfo.cpp:231
#1  0x00007ffff7f04b5d in Konsole::Session::getDynamicTitle (this=0x4b3290) at /var/tmp/konsole/src/Session.cpp:1058
#2  0x00007ffff7f11829 in Konsole::SessionController::snapshot (this=0x660f30) at /var/tmp/konsole/src/SessionController.cpp:253
#3  0x00007ffff7f19d1d in QtPrivate::FunctorCall<QtPrivate::IndexesList<>, QtPrivate::List<>, void, void (Konsole::SessionController::*)()>::call(void (Konsole::SessionController::*)(), Konsole::SessionController*, void**) (arg=<optimized out>, o=<optimized out>, f=<optimized out>) at /usr/include/qt5/QtCore/qobjectdefs_impl.h:507
#4  QtPrivate::FunctionPointer<void (Konsole::SessionController::*)()>::call<QtPrivate::List<>, void>(void (Konsole::SessionController::*)(), Konsole::SessionController*, void**)
    (arg=<optimized out>, o=<optimized out>, f=<optimized out>) at /usr/include/qt5/QtCore/qobjectdefs_impl.h:526
#5  QtPrivate::QSlotObject<void (Konsole::SessionController::*)(), QtPrivate::List<>, void>::impl(int, QtPrivate::QSlotObjectBase*, QObject*, void**, bool*) (
    which=<optimized out>, this_=<optimized out>, r=<optimized out>, a=<optimized out>, ret=<optimized out>) at /usr/include/qt5/QtCore/qobject_impl.h:149
#6  0x00007ffff61fbced in QMetaObject::activate(QObject*, int, int, void**) () from /usr/lib/libQt5Core.so.5
#7  0x00007ffff6208728 in QTimer::timerEvent(QTimerEvent*) () from /usr/lib/libQt5Core.so.5
#8  0x00007ffff61fc8cb in QObject::event(QEvent*) () from /usr/lib/libQt5Core.so.5
#9  0x00007ffff6aefe9c in QApplicationPrivate::notify_helper(QObject*, QEvent*) () from /usr/lib/libQt5Widgets.so.5
#10 0x00007ffff6af7174 in QApplication::notify(QObject*, QEvent*) () from /usr/lib/libQt5Widgets.so.5
#11 0x00007ffff61d0000 in QCoreApplication::notifyInternal2(QObject*, QEvent*) () from /usr/lib/libQt5Core.so.5
#12 0x00007ffff6222f0e in QTimerInfoList::activateTimers() () from /usr/lib/libQt5Core.so.5
#13 0x00007ffff6223481 in timerSourceDispatch(_GSource*, int (*)(void*), void*) () from /usr/lib/libQt5Core.so.5
#14 0x00007ffff43b9dce in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0
#15 0x00007ffff43ba038 in g_main_context_iterate.isra () from /usr/lib/libglib-2.0.so.0
#16 0x00007ffff43ba0ec in g_main_context_iteration () from /usr/lib/libglib-2.0.so.0
#17 0x00007ffff6223fdf in QEventDispatcherGlib::processEvents(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/libQt5Core.so.5
#18 0x00007ffff61ce44a in QEventLoop::exec(QFlags<QEventLoop::ProcessEventsFlag>) () from /usr/lib/libQt5Core.so.5
#19 0x00007ffff61d67ed in QCoreApplication::exec() () from /usr/lib/libQt5Core.so.5
#20 0x00007ffff7fd2a9d in kdemain (argc=<optimized out>, argv=<optimized out>) at /var/tmp/konsole/src/main.cpp:174
#21 0x0000000000400a9d in main (argc=<optimized out>, argv=<optimized out>) at /var/tmp/konsole/build/src/konsole_dummy.cpp:3
Comment 10 Martin Sandsmark 2016-08-28 13:29:41 UTC
Created attachment 100818 [details]
Patch #3

Try this. There were some implicit dependencies on side-effects...
Comment 11 Markus Trippelsdorf 2016-08-28 13:54:13 UTC
Seems to work now. Thanks.
BTW your:
commit 04dce67996a9318494c79c2617e3c88620e8e19e
Author: Martin T. H. Sandsmark <martin.sandsmark@kde.org>
Date:   Sat Aug 27 14:38:26 2016 +0200

    Make the URL hint keyboard modifiers configurable
    
    Instead of hardcoding CTRL which was a bit annoying add checkboxes to
    the profile edit dialog to let the user select which keys to use.
    
    REVIEW: 128778

Creates strange visible distortions. When I type something in my shell, completely unrelated glyphs from older output get changed in strange ways. And konsole consumes a lot of CPU and general response time is unbearable.
Comment 12 Markus Trippelsdorf 2016-08-28 14:02:20 UTC
And it still doesn't fully work unfortunately.
When I start tmux for the first time, then close Konsole, then re-attach:
the last active shell window is now gone.
Comment 13 Markus Trippelsdorf 2016-08-28 14:12:54 UTC
I have "konsole -e "tmux attach-session" > /dev/null 2>&1 &" in my .xinitrc,
so there is no underlying shell in this case.
Comment 14 Martin Sandsmark 2016-08-28 17:18:13 UTC
Ah, the -e messes it up indeed.

Ok, I'll push the first patch anyways, it makes things a bit tidier, and then I'll just check the name of the base process against a list of known shells, I guess. Unless I come up with a better idea.
Comment 15 Martin Sandsmark 2016-08-28 17:18:21 UTC
Git commit 79ca76a94696eda408927b122b0b531676d4ba93 by Martin T. H. Sandsmark.
Committed on 28/08/2016 at 17:12.
Pushed by sandsmark into branch 'master'.

Fix checking of foreground process

The old method of checking it has unnecessary overhead (doing a full
process info update), and wasn't very reliable.

Instead just get the original shell PID from QProcess (via KProcess),
and the foreground process from the PTY.
REVIEW: 128789

M  +2    -1    src/MainWindow.cpp
M  +5    -4    src/Session.cpp

http://commits.kde.org/konsole/79ca76a94696eda408927b122b0b531676d4ba93
Comment 16 Martin Sandsmark 2016-08-28 17:31:04 UTC
Created attachment 100830 [details]
Patch to only send EOF to known shells

This should fix it once and for all.
Comment 17 Markus Trippelsdorf 2016-08-28 17:49:10 UTC
(In reply to Martin Sandsmark from comment #16)
> Created attachment 100830 [details]
> Patch to only send EOF to known shells
> 
> This should fix it once and for all.

Indeed it does. Thank you for your patience.
Comment 18 Martin Sandsmark 2016-08-28 19:51:43 UTC
> Indeed it does. Thank you for your patience.

Thank you for your patience as well. :-)

https://git.reviewboard.kde.org/r/128791/
Comment 19 Martin Sandsmark 2016-08-28 22:13:17 UTC
Git commit ce4ab922496cb9e133f96db78c5b67bc01429512 by Martin T. H. Sandsmark.
Committed on 28/08/2016 at 22:12.
Pushed by sandsmark into branch 'master'.

Only send EOF to known shells

The only processes that are "safe" or require us to send an EOF to
terminate cleanly are shells, so verify that people didn't run with -e
tmux or similar.

REVIEW: 128791

M  +4    -2    src/Session.cpp

http://commits.kde.org/konsole/ce4ab922496cb9e133f96db78c5b67bc01429512
Comment 20 Kurt Hindenburg 2016-08-31 12:49:01 UTC
Git commit 17a3ee5dfa1fc99776a015b31e5862ce82ab1a56 by Kurt Hindenburg, on behalf of Martin T. H. Sandsmark.
Committed on 31/08/2016 at 12:37.
Pushed by hindenburg into branch 'Applications/16.08'.

Fix checking of foreground process

The old method of checking it has unnecessary overhead (doing a full
process info update), and wasn't very reliable.

Instead just get the original shell PID from QProcess (via KProcess),
and the foreground process from the PTY.
REVIEW: 128789
(cherry picked from commit 79ca76a94696eda408927b122b0b531676d4ba93)

M  +2    -1    src/MainWindow.cpp
M  +5    -4    src/Session.cpp

http://commits.kde.org/konsole/17a3ee5dfa1fc99776a015b31e5862ce82ab1a56
Comment 21 Kurt Hindenburg 2016-08-31 12:56:50 UTC
Git commit 7cecfc078e49e1a8654a759481be31e335be2b09 by Kurt Hindenburg, on behalf of Martin T. H. Sandsmark.
Committed on 31/08/2016 at 12:38.
Pushed by hindenburg into branch 'Applications/16.08'.

Only send EOF to known shells

The only processes that are "safe" or require us to send an EOF to
terminate cleanly are shells, so verify that people didn't run with -e
tmux or similar.

REVIEW: 128791
(cherry picked from commit ce4ab922496cb9e133f96db78c5b67bc01429512)

M  +4    -2    src/Session.cpp

http://commits.kde.org/konsole/7cecfc078e49e1a8654a759481be31e335be2b09