QtcProcess: Fix terminate() for process launcher

ProcessLauncherImpl::terminate() was behaving the same as
ProcessLauncherImpl::kill() in case of process launcher
implementation - in both cases the launcher
immediately returned confirmation about receiving the
close request and was putting the corresponding
process into its reaper. However, the issue with this approach
is that we can't receive anymore any potential ready
read signal from the process being terminated after
a call to terminate().

The fix is to diverge the behavior of terminate()
and kill() of ProcessLauncherImpl. The behavior of kill()
remains the same, while terminate() just instructs the
process launcher to start termination without putting
the process into the reaper, yet.

Add a test that checks for any possible zombies.

We run the RecursiveBlockingProcess recursively. The most
nested process blocks. After a short wait we terminate
the outermost process. With this test we are trying to see
if terminating the middle process terminates also its
children and doesn't leave zombies.

Before we execute the test (and also by the end of this test)
we call Singleton::deleteAll() in order to ensure that reaping
of previously running processes has finished. We ensure the
number of running processtestapps is zero. Later, we ensure that
the leaf process was already started and the number of running
processtestapps equals the depth of recursion.

Fix apparent bug in getLocalProcessesUsingProc().

Change-Id: I7e2bc46ad5ca22f26620da86fbaf0fa00a7db3c3
Reviewed-by: hjk <hjk@qt.io>
Reviewed-by: <github-actions-qt-creator@cristianadam.eu>
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>

7 files changed, 66 insertions, 33 deletions

diff --git a/src/libs/utils/launcherpackets.cpp b/src/libs/utils/launcherpackets.cpp
index d9a5d194c1..07a8b41d1b 100644
--- a/src/libs/utils/launcherpackets.cpp
+++ b/src/libs/utils/launcherpackets.cpp
@@ -114,12 +114,14 @@ StopProcessPacket::StopProcessPacket(quintptr token)
 
 void StopProcessPacket::doSerialize(QDataStream &stream) const
 {
-    Q_UNUSED(stream);
+    stream << int(signalType);
 }
 
 void StopProcessPacket::doDeserialize(QDataStream &stream)
 {
-    Q_UNUSED(stream);
+    int sig;
+    stream >> sig;
+    signalType = SignalType(sig);
 }
 
 void WritePacket::doSerialize(QDataStream &stream) const
diff --git a/src/libs/utils/launcherpackets.h b/src/libs/utils/launcherpackets.h
index 1fed478d6d..0989ff791c 100644
--- a/src/libs/utils/launcherpackets.h
+++ b/src/libs/utils/launcherpackets.h
@@ -142,6 +142,13 @@ class StopProcessPacket : public LauncherPacket
 public:
     StopProcessPacket(quintptr token);
 
+    enum class SignalType {
+        Kill,
+        Terminate
+    };
+
+    SignalType signalType = SignalType::Kill;
+
 private:
     void doSerialize(QDataStream &stream) const override;
     void doDeserialize(QDataStream &stream) override;
diff --git a/src/libs/utils/launchersocket.cpp b/src/libs/utils/launchersocket.cpp
index e6dd83d8bd..1a9e82b607 100644
--- a/src/libs/utils/launchersocket.cpp
+++ b/src/libs/utils/launchersocket.cpp
@@ -308,25 +308,44 @@ QProcess::ProcessState CallerHandle::state() const
     return m_processState;
 }
 
-void CallerHandle::cancel()
+bool CallerHandle::isStartPacketAwaitingAndClear()
 {
-    QTC_ASSERT(isCalledFromCallersThread(), return);
-    switch (m_processState.exchange(QProcess::NotRunning)) {
-    case QProcess::NotRunning:
-        break;
-    case QProcess::Starting:
-        m_result.m_errorString = QCoreApplication::translate("Utils::LauncherHandle",
-                                 "Process was canceled before it was started.");
-        m_result.m_error = QProcess::FailedToStart;
-        if (LauncherInterface::isReady()) // TODO: race condition with m_processState???
-            sendPacket(StopProcessPacket(m_token));
-        else
-            emit errorOccurred(m_result.m_error);
-        break;
-    case QProcess::Running:
-        sendPacket(StopProcessPacket(m_token));
-        break;
+    QMutexLocker locker(&m_mutex);
+    const bool startPacketExisted = m_startPacket.get();
+    m_startPacket.reset();
+    return startPacketExisted;
+}
+
+void CallerHandle::sendStopPacket(StopProcessPacket::SignalType signalType)
+{
+    if (m_processState == QProcess::NotRunning)
+        return;
+
+    if (m_processState == QProcess::Running || !isStartPacketAwaitingAndClear()) {
+        StopProcessPacket packet(m_token);
+        packet.signalType = signalType;
+        sendPacket(packet);
+        return;
     }
+
+    m_processState.store(QProcess::NotRunning);
+    m_result.m_errorString = QCoreApplication::translate("Utils::LauncherHandle",
+                                                "Process was canceled before it was started.");
+    m_result.m_error = QProcess::FailedToStart;
+    emit errorOccurred(m_result.m_error);
+}
+
+
+void CallerHandle::terminate()
+{
+    QTC_ASSERT(isCalledFromCallersThread(), return);
+    sendStopPacket(StopProcessPacket::SignalType::Terminate);
+}
+
+void CallerHandle::kill()
+{
+    QTC_ASSERT(isCalledFromCallersThread(), return);
+    sendStopPacket(StopProcessPacket::SignalType::Kill);
 }
 
 QByteArray CallerHandle::readAllStandardOutput()
@@ -416,7 +435,7 @@ void CallerHandle::doStart()
     if (!m_startPacket)
         return;
     sendPacket(*m_startPacket);
-    m_startPacket.reset(nullptr);
+    m_startPacket.reset();
 }
 
 // Called from caller's or launcher's thread.
