Sanitize error/finished handling in executor jobs and command executors.

The classes as they are now specify different signals for signaling
failure and finishing. Some of these error() signals are followed by a
finished() signal, others are not, which is just asking for trouble.
Under certain circumstances there can also be several error() signals
for one and the same process. In fact, there is almost certainly a race
condition there regarding executor job re-use.
This patch changes the design so that there is always exactly one
finished() signal that carries an error status. For some non-fatal
problems that higher-level code cannot sensibly handle, we now log a
warning instead of emitting an error signal.

Change-Id: I9e3df11564e7337ad766ca0d009303367d43c4ec
Reviewed-by: Joerg Bornemann <joerg.bornemann@digia.com>

7 files changed, 44 insertions, 75 deletions

diff --git a/src/lib/corelib/buildgraph/abstractcommandexecutor.h b/src/lib/corelib/buildgraph/abstractcommandexecutor.h
index 0abb5c137..a4f781d62 100644
--- a/src/lib/corelib/buildgraph/abstractcommandexecutor.h
+++ b/src/lib/corelib/buildgraph/abstractcommandexecutor.h
@@ -31,6 +31,7 @@
 #define QBS_ABSTRACTCOMMANDEXECUTOR_H
 
 #include <logging/logger.h>
+#include <tools/error.h>
 
 #include <QObject>
 
@@ -58,8 +59,7 @@ public slots:
 
 signals:
     void reportCommandDescription(const QString &highlight, const QString &message);
-    void error(const qbs::ErrorInfo &err);
-    void finished();
+    void finished(const qbs::ErrorInfo &err = ErrorInfo()); // !hasError() <=> command successful
 
 protected:
     const AbstractCommand *command() const { return m_command; }
diff --git a/src/lib/corelib/buildgraph/executor.cpp b/src/lib/corelib/buildgraph/executor.cpp
index f54034173..aab2c4220 100644
--- a/src/lib/corelib/buildgraph/executor.cpp
+++ b/src/lib/corelib/buildgraph/executor.cpp
@@ -641,9 +641,8 @@ void Executor::addExecutorJobs()
                 this, SIGNAL(reportCommandDescription(QString,QString)), Qt::QueuedConnection);
         connect(job, SIGNAL(reportProcessResult(qbs::ProcessResult)),
                 this, SIGNAL(reportProcessResult(qbs::ProcessResult)), Qt::QueuedConnection);
-        connect(job, SIGNAL(error(qbs::ErrorInfo)),
-                this, SLOT(onProcessError(qbs::ErrorInfo)), Qt::QueuedConnection);
-        connect(job, SIGNAL(success()), this, SLOT(onProcessSuccess()), Qt::QueuedConnection);
+        connect(job, SIGNAL(finished(qbs::ErrorInfo)),
+                this, SLOT(onJobFinished(qbs::ErrorInfo)), Qt::QueuedConnection);
     }
 }
 
@@ -805,9 +804,9 @@ void Executor::finishTransformer(const TransformerPtr &transformer)
         finishArtifact(artifact);
 }
 
