From fe4156de433d22e8d98a714cd8f6743f4d5ba46f Mon Sep 17 00:00:00 2001
From: Jarek Kobus <jaroslaw.kobus@qt.io>
Date: Mon, 31 Jan 2022 14:53:41 +0100
Subject: [PATCH] SshDeviceProcess: Fix handling finished() signal

We can't disconnect all signals inside
SshDeviceProcessPrivate::setState() since it will also
disconnect all external slots from *this signals.
The original intention was to just disconnect from
finished() signal to *this slot.

Currently there are two connections to finished() signal:
one is internal and one external. In order to guarantee
that the internal one is always invoked before the external
one, we need to do the internal connection first - so we
move the connection to the constructor. In addition we
introduce the ignoreFinished flag. The true value means
the connection is disconnected. So, instead of connecting
and disconnecting dynamically we change the flag accordingly.

In case when handleThisProcessFinished() was called we
don't emit a finised() signal from handleProcessFinished()
since this signal is going to be delivered for all other
external slots anyway after handling of
handleThisProcessFinished() finishes.

Amends 9ec997b37654894b027cf4bbd98ca8bba47171b5

Change-Id: Iae12c10251de242391c03d185a0856f421b99fe4
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Christian Kandeler <christian.kandeler@qt.io>
---
 .../devicesupport/sshdeviceprocess.cpp        | 29 +++++++++++++++----
 .../devicesupport/sshdeviceprocess.h          |  4 ++-
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/src/plugins/projectexplorer/devicesupport/sshdeviceprocess.cpp b/src/plugins/projectexplorer/devicesupport/sshdeviceprocess.cpp
index 1386c7a630b..5d7d285e668 100644
--- a/src/plugins/projectexplorer/devicesupport/sshdeviceprocess.cpp
+++ b/src/plugins/projectexplorer/devicesupport/sshdeviceprocess.cpp
@@ -51,6 +51,7 @@ public:
     SshDeviceProcessPrivate(SshDeviceProcess *q) : q(q) {}
 
     SshDeviceProcess * const q;
+    bool ignoreFinished = true;
     QSsh::SshConnection *connection = nullptr;
     QSsh::SshRemoteProcessPtr remoteProcess;
     Runnable runnable;
@@ -73,6 +74,7 @@ SshDeviceProcess::SshDeviceProcess(const IDevice::ConstPtr &device, QObject *par
     : DeviceProcess(device, QtcProcess::TerminalOn, parent),
       d(std::make_unique<SshDeviceProcessPrivate>(this))
 {
+    connect(this, &QtcProcess::finished, this, &SshDeviceProcess::handleThisProcessFinished);
     connect(&d->killTimer, &QTimer::timeout, this, &SshDeviceProcess::handleKillOperationTimeout);
 }
 
@@ -184,8 +186,7 @@ void SshDeviceProcess::handleConnected()
     if (!display.isEmpty())
         d->remoteProcess->requestX11Forwarding(display);
     if (runInTerminal()) {
-        connect(this, &QtcProcess::finished,
-                this, [this] { handleProcessFinished(errorString()); });
+        d->ignoreFinished = false;
         setAbortOnMetaChars(false);
         setCommand(d->remoteProcess->fullLocalCommandLine(true));
         QtcProcess::start();
@@ -193,7 +194,7 @@ void SshDeviceProcess::handleConnected()
         connect(d->remoteProcess.get(), &QSsh::SshRemoteProcess::started,
                 this, &SshDeviceProcess::handleProcessStarted);
         connect(d->remoteProcess.get(), &QSsh::SshRemoteProcess::done,
-                this, &SshDeviceProcess::handleProcessFinished);
+                this, &SshDeviceProcess::handleRemoteProcessFinished);
         connect(d->remoteProcess.get(), &QSsh::SshRemoteProcess::readyReadStandardOutput,
                 this, &QtcProcess::readyReadStandardOutput);
         connect(d->remoteProcess.get(), &QSsh::SshRemoteProcess::readyReadStandardError,
@@ -236,13 +237,29 @@ void SshDeviceProcess::handleProcessStarted()
     emit started();
 }
 
-void SshDeviceProcess::handleProcessFinished(const QString &error)
+void SshDeviceProcess::handleThisProcessFinished()
+{
+    if (d->ignoreFinished)
+        return;
+    // Hack: we rely on fact that this slot was called before any other external slot connected
+    // to finished() signal. That's why we don't emit finished() signal from inside
+    // handleProcessFinished() since this signal will reach all other external slots anyway.
+    handleProcessFinished(QtcProcess::errorString(), false);
+}
+
+void SshDeviceProcess::handleRemoteProcessFinished(const QString &error)
+{
+    handleProcessFinished(error, true);
+}
+
+void SshDeviceProcess::handleProcessFinished(const QString &error, bool emitFinished)
 {
     d->errorMessage = error;
     if (d->killOperation && error.isEmpty())
         d->errorMessage = tr("The process was ended forcefully.");
     d->setState(SshDeviceProcessPrivate::Inactive);
-    emit finished();
+    if (emitFinished)
+        emit finished();
 }
 
 void SshDeviceProcess::handleKillOperationFinished(const QString &errorMessage)
@@ -328,7 +345,7 @@ void SshDeviceProcess::SshDeviceProcessPrivate::setState(SshDeviceProcess::SshDe
             QMetaObject::invokeMethod(q, &QtcProcess::stopProcess, Qt::QueuedConnection);
     }
     killTimer.stop();
-    q->disconnect();
+    ignoreFinished = true;
     if (remoteProcess)
         remoteProcess->disconnect(q);
     if (connection) {
diff --git a/src/plugins/projectexplorer/devicesupport/sshdeviceprocess.h b/src/plugins/projectexplorer/devicesupport/sshdeviceprocess.h
index 0980d530bcf..11b5a92bc12 100644
--- a/src/plugins/projectexplorer/devicesupport/sshdeviceprocess.h
+++ b/src/plugins/projectexplorer/devicesupport/sshdeviceprocess.h
@@ -60,7 +60,9 @@ private:
     void handleConnectionError();
     void handleDisconnected();
     void handleProcessStarted();
-    void handleProcessFinished(const QString &error);
+    void handleThisProcessFinished();
+    void handleRemoteProcessFinished(const QString &error);
+    void handleProcessFinished(const QString &error, bool emitFinished);
     void handleKillOperationFinished(const QString &errorMessage);
     void handleKillOperationTimeout();