Loader: Do not share logger between threads

The logger is not thread-safe when stored warnings are enabled.

Change-Id: Icb7a3add9f6162ab823c0b27059aea88822ff60a
Reviewed-by: Ivan Komissarov <ABBAPOH@gmail.com>

6 files changed, 40 insertions, 19 deletions

diff --git a/src/lib/corelib/loader/loaderutils.cpp b/src/lib/corelib/loader/loaderutils.cpp
index 07628de5f..7c9bb899a 100644
--- a/src/lib/corelib/loader/loaderutils.cpp
+++ b/src/lib/corelib/loader/loaderutils.cpp
@@ -549,24 +549,27 @@ class LoaderState::Private
 public:
     Private(LoaderState &q, const SetupProjectParameters &parameters,
             TopLevelProjectContext &topLevelProject, ItemPool &itemPool, ScriptEngine &engine,
-            Logger &logger)
+            Logger &&logger)
         : parameters(parameters), topLevelProject(topLevelProject), itemPool(itemPool),
-          logger(logger), itemReader(q), evaluator(&engine)
-    {}
+          logger(std::move(logger)), itemReader(q), evaluator(&engine)
+    {
+        this->logger.clearWarnings();
+        this->logger.storeWarnings();
+    }
 
     const SetupProjectParameters &parameters;
     TopLevelProjectContext &topLevelProject;
     ItemPool &itemPool;
-    Logger &logger;
 
+    Logger logger;
     ItemReader itemReader;
     Evaluator evaluator;
 };
 
 LoaderState::LoaderState(const SetupProjectParameters &parameters,
                          TopLevelProjectContext &topLevelProject, ItemPool &itemPool,
-                         ScriptEngine &engine, Logger &logger)
-    : d(makePimpl<Private>(*this, parameters, topLevelProject, itemPool, engine, logger))
+                         ScriptEngine &engine, Logger logger)
+    : d(makePimpl<Private>(*this, parameters, topLevelProject, itemPool, engine, std::move(logger)))
 {
     d->itemReader.init();
 }
diff --git a/src/lib/corelib/loader/loaderutils.h b/src/lib/corelib/loader/loaderutils.h
index de611303b..a98f80a2a 100644
--- a/src/lib/corelib/loader/loaderutils.h
+++ b/src/lib/corelib/loader/loaderutils.h
@@ -410,7 +410,7 @@ class LoaderState
 {
 public:
     LoaderState(const SetupProjectParameters &parameters, TopLevelProjectContext &topLevelProject,
-                ItemPool &itemPool, ScriptEngine &engine, Logger &logger);
+                ItemPool &itemPool, ScriptEngine &engine, Logger logger);
     ~LoaderState();
 
     Evaluator &evaluator();
diff --git a/src/lib/corelib/loader/productsresolver.cpp b/src/lib/corelib/loader/productsresolver.cpp
index d58026445..8d1ea99af 100644
--- a/src/lib/corelib/loader/productsresolver.cpp
+++ b/src/lib/corelib/loader/productsresolver.cpp
@@ -532,6 +532,13 @@ void ProductsResolver::postProcess()
 
     for (const auto &engine : m_enginePool)
         m_loaderState.topLevelProject().collectDataFromEngine(*engine);
+
+    QBS_CHECK(!m_loaderState.topLevelProject().projects().empty());
+    const auto project = std::dynamic_pointer_cast<TopLevelProject>(
+                m_loaderState.topLevelProject().projects().front()->project);
+    QBS_CHECK(project);
+    for (LoaderState * const loaderState : m_availableLoaderStates)
+        project->warningsEncountered << loaderState->logger().warnings();
 }
 
 int ProductsResolver::dependsItemCount(const Item *item)
diff --git a/src/lib/corelib/loader/projectresolver.cpp b/src/lib/corelib/loader/projectresolver.cpp
index eb7b66296..d928048fb 100644
--- a/src/lib/corelib/loader/projectresolver.cpp
+++ b/src/lib/corelib/loader/projectresolver.cpp
@@ -300,8 +300,8 @@ TopLevelProjectPtr ProjectResolver::Private::resolveTopLevelProject()
     state.topLevelProject().collectDataFromEngine(*engine);
     makeSubProjectNamesUniqe(project);
     checkForDuplicateProductNames(project);