-void Executor::onProcessError(const qbs::ErrorInfo &err)
+void Executor::onJobFinished(const qbs::ErrorInfo &err)
 {
-    try {
+    if (err.hasError()) {
         if (m_buildOptions.keepGoing()) {
             ErrorInfo fullWarning(err);
             fullWarning.prepend(Tr::tr("Ignoring the following errors on user request:"));
@@ -815,19 +814,13 @@ void Executor::onProcessError(const qbs::ErrorInfo &err)
         } else {
             m_error = err;
         }
-        ExecutorJob * const job = qobject_cast<ExecutorJob *>(sender());
-        finishJob(job, false);
-    } catch (const ErrorInfo &error) {
-        handleError(error);
     }
-}
 
-void Executor::onProcessSuccess()
-{
+    ExecutorJob * const job = qobject_cast<ExecutorJob *>(sender());
+    QBS_CHECK(job);
+
     try {
-        ExecutorJob *job = qobject_cast<ExecutorJob *>(sender());
-        QBS_CHECK(job);
-        finishJob(job, true);
+        finishJob(job, !err.hasError());
     } catch (const ErrorInfo &error) {
         handleError(error);
     }
diff --git a/src/lib/corelib/buildgraph/executor.h b/src/lib/corelib/buildgraph/executor.h
index 63dcd7dd2..ba5011ce0 100644
--- a/src/lib/corelib/buildgraph/executor.h
+++ b/src/lib/corelib/buildgraph/executor.h
@@ -78,8 +78,7 @@ signals:
     void finished();
 
 private slots:
-    void onProcessError(const qbs::ErrorInfo &err);
-    void onProcessSuccess();
+    void onJobFinished(const qbs::ErrorInfo &err);
     void finish();
 
 private:
diff --git a/src/lib/corelib/buildgraph/executorjob.cpp b/src/lib/corelib/buildgraph/executorjob.cpp
index fea181fa4..1ad82cdb5 100644
--- a/src/lib/corelib/buildgraph/executorjob.cpp
+++ b/src/lib/corelib/buildgraph/executorjob.cpp
@@ -52,14 +52,12 @@ ExecutorJob::ExecutorJob(const Logger &logger, QObject *parent)
             this, SIGNAL(reportCommandDescription(QString,QString)));
     connect(m_processCommandExecutor, SIGNAL(reportProcessResult(qbs::ProcessResult)),
             this, SIGNAL(reportProcessResult(qbs::ProcessResult)));
-    connect(m_processCommandExecutor, SIGNAL(error(qbs::ErrorInfo)),
-            this, SLOT(onCommandError(qbs::ErrorInfo)));
-    connect(m_processCommandExecutor, SIGNAL(finished()), SLOT(onCommandFinished()));
+    connect(m_processCommandExecutor, SIGNAL(finished(qbs::ErrorInfo)),
+            this, SLOT(onCommandFinished(qbs::ErrorInfo)));
     connect(m_jsCommandExecutor, SIGNAL(reportCommandDescription(QString,QString)),
             this, SIGNAL(reportCommandDescription(QString,QString)));
-    connect(m_jsCommandExecutor, SIGNAL(error(qbs::ErrorInfo)),
-            this, SLOT(onCommandError(qbs::ErrorInfo)));
-    connect(m_jsCommandExecutor, SIGNAL(finished()), SLOT(onCommandFinished()));
+    connect(m_jsCommandExecutor, SIGNAL(finished(qbs::ErrorInfo)),
+            this, SLOT(onCommandFinished(qbs::ErrorInfo)));
     reset();
 }
 
@@ -136,27 +134,22 @@ void ExecutorJob::runNextCommand()
     m_currentCommandExecutor->start(m_transformer, command.data());
 }
 
-void ExecutorJob::onCommandError(const ErrorInfo &err)
+void ExecutorJob::onCommandFinished(const ErrorInfo &err)
 {
-    m_error = err;
-    setFinished();
-}
-
-void ExecutorJob::onCommandFinished()
-{
-    if (!m_transformer)
-        return;
-    runNextCommand();
+    QBS_ASSERT(m_transformer, return);
+    if (err.hasError()) {
+        m_error = err;
+        setFinished();
+    } else {
+        runNextCommand();
+    }
 }
 
 void ExecutorJob::setFinished()
 {
     const ErrorInfo err = m_error;
     reset();
-    if (err.hasError())
-        emit error(err);
-    else
-        emit success();
+    emit finished(err);
 }
 
 void ExecutorJob::reset()
diff --git a/src/lib/corelib/buildgraph/executorjob.h b/src/lib/corelib/buildgraph/executorjob.h
index dee7080b8..a6e4d1674 100644
--- a/src/lib/corelib/buildgraph/executorjob.h
+++ b/src/lib/corelib/buildgraph/executorjob.h
@@ -64,13 +64,11 @@ public:
 signals:
     void reportCommandDescription(const QString &highlight, const QString &message);
     void reportProcessResult(const qbs::ProcessResult &result);
-    void error(const qbs::ErrorInfo &error);
-    void success();
+    void finished(const qbs::ErrorInfo &error = ErrorInfo()); // !hasError() <=> command successful
 
 private slots:
     void runNextCommand();
-    void onCommandError(const qbs::ErrorInfo &err);
-    void onCommandFinished();
+    void onCommandFinished(const qbs::ErrorInfo &err);
 
 private:
     void setFinished();
diff --git a/src/lib/corelib/buildgraph/jscommandexecutor.cpp b/src/lib/corelib/buildgraph/jscommandexecutor.cpp
index 0b09a69ac..698002988 100644
--- a/src/lib/corelib/buildgraph/jscommandexecutor.cpp
+++ b/src/lib/corelib/buildgraph/jscommandexecutor.cpp
@@ -174,7 +174,7 @@ void JsCommandExecutor::onJavaScriptCommandFinished()
         logger().qbsDebug() << "JS code:\n" << jsCommand()->sourceCode();
         QString msg = tr("Error while executing JavaScriptCommand:\n");
         msg += result.errorMessage;
-        emit error(ErrorInfo(msg, result.errorLocation));
+        emit finished(ErrorInfo(msg, result.errorLocation));
     }
     emit finished();
 }
