Bug 288200 - [PATCH] "konsole --nofork" crashes when started not from terminal
Summary: [PATCH] "konsole --nofork" crashes when started not from terminal
Status: RESOLVED FIXED
Alias: None
Product: konsole
Classification: Applications
Component: single-process (show other bugs)
Version: 2.6.5
Platform: Debian unstable Linux
: NOR crash
Target Milestone: ---
Assignee: Konsole Developer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-12-04 14:00 UTC by Askar Safin
Modified: 2012-08-18 09:29 UTC (History)
5 users (show)

See Also:
Latest Commit:
Version Fixed In: 4.9.0


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Askar Safin 2011-12-04 14:00:39 UTC
Version:           4.6 (using KDE 4.6.5) 
OS:                Linux

"konsole --nofork" crashes when started not from terminal (if another konsole runs) or with redirected stdout (if another konsole runs).

Why? Because in konsole/src/main.cpp:79-87 in following lines konsole makes wrong decision that forcing new process is not needed:

bool forceNewProcess()
{
    // when starting Konsole from a terminal, a new process must be used 
    // so that the current environment is propagated into the shells of the new
    // Konsole and any debug output or warnings from Konsole are written to
    // the current terminal
    KCmdLineArgs* args = KCmdLineArgs::parsedArgs();
    return isatty(1) && !args->isSet("new-tab");
}

Konsole runned not from terminal, so isatty(1) will return "false". So, "forceNewProcess" will return "false". But is not logical. If I put option "--nofork" I want konsole in this process! I don't want konsole to open new window in existing process. I want to have ability to wait konsole to terminate. If I call "konsole --nofork" from script I want the command to stop after konsole window closes.

Then function "kdemain" (konsole/src/main:65-77) doesn't put "KUniqueApplication::NonUniqueInstance" to startFlags and runs "KUniqueApplication::start":

    if (forceNewProcess())
        startFlags = KUniqueApplication::NonUniqueInstance;

    // create a new application instance if there are no running Konsole instances,
    // otherwise inform the existing Konsole process and exit
    if ( !KUniqueApplication::start(startFlags) )
    {
        exit(0);
    }
    
    Application app;
    restoreSession(app);
    return app.exec();

Then we fall to code from package "kdelibs" to function "KUniqueApplication::start" (kdeui/kernel/kuniqueapplication.cpp:140-164):

  if (Private::s_nofork)
  {

#if defined(Q_OS_DARWIN) || defined (Q_OS_MAC)
     mac_initialize_dbus();
#endif

     QDBusConnectionInterface* dbusService = tryToInitDBusConnection();

     QString pid = QString::number(getpid());
     if (forceNewProcess)
        appName = appName + '-' + pid;

     // Check to make sure that we're actually able to register with the D-Bus session
     // server.
     bool registered = dbusService->registerService(appName) == QDBusConnectionInterface::ServiceRegistered;
     if (!registered)
     {
        kError() << "KUniqueApplication: Can't setup D-Bus service. Probably already running."
                 << endl;
#if defined(Q_WS_WIN) && !defined(_WIN32_WCE)
        KApplication_activateWindowForProcess(KCmdLineArgs::aboutData()->appName());
#endif
        ::exit(255);
     }

We use "--nofork" option, so "Private::s_nofork" is true, we didn't put "NonUniqueInstance" flag, so "forceNewProcess" is false. So, "pid" doesn't add to "appName", so service registration failed (another konsole runs). So, we get message "KUniqueApplication: Can't setup D-Bus service. Probably already running." and konsole crashes.

Why is it so important? Because I want to start "konsole --nofork" as daemon, from some shell script, from application launcher (Alt-F2) or from some another strange environment and not only from terminal! And I want to wait ending of this command.

PATCH to fix this problem:

--- kde4libs-4.6.5-orig/kdeui/kernel/kuniqueapplication.cpp     2011-06-30 21:34:17.000000000 +0000
+++ kde4libs-4.6.5/kdeui/kernel/kuniqueapplication.cpp          2011-12-04 13:54:46.000000000 +0000
@@ -135,7 +135,7 @@
         appName.prepend(s);
      }
 
