Fix unwanted "concurrency" in Executor.v1.4.51.4

Script engine evaluation is interrupted once a second to check for
cancel requests. At this time, jobs can finish, leading to the
onJobFinished() slot being called while we are in the middle of
executeRuleNode(). Possible effects:
    - The job that just finished was the only currently running one,
      and its transformer outputs have no parents. Then the Executor
      will think it's done, and higher-level code will free the
      evaluation context (and with it the engine). When script execution
      continues after the current round of events processing, a null
      pointer or freed memory will be accessed.
    - The job's transformer outputs do have parent artifacts. In this
      case, we are suddenly and most unexpectedly in executeRuleNode()
      twice, with the potential for all kinds of stunning behavior.
Fix the problem by checking in the onJobFinished() slot whether the
rules evaluation context is currently active and delaying slot execution
if it is.

Task-number: QBS-932
Change-Id: I03ca4f4ad844357e49d5acaddff066395a7f95cf
Reviewed-by: Joerg Bornemann <joerg.bornemann@theqtcompany.com>

1 files changed, 21 insertions, 11 deletions

diff --git a/src/lib/corelib/buildgraph/executor.cpp b/src/lib/corelib/buildgraph/executor.cpp
index 8a028f0dd..7cfc3137a 100644
--- a/src/lib/corelib/buildgraph/executor.cpp
+++ b/src/lib/corelib/buildgraph/executor.cpp
@@ -435,6 +435,7 @@ void Executor::buildArtifact(Artifact *artifact)
 
 void Executor::executeRuleNode(RuleNode *ruleNode)
 {
+    QBS_CHECK(!m_evalContext->isActive());
     ArtifactSet changedInputArtifacts;
     if (ruleNode->rule()->isDynamic()) {
         foreach (Artifact *artifact, m_changedSourceArtifacts) {
@@ -891,20 +892,28 @@ void Executor::possiblyInstallArtifact(const Artifact *artifact)
 
 void Executor::onJobFinished(const qbs::ErrorInfo &err)
 {
-    if (err.hasError()) {
-        if (m_buildOptions.keepGoing()) {
-            ErrorInfo fullWarning(err);
-            fullWarning.prepend(Tr::tr("Ignoring the following errors on user request:"));
-            m_logger.printWarning(fullWarning);
-        } else {
-            if (!m_error.hasError())
-                m_error = err; // All but the first one could be due to canceling.
-        }
-    }
-
     try {
         ExecutorJob * const job = qobject_cast<ExecutorJob *>(sender());
         QBS_CHECK(job);
+        if (m_evalContext->isActive()) {
+            m_logger.qbsDebug() << "Executor job finished while rule execution is pausing. "
+                                   "Delaying slot execution.";
+            QMetaObject::invokeMethod(job, "finished", Qt::QueuedConnection,
+                                      Q_ARG(qbs::ErrorInfo, err));
+            return;
+        }
+
+        if (err.hasError()) {
+            if (m_buildOptions.keepGoing()) {
+                ErrorInfo fullWarning(err);
+                fullWarning.prepend(Tr::tr("Ignoring the following errors on user request:"));
+                m_logger.printWarning(fullWarning);
+            } else {
+                if (!m_error.hasError())
+                    m_error = err; // All but the first one could be due to canceling.
+            }
+        }
+
         finishJob(job, !err.hasError());
     } catch (const ErrorInfo &error) {
         handleError(error);
@@ -914,6 +923,7 @@ void Executor::onJobFinished(const qbs::ErrorInfo &err)
 void Executor::finish()
 {
     QBS_ASSERT(m_state != ExecutorIdle, /* ignore */);
+    QBS_ASSERT(!m_evalContext || !m_evalContext->isActive(), /* ignore */);
 
     QList<ResolvedProductPtr> unbuiltProducts;
     foreach (const ResolvedProductPtr &product, m_productsToBuild) {