diff --git a/src/lib/corelib/buildgraph/processcommandexecutor.cpp b/src/lib/corelib/buildgraph/processcommandexecutor.cpp
index 6174d25dc..e1733ec40 100644
--- a/src/lib/corelib/buildgraph/processcommandexecutor.cpp
+++ b/src/lib/corelib/buildgraph/processcommandexecutor.cpp
@@ -133,7 +133,7 @@ void ProcessCommandExecutor::doStart()
             responseFile.setAutoRemove(false);
             responseFile.setFileTemplate(QDir::tempPath() + QLatin1String("/qbsresp"));
             if (!responseFile.open()) {
-                emit error(ErrorInfo(Tr::tr("Cannot create response file '%1'.")
+                emit finished(ErrorInfo(Tr::tr("Cannot create response file '%1'.")
                                  .arg(responseFile.fileName())));
                 return;
             }
@@ -175,7 +175,7 @@ QString ProcessCommandExecutor::filterProcessOutput(const QByteArray &_output,
                                                            + filterFunctionSource
                                                            + QLatin1String("; f"));
     if (!filterFunction.isFunction()) {
-        emit error(ErrorInfo(Tr::tr("Error in filter function: %1.\n%2")
+        logger().printWarning(ErrorInfo(Tr::tr("Error in filter function: %1.\n%2")
                          .arg(filterFunctionSource, filterFunction.toString())));
         return output;
     }
@@ -184,7 +184,7 @@ QString ProcessCommandExecutor::filterProcessOutput(const QByteArray &_output,
     outputArg.setProperty(0, scriptEngine()->toScriptValue(output));
     QScriptValue filteredOutput = filterFunction.call(scriptEngine()->undefinedValue(), outputArg);
     if (scriptEngine()->hasErrorOrException(filteredOutput)) {
-        emit error(ErrorInfo(Tr::tr("Error when calling output filter function: %1")
+        logger().printWarning(ErrorInfo(Tr::tr("Error when calling output filter function: %1")
                          .arg(filteredOutput.toString())));
         return output;
     }
@@ -224,32 +224,19 @@ void ProcessCommandExecutor::sendProcessOutput(bool success)
 
 void ProcessCommandExecutor::onProcessError()
 {
-    sendProcessOutput(false);
-    removeResponseFile();
-    QString errorMessage;
-    const QString binary = QDir::toNativeSeparators(processCommand()->program());
     switch (m_process.error()) {
-    case QProcess::FailedToStart:
-        errorMessage = Tr::tr("The process '%1' could not be started: %2").
-                arg(binary, m_process.errorString());
+    case QProcess::FailedToStart: {
+        removeResponseFile();
+        const QString binary = QDir::toNativeSeparators(processCommand()->program());
+        emit finished(ErrorInfo(Tr::tr("The process '%1' could not be started: %2")
+                                .arg(binary, m_process.errorString())));
         break;
+    }
     case QProcess::Crashed:
-        errorMessage = Tr::tr("The process '%1' crashed.").arg(binary);
-        break;
-    case QProcess::Timedout:
-        errorMessage = Tr::tr("The process '%1' timed out.").arg(binary);
-        break;
-    case QProcess::ReadError:
-        errorMessage = Tr::tr("Error reading process output from '%1'.").arg(binary);
-        break;
-    case QProcess::WriteError:
-        errorMessage = Tr::tr("Error writing to process '%1'.").arg(binary);
-        break;
+        break; // Ignore. Will be handled by onProcessFinished().
     default:
-        errorMessage = Tr::tr("Unknown process error running '%1'.").arg(binary);
-        break;
+        logger().qbsWarning() << "QProcess error: " << m_process.errorString();
     }
-    emit error(ErrorInfo(errorMessage));
 }
 
 void ProcessCommandExecutor::onProcessFinished(int exitCode)
@@ -260,13 +247,12 @@ void ProcessCommandExecutor::onProcessFinished(int exitCode)
             || quint32(exitCode) > quint32(processCommand()->maxExitCode());
     sendProcessOutput(!errorOccurred);
 
-    if (Q_UNLIKELY(errorOccurred && !crashed)) { // Crash is already reported in onProcessError().
-        emit error(ErrorInfo(Tr::tr("Process failed with exit code %1.").arg(exitCode)));
-        return;
-    }
-
-
-    emit finished();
+    if (Q_UNLIKELY(crashed))
+        emit finished(ErrorInfo(Tr::tr("Process crashed.")));
+    else if (Q_UNLIKELY(errorOccurred))
+        emit finished(ErrorInfo(Tr::tr("Process failed with exit code %1.").arg(exitCode)));
+    else
+        emit finished();
 }
 
 void ProcessCommandExecutor::removeResponseFile()