-  bool forceNewProcess = Private::s_multipleInstances || flags & NonUniqueInstance;
+  bool forceNewProcess = Private::s_multipleInstances || flags & NonUniqueInstance || Private::s_nofork;
 
   if (Private::s_nofork)
   {



Reproducible: Always

Steps to Reproduce:
Run command "konsole --nofork > /dev/null" (if another konsole runs) or press Alt-F2 (application launcher will appear) and type "konsole --nofork" (if another konsole runs)

Actual Results:  
konsole does not appear. Following message goes to stderr:
unnamed app(23374): KUniqueApplication: Can't setup D-Bus service. Probably already running.

Expected Results:  
konsole appear
Comment 1 Frank Reininghaus 2011-12-04 17:46:11 UTC
Thanks for the bug report! Could you post your patch at https://git.reviewboard.kde.org/ in order to make sure that the people who are familiar with the code see it? Patches get lost very easily here on bugs.kde.org :-(
Comment 2 Askar Safin 2011-12-05 04:19:52 UTC
Thanks, Frank. I posted to https://git.reviewboard.kde.org/r/103333
Comment 3 Jekyll Wu 2011-12-05 19:11:14 UTC
FYI, the way konsole calls isatty() will change a bit in the upcoming KDE SC 4.8. It will check STDIN, instead of STDOUT. I made that change a few weeks ago just for the 'konsole --nofork > /dev/null' case.
Comment 4 Askar Safin 2011-12-05 21:01:04 UTC
Jekyll Wu, WHY????????????? isatty(0) is bad way to check status of process. isatty(1) is also bad way. open("/dev/tty", O_RDONLY) is the only right way. If process has controlling terminal, it is the daemon. If has not, it is not.
Comment 5 Askar Safin 2011-12-11 18:12:32 UTC
It doesn't matter how to check is this process daemon. isatty(1), isatty(0) or something else. I want konsole to work in following example:

setsid konsole --nofork < /dev/null > /dev/null 2> /dev/null &

There is only one way to fix problem: applying patch.
Please, APPLY IT!
Comment 6 Jekyll Wu 2012-01-12 19:31:45 UTC
Git commit 595ccda304f4a1c6a54039ce2f5d2be0f1ba3c41 by Jekyll Wu.
Committed on 12/01/2012 at 20:18.
Pushed by jekyllwu into branch 'master'.

When --nofork option is specified, Konsole should alwasy use new process

The intention of using --nofork with Konsole is to start Konsole in
foreground and wait for it to finish, so the only logical behavior is
using new process instead of reusing existing process.

M  +11   -3    src/main.cpp

http://commits.kde.org/konsole/595ccda304f4a1c6a54039ce2f5d2be0f1ba3c41
Comment 7 Jekyll Wu 2012-01-12 19:39:42 UTC
Hi Askar, I have made some change to the related code. Now Konsole works for me with your given example :

    setsid konsole --nofork < /dev/null > /dev/null 2> /dev/null &

Please try the latest code and check whether Konsole behaves as your expectation in those strange environments.
Comment 8 Askar Safin 2012-01-13 12:51:01 UTC
Thank you, Jekyll Wu for fixing bug and for open(/dev/tty).
I hardly tested konsole and all is working well. But now suddenly --new-tab don't work!

Reproducible: Always

Steps to Reproduce:
Close all konsoles, then run konsole from lastest sources (for example, from xterm), then type to it "konsole --new-tab" or "konsole --nofork --new-tab"

Actual Results:  
New konsole window appear

Expected Results:  
New konsole tab must appear
Comment 9 Jekyll Wu 2012-01-13 13:58:09 UTC
(In reply to comment #8)

I just tried the code with or without that commit. The results are the same as you have described. 

However, I'm not sure that can be called as bug. The intention of --new-tab option is try to reuse existing konsole process (if POSSIBLE) and create new tab in that process. If such konsole process does not exist, new konsole process will be used.

Although how KUniqueApplication works is still like black magic to me,  I guess any KUniqueApplication started with the NonUniqueInstance flag will lose the possibility of being reused. And konsole started from xterm is a good example.

Personally, I think the side effect of konsole being a KUniqueApplication is really confusing. I would say konsole abuses KuniqueApplication for performance and some nice features(including this --new-tab option). But a terminal emulator is not supposed to be unique process. And I think the primary goal of   KUniqueApplication is ensuing uniqueness, instead of being reused to save resources.

Anyhow, the situation is unlikely to change in the near future. I will try to add some explanation into the handbook for those strange bahaviors due to KUniqueApplication.
Comment 10 Askar Safin 2012-01-13 14:04:40 UTC
I mean following steps:

1. Close all konsoles
2. Open xterm
3. Type to xterm "/path/to/lastest/konsole"
4. Type to new konsole window "/path/to/lastest/konsole --new-tab"

But no following:


1. Close all konsoles
2. Open xterm
4. Type to xterm "/path/to/lastest/konsole --new-tab"
Comment 11 Askar Safin 2012-01-13 14:05:31 UTC
So, konsole exists and ".../konsole --new-tab" must reuse process
Comment 12 Jekyll Wu 2012-01-13 14:19:19 UTC
(In reply to comment #10)

I see your point. I didn't misunderstand the steps :)

(In reply to comment #11)
> So, konsole exists and ".../konsole --new-tab" must reuse process

Yes, but ONLY IF the existing konsole process is reusable. 

In step 3 of comment 10, that started konsole process is NOT reusable because NonUniqueInstance is used. The basic problem is that konsole process uses org.kde.konsole-<pid> instead of org.kde.konsole as its dbus address.

Of course, that is just my understanding. I will check again whether that problem is really one regression caused by that commit.
Comment 13 Askar Safin 2012-01-13 14:27:42 UTC
Sorry for comment 10 :)
I just tested old konsole version. It has same behavior. So lastest commit doesn't matter for it.
I agree, this behavior is very strange, but likely it is not bug.
Comment 14 Jekyll Wu 2012-01-13 14:42:37 UTC
Thanks for your feedback.

So, could this report, and possibly bug 279915,  be closed now? If so, please also close your review request as discarded.
Comment 15 Askar Safin 2012-01-13 14:55:39 UTC
Yes, this bug FIXED. Thank you. I'm testing bug 279915 now