+    project->warningsEncountered << logger.warnings();
 
-    project->warningsEncountered = logger.warnings();
     return project;
 }
 
diff --git a/tests/auto/api/testdata/restored-warnings/restored-warnings.qbs b/tests/auto/api/testdata/restored-warnings/restored-warnings.qbs
index bbdfbeadb..f6a68f27c 100644
--- a/tests/auto/api/testdata/restored-warnings/restored-warnings.qbs
+++ b/tests/auto/api/testdata/restored-warnings/restored-warnings.qbs
@@ -1,14 +1,21 @@
 import qbs.Process 1.5
 
-CppApplication {
-    name: "theProduct"
+Project {
+    CppApplication {
+        name: "theProduct"
 
-    property bool moreFiles: false
-    cpp.blubb: true
+        property bool moreFiles: false
+        cpp.blubb: true
 
-    files: ["file.cpp", "main.cpp"]
-    Group {
-        condition: moreFiles
-        files: ["blubb.cpp"]
+        files: ["file.cpp", "main.cpp"]
+        Group {
+            condition: moreFiles
+            files: ["blubb.cpp"]
+        }
+    }
+
+    Product {
+        name: "theOtherProduct"
+        property bool dummy: { throw "this one comes from a thread"; }
     }
 }
diff --git a/tests/auto/api/tst_api.cpp b/tests/auto/api/tst_api.cpp
index 4eb6b4654..140cadeb9 100644
--- a/tests/auto/api/tst_api.cpp
+++ b/tests/auto/api/tst_api.cpp
@@ -2711,12 +2711,14 @@ void TestApi::restoredWarnings()
     waitForFinished(job.get());
     QVERIFY2(!job->error().hasError(), qPrintable(job->error().toString()));
     job.reset(nullptr);
-    QCOMPARE(toSet(m_logSink->warnings).size(), 3);
+    QCOMPARE(toSet(m_logSink->warnings).size(), 5);
     const auto beforeErrors = m_logSink->warnings;
     for (const qbs::ErrorInfo &e : beforeErrors) {
         const QString msg = e.toString();
         QVERIFY2(msg.contains("Superfluous version")
                  || msg.contains("Property 'blubb' is not declared")
+                 || msg.contains("this one comes from a thread")
+                 || msg.contains("Product 'theOtherProduct' had errors and was disabled")
                  || msg.contains("Product 'theProduct' had errors and was disabled"),
                  qPrintable(msg));
     }
@@ -2727,7 +2729,7 @@ void TestApi::restoredWarnings()
     waitForFinished(job.get());
     QVERIFY2(!job->error().hasError(), qPrintable(job->error().toString()));
     job.reset(nullptr);
-    QCOMPARE(toSet(m_logSink->warnings).size(), 3);
+    QCOMPARE(toSet(m_logSink->warnings).size(), 5);
     m_logSink->warnings.clear();
 
     // Re-resolving with changes: Errors come from the re-resolving, stored ones must be suppressed.
@@ -2738,13 +2740,15 @@ void TestApi::restoredWarnings()
     waitForFinished(job.get());
     QVERIFY2(!job->error().hasError(), qPrintable(job->error().toString()));
     job.reset(nullptr);
-    QCOMPARE(toSet(m_logSink->warnings).size(), 4); // One more for the additional group
+    QCOMPARE(toSet(m_logSink->warnings).size(), 6); // One more for the additional group
     const auto afterErrors = m_logSink->warnings;
     for (const qbs::ErrorInfo &e : afterErrors) {
         const QString msg = e.toString();
         QVERIFY2(msg.contains("Superfluous version")
                  || msg.contains("Property 'blubb' is not declared")
                  || msg.contains("blubb.cpp' does not exist")
+                 || msg.contains("this one comes from a thread")
+                 || msg.contains("Product 'theOtherProduct' had errors and was disabled")
                  || msg.contains("Product 'theProduct' had errors and was disabled"),
                  qPrintable(msg));
     }