diff --git a/src/libs/utils/launchersocket.h b/src/libs/utils/launchersocket.h
index 4696c516fb..f4993886c2 100644
--- a/src/libs/utils/launchersocket.h
+++ b/src/libs/utils/launchersocket.h
@@ -90,7 +90,10 @@ public:
 
     // Called from caller's or launcher's thread.
     QProcess::ProcessState state() const;
-    void cancel();
+    bool isStartPacketAwaitingAndClear();
+    void sendStopPacket(StopProcessPacket::SignalType signalType);
+    void terminate();
+    void kill();
 
     QByteArray readAllStandardOutput();
     QByteArray readAllStandardError();
diff --git a/src/libs/utils/processinfo.cpp b/src/libs/utils/processinfo.cpp
index 67c6177efd..98776784dc 100644
--- a/src/libs/utils/processinfo.cpp
+++ b/src/libs/utils/processinfo.cpp
@@ -98,7 +98,7 @@ static QList<ProcessInfo> getLocalProcessesUsingProc()
 
         if (proc.executable.isEmpty()) {
             QFile statFile(root + QLatin1String("/stat"));
-            if (!statFile.open(QIODevice::ReadOnly)) {
+            if (statFile.open(QIODevice::ReadOnly)) {
                 const QStringList data = QString::fromLocal8Bit(statFile.readAll()).split(QLatin1Char(' '));
                 if (data.size() < 2)
                     continue;
diff --git a/src/libs/utils/qtcprocess.cpp b/src/libs/utils/qtcprocess.cpp
index b7499e1ca2..f85244b82a 100644
--- a/src/libs/utils/qtcprocess.cpp
+++ b/src/libs/utils/qtcprocess.cpp
@@ -422,7 +422,7 @@ public:
     }
     ~ProcessLauncherImpl() final
     {
-        cancel();
+        m_handle->kill();
         LauncherInterface::unregisterHandle(token());
         m_handle = nullptr;
     }
@@ -435,9 +435,9 @@ public:
         if (m_setup->m_useCtrlCStub) // bypass launcher and interrupt directly
             ProcessHelper::interruptPid(processId());
     }
-    void terminate() final { cancel(); } // TODO: what are differences among terminate, kill and close?
-    void kill() final { cancel(); } // TODO: see above
-    void close() final { cancel(); } // TODO: see above
+    void terminate() final { m_handle->terminate(); }
+    void kill() final { m_handle->kill(); }
+    void close() final { m_handle->kill(); } // TODO: is it more like terminate or kill?
     qint64 write(const QByteArray &data) final { return m_handle->write(data); }
 
     ProcessResultData resultData() const final { return m_handle->resultData(); };
@@ -456,8 +456,6 @@ private:
         m_handle->start(program, arguments);
     }
 
-    void cancel();
-
     quintptr token() const { return m_token; }
 
     const uint m_token = 0;
@@ -465,11 +463,6 @@ private:
     CallerHandle *m_handle = nullptr;
 };
 
-void ProcessLauncherImpl::cancel()
-{
-    m_handle->cancel();
-}
-
 static ProcessImpl defaultProcessImpl()
 {
     if (qEnvironmentVariableIsSet("QTC_USE_QPROCESS"))
diff --git a/src/tools/processlauncher/launchersockethandler.cpp b/src/tools/processlauncher/launchersockethandler.cpp
index ccf7fa0950..9b64314843 100644
--- a/src/tools/processlauncher/launchersockethandler.cpp
+++ b/src/tools/processlauncher/launchersockethandler.cpp
@@ -247,6 +247,15 @@ void LauncherSocketHandler::handleStopPacket()
         logDebug("Got stop request for unknown process");
         return;
     }
+    const auto packet = LauncherPacket::extractPacket<StopProcessPacket>(
+                m_packetParser.token(),
+                m_packetParser.packetData());
+
+    if (packet.signalType == StopProcessPacket::SignalType::Terminate) {
+        process->terminate();
+        return;
+    }
+
     if (process->state() == QProcess::NotRunning) {
         // This shouldn't happen, since as soon as process finishes or error occurrs
         // the process is being removed.