From e331329e4f8bb3de30f99f0905a0d2517d4c227c Mon Sep 17 00:00:00 2001
From: Jarek Kobus <jaroslaw.kobus@qt.io>
Date: Wed, 12 Jun 2024 15:42:07 +0200
Subject: [PATCH] TaskTree: Detect a misconfigured recipe of a nested task tree

Change-Id: I6652336023ac111cde5334e655f5dd972977b07f
Reviewed-by: hjk <hjk@qt.io>
---
 src/libs/solutions/tasking/tasktree.cpp      | 22 +++++----
 tests/auto/solutions/tasking/tst_tasking.cpp | 50 ++++++++++++++++++++
 2 files changed, 64 insertions(+), 8 deletions(-)

diff --git a/src/libs/solutions/tasking/tasktree.cpp b/src/libs/solutions/tasking/tasktree.cpp
index d923eb31d0e..5c1f65e503f 100644
--- a/src/libs/solutions/tasking/tasktree.cpp
+++ b/src/libs/solutions/tasking/tasktree.cpp
@@ -1291,6 +1291,12 @@ const void *Loop::valuePtr() const
 
 using StoragePtr = void *;
 
+static QString s_activeStorageWarning =
+    "The referenced storage is not reachable in the running tree. "
+    "A nullptr will be returned which might lead to a crash in the calling code. "
+    "It is possible that no storage was added to the tree, "
+    "or the storage is not reachable from where it is referenced.";
+
 class StorageThreadData
 {
     Q_DISABLE_COPY_MOVE(StorageThreadData)
@@ -1299,7 +1305,7 @@ public:
     StorageThreadData() = default;
     void pushStorage(StoragePtr storagePtr)
     {
-        m_activeStorageStack.push_back(storagePtr);
+        m_activeStorageStack.push_back({storagePtr, activeTaskTree()});
     }
     void popStorage()
     {
@@ -1308,16 +1314,16 @@ public:
     }
     StoragePtr activeStorage() const
     {
-        QT_ASSERT(m_activeStorageStack.size(), qWarning(
-            "The referenced storage is not reachable in the running tree. "
-            "A nullptr will be returned which might lead to a crash in the calling code. "
-            "It is possible that no storage was added to the tree, "
-            "or the storage is not reachable from where it is referenced."); return nullptr);
-        return m_activeStorageStack.last();
+        QT_ASSERT(m_activeStorageStack.size(),
+                  qWarning().noquote() << s_activeStorageWarning; return nullptr);
+        const QPair<StoragePtr, TaskTree *> &top = m_activeStorageStack.last();
+        QT_ASSERT(top.second == activeTaskTree(),
+                  qWarning().noquote() << s_activeStorageWarning; return nullptr);
+        return top.first;
     }
 
 private:
-    QList<StoragePtr> m_activeStorageStack;
+    QList<QPair<StoragePtr, TaskTree *>> m_activeStorageStack;
 };
 
 class StorageData
diff --git a/tests/auto/solutions/tasking/tst_tasking.cpp b/tests/auto/solutions/tasking/tst_tasking.cpp
index db293d4ebfb..f3d83b4806d 100644
--- a/tests/auto/solutions/tasking/tst_tasking.cpp
+++ b/tests/auto/solutions/tasking/tst_tasking.cpp
@@ -118,6 +118,7 @@ private slots:
     void storageOperators();
     void storageDestructor();
     void storageZeroInitialization();
+    void nestedBrokenStorage();
     void restart();
     void destructorOfTaskEmittingDone();
 };
@@ -3452,6 +3453,55 @@ void tst_Tasking::storageZeroInitialization()
     QCOMPARE(defaultValue, 0);
 }
 
+// This test ensures that when a missing storage object inside the nested task tree is accessed
+// directly from the outer task tree's handler, containing the same storage object, then we
+// detect this misconfigured recipe, issue a warning, and return nullptr for the storage
+// being accessed (instead of returning parent's task tree's storage instance).
+// This test should also trigger a runtime assert that we are accessing the nullptr storage.
+void tst_Tasking::nestedBrokenStorage()
+{
+    int *outerStorage = nullptr;
+    int *innerStorage1 = nullptr;
+    int *innerStorage2 = nullptr;
+    const Storage<int> storage;
+
+    const auto onOuterSync = [storage, &outerStorage, &innerStorage1, &innerStorage2] {
+        outerStorage = &*storage;
+
+        const auto onInnerSync1 = [storage, &innerStorage1] {
+            innerStorage1 = &*storage; // Triggers the runtime assert on purpose.
+        };
+        const auto onInnerSync2 = [storage, &innerStorage2] {
+            innerStorage2 = &*storage; // Storage is accessible in currently running tree - all OK.
+        };
+
+        const Group innerRecipe {
+            Group {
+                // Broken subrecipe, the storage wasn't placed inside the recipe.
+                // storage,
+                Sync(onInnerSync1)
+            },
+            Group {
+                storage, // Subrecipe OK, another instance for the nested storage will be created.
+                Sync(onInnerSync2)
+            }
+        };
+
+        TaskTree::runBlocking(innerRecipe);
+    };
+
+    const Group outerRecipe {
+        storage,
+        Sync(onOuterSync)
+    };
+
+    TaskTree::runBlocking(outerRecipe);
+    QVERIFY(outerStorage != nullptr);
+    QCOMPARE(innerStorage1, nullptr);
+    QVERIFY(innerStorage2 != nullptr);
+    QVERIFY(innerStorage2 != outerStorage);
+}
+
 void tst_Tasking::restart()
 {
     TaskTree taskTree({TestTask([](TaskObject &taskObject) { taskObject = 1000ms; })});