Several race related fixes for batch mode

Fixes race in batch mode, where main thread could cleanup before
batch mode thread, which resulted in crash.
Fixes race, where timer could fire after exception happened in batch
mode and cleanup was done, crashing an application.

Change-Id: I6b2dad49f3f154841bf5e436f23f0d5e796c9b3d

diff --git a/src/DoneCallback.cpp b/src/DoneCallback.cpp
index 77caab7..6b70d56 100644 (file)
--- a/src/DoneCallback.cpp
+++ b/src/DoneCallback.cpp
@@ -1,5 +1,6 @@
 #include "DoneCallback.hpp"
 #include "ecore.hpp"
+#include "UniversalSwitch.hpp"
 
 class DoneCallback::Data : public std::enable_shared_from_this<DoneCallback::Data>
 {
@@ -20,16 +21,21 @@ public:
        }
        void callAfterTimeElapsed(std::chrono::milliseconds ms)
        {
-               if (doneCb && !timer.isSet()) {
-                       timer.reset(ms.count() / 1000.0, [self = shared_from_this()]() {
+               if (doneCb && !timerIt) {
+                       timerIt = Singleton<UniversalSwitch>::instance().getBatchModeTimer();
+                       (**timerIt).reset(ms.count() / 1000.0, [self = shared_from_this()]() {
                                self->callAndReset();
+                               if (self->timerIt) {
+                                       Singleton<UniversalSwitch>::instance().eraseBatchModeTimer(*self->timerIt);
+                                       self->timerIt = {};
+                               }
                                return ecore::TimerRepetitionPolicy::cancel;
                        });
                }
        }
 private:
        std::function<void()> doneCb;
-       ecore::Timer timer;
+       Optional<std::list<ecore::Timer>::iterator> timerIt;
 };
 
 DoneCallback::DoneCallback(std::function<void()> doneCb)

diff --git a/src/NavigationInterface.cpp b/src/NavigationInterface.cpp
index 6a1173f..42950c1 100644 (file)
--- a/src/NavigationInterface.cpp
+++ b/src/NavigationInterface.cpp
@@ -690,6 +690,12 @@ private:
 
        ecore::Timer restartTimer;
 
+       void terminate()
+       {
+               restartTimer.reset();
+               rebuildContextTimer.reset();
+       }
+
        void rebuildContextFromZero(bool force)
        {
                auto atspi = Singleton<UniversalSwitch>::instance().getAtspi();

diff --git a/src/NavigationInterface.hpp b/src/NavigationInterface.hpp
index b4d7060..d63446b 100644 (file)
--- a/src/NavigationInterface.hpp
+++ b/src/NavigationInterface.hpp
@@ -107,6 +107,7 @@ public:
        virtual ~NavigationInterface();
        virtual std::shared_ptr<UIElement> getCurrentVisibleRoot() const = 0;
        virtual std::shared_ptr<NavigationElement> getCurrentNavigationContext() const = 0;
+       virtual void terminate() = 0;
 
        template <NavigationCallbackType type>
        CallbackHandle registerCb(std::function<typename NavigationCallbackTraits<type>::CallbackFunctionType> callback)

diff --git a/src/UniversalSwitch.cpp b/src/UniversalSwitch.cpp
index 68154ba..2c5b8a9 100644 (file)
--- a/src/UniversalSwitch.cpp
+++ b/src/UniversalSwitch.cpp
@@ -35,6 +35,7 @@
 #include "utils.hpp"
 #include "DBus.hpp"
 #include "batch/BatchRunner.hpp"
+#include "NavigationInterface.hpp"
 
 #include <device/display.h>
 
@@ -48,7 +49,8 @@ bool UniversalSwitch::initialize(const std::array<Optional<std::string>, (size_t
                batchOutputPath = *arguments[(size_t)utils::Argument::OutputPath];
 
                changeMode(Mode::testExecution);
-               return runBatch(batchFilePath, batchOutputPath);
+               batchThread = runBatch(batchFilePath, batchOutputPath);
+               return bool(batchThread);
        }
 
        auto compositeSwitchProvider = std::make_shared<CompositeSwitchProvider>();
@@ -229,8 +231,30 @@ void UniversalSwitch::setConfiguration(const std::shared_ptr<Configuration> &con
        configuration = config;
 }
 
+void UniversalSwitch::purgeBatchTimerIfNeeded()
+{
+       if (batchModeTimerToDelete != batchModeTimers.end()) {
+               batchModeTimers.erase(batchModeTimerToDelete);
+               batchModeTimerToDelete = batchModeTimers.end();
+       }
+}
+
+std::list<ecore::Timer>::iterator UniversalSwitch::getBatchModeTimer()
+{
+       purgeBatchTimerIfNeeded();
+       batchModeTimers.emplace_back();
+       return --batchModeTimers.end();
+}
+
+void UniversalSwitch::eraseBatchModeTimer(std::list<ecore::Timer>::iterator it)
+{
+       purgeBatchTimerIfNeeded();
+       batchModeTimerToDelete = it;
+}
+
 void UniversalSwitch::terminate()
 {
+       navigationInterface->terminate();
        device_remove_callback(DEVICE_CALLBACK_DISPLAY_STATE, displayStateChangedCallback);
        callbackHandle.reset();
        changeMode(Mode::passive);
@@ -238,4 +262,11 @@ void UniversalSwitch::terminate()
        compositeSwitchProvider.reset();
        configuration.reset();
        dbusInterface.reset();
+       if (batchThread) {
+               DEBUG("waiting for batch thread to finalize");
+               batchThread->join();
+               DEBUG("waiting for batch thread to finalize done");
+       }
+       purgeBatchTimerIfNeeded();
+       batchModeTimers.clear();
 }

diff --git a/src/UniversalSwitch.hpp b/src/UniversalSwitch.hpp
index 088b2c0..99117cb 100644 (file)
--- a/src/UniversalSwitch.hpp
+++ b/src/UniversalSwitch.hpp
@@ -21,10 +21,13 @@
 #include "VConf.hpp"
 #include "Optional.hpp"
 #include "utils.hpp"
+#include "ecore.hpp"
 
 #include <device/callback.h>
 
 #include <memory>
+#include <list>
+#include <thread>
 
 class Atspi;
 class CompositeSwitchProvider;
@@ -65,6 +68,9 @@ public:
        bool isInBatchMode() const;
        bool initialize(const std::array<Optional<std::string>, (size_t)utils::Argument::_count> &arguments);
 
+       std::list<ecore::Timer>::iterator getBatchModeTimer();
+       void eraseBatchModeTimer(std::list<ecore::Timer>::iterator it);
+       
        void terminate();
 private:
        void changeMode(Mode mode);
@@ -74,6 +80,7 @@ private:
        void setConfiguration(const std::shared_ptr<Configuration> &config);
        void setDBusInterface(const std::shared_ptr<DBusInterface> &dbus);
        void resetA11yDbusProperties();
+       void purgeBatchTimerIfNeeded();
        static void displayStateChangedCallback(device_callback_e type, void *value, void *user_data);
 
        std::shared_ptr<SwitchManager> switchManager;
@@ -94,6 +101,9 @@ private:
        std::string batchFilePath;
        std::string batchOutputPath;
        Mode mode;
+       std::list<ecore::Timer> batchModeTimers;
+       std::list<ecore::Timer>::iterator batchModeTimerToDelete = batchModeTimers.end();
+       Optional<std::thread> batchThread;
 };
 
 #endif

diff --git a/src/batch/BatchRunner.cpp b/src/batch/BatchRunner.cpp
index f1ef3a9..9be75eb 100644 (file)
--- a/src/batch/BatchRunner.cpp
+++ b/src/batch/BatchRunner.cpp
@@ -254,10 +254,12 @@ struct TestExecutor : ExecutorInterface {
                        [&](EvaluationContext & ec, std::string name) -> EvaluationValue {
                                auto h = contextInfo.lock();
                                auto untilMoment = std::chrono::high_resolution_clock::now() + 3000ms;
-                               if (!h.waitForCondition(untilMoment, [&]() {
+                               auto success = h.waitForCondition(untilMoment, [&]() {
                                        return h->rootName == name;
-                               }))
-                                       throw EvaluationFailure{} << "wait_for_application('" << name << "'): operation timeouted";
+                               });
+                               if (!success)
+                                       throw EvaluationFailure{} << "wait_for_application('" << name << "'): operation timeouted, " <<
+                                                                                                 "current root name is '" << h->rootName << "'";
                                return {};
                        }
                };
@@ -378,6 +380,7 @@ struct TestExecutor : ExecutorInterface {
                                if (r)
                                        name = std::move(*r);
                        }
+                       DEBUG("context changed to root %s (%s)", root ? Atspi::getUniqueId(root->getObject()).c_str() : "", name.c_str());
                        auto h = contextInfo.lock();
                        h->navigation = std::move(navigationContext);
                        h->root = std::move(root);
@@ -757,7 +760,7 @@ struct TestExecutor : ExecutorInterface {
        }
 };
 
-bool runBatch(const std::string &sourceName, std::string outputName)
+Optional<std::thread> runBatch(const std::string &sourceName, std::string outputName)
 {
        std::unique_ptr<std::ostream> outputPtr;
        if (outputName.empty()) outputName = "/dev/null";
@@ -766,13 +769,13 @@ bool runBatch(const std::string &sourceName, std::string outputName)
 
        if (!static_cast<std::ofstream *>(outputPtr.get())->is_open()) {
                ERROR("failed to open output file '%s'", outputName.c_str());
-               return false;
+               return {};
        }
        auto &output = *outputPtr;
        auto source = std::ifstream{ sourceName, std::ios_base::in };
        if (!source.is_open()) {
                output << "failed to open file '" << sourceName << "'\n";
-               return false;
+               return {};
        }
 
        source.seekg(0, std::ios_base::end);
@@ -783,7 +786,7 @@ bool runBatch(const std::string &sourceName, std::string outputName)
        source.read(bufor.data(), total);
        if ((size_t)source.gcount() != total) {
                output << "error while reading file '" << sourceName << "'\n";
-               return false;
+               return {};
        }
        auto content = std::string{ bufor.data(), (size_t)total };
 
@@ -791,7 +794,7 @@ bool runBatch(const std::string &sourceName, std::string outputName)
        auto tokens = lexTest(error, sourceName, content);
        if (!error.empty()) {
                output << error << "\nlexing failed\n";
-               return false;
+               return {};
        }
 
        std::vector<std::string> errors;
@@ -802,7 +805,7 @@ bool runBatch(const std::string &sourceName, std::string outputName)
                        output << e << "\n";
                }
                output << "parsing failed\n";
-               return false;
+               return {};
        }
 
        // std::ostringstream os;
@@ -811,39 +814,43 @@ bool runBatch(const std::string &sourceName, std::string outputName)
 
        output << "executing test '" << sourceName << "'\n";
        auto exec = std::make_unique<TestExecutor>(*outputPtr);
-       std::thread { [result, exec = std::move(exec), outputPtr = std::move(outputPtr)]()
+       return std::thread { [result = std::move(result), exec = std::move(exec), outputPtr = std::move(outputPtr)]()
        {
                EvaluationContext &ec = exec->ec;
                exec->outputStream() << "waiting for context change...\n";
+               bool hasContext = false;
                auto until = std::chrono::high_resolution_clock::now() + std::chrono::seconds{ 2 };
-               while (true) {
-                       if (std::chrono::high_resolution_clock::now() >= until) {
-                               exec->outputStream() << "timeouted\n";
-                               DEBUG("timeouted, when waiting for context change");
-                               break;
-                       }
-                       if (exec->getVisibleRoot()) {
-                               exec->outputStream() << "evaluation started\n";
-                               try {
-                                       result->evaluate(ec);
-                                       exec->outputStream() << "evaluation successed\n";
-                                       std::this_thread::sleep_for(std::chrono::seconds{ 1 });
-                               } catch (EvaluationFailure &e) {
-                                       if (e.hasLocation())
-                                               exec->outputStream() << e.location().toString() << ": ";
-                                       exec->outputStream() << e.message() << "\nevaluation failed\n";
-                               } catch (...) {
-                                       exec->outputStream() << "unhandled exception\nevaluation failed\n";
-                               }
-                               break;
+               {
+                       auto h = exec->contextInfo.lock();
+                       hasContext = h.waitForCondition(until, [&]() {
+                               // h->root is pointer to root of visible at-spi hierarchy
+                               // if it's null, then either context building machinery didn't yet finish,
+                               // there's nothing visible to show or something went wrong
+                               // here we wait for it to be non-null (context finished constructing)
+                               // if it timeouts - something went wrong, so we bail out with an error
+                               return bool(h->root);
+                       });
+               }
+               if (hasContext) {
+                       exec->outputStream() << "evaluation started\n";
+                       try {
+                               result->evaluate(ec);
+                               exec->outputStream() << "evaluation successed\n";
+                       } catch (EvaluationFailure &e) {
+                               if (e.hasLocation())
+                                       exec->outputStream() << e.location().toString() << ": ";
+                               exec->outputStream() << e.message() << "\nevaluation failed\n";
+                       } catch (...) {
+                               exec->outputStream() << "unhandled exception\nevaluation failed\n";
                        }
-                       std::this_thread::sleep_for(std::chrono::milliseconds{ 10 });
+               } else {
+                       exec->outputStream() << "timeouted\n";
+                       DEBUG("timeouted, when waiting for context change");
                }
                outputPtr->flush();
-
                executeOnMainThread([]() {
                        ecore_main_loop_quit();
                });
-       } } .detach();
-       return true;
+               DEBUG("done batch");
+       } };
 }

diff --git a/src/batch/BatchRunner.hpp b/src/batch/BatchRunner.hpp
index 76ae4fb..bcc2248 100644 (file)
--- a/src/batch/BatchRunner.hpp
+++ b/src/batch/BatchRunner.hpp
@@ -18,6 +18,7 @@
 #define BATCH_RUNNER_HPP
 
 #include <string>
+#include <thread>
 #include "EvaluationValue.hpp"
 #include "../Optional.hpp"
 
@@ -47,6 +48,6 @@ public:
        }
 };
 
-bool runBatch(const std::string &sourceName, std::string outputName);
+Optional<std::thread> runBatch(const std::string &sourceName, std::string outputName);
 
 #endif

diff --git a/src/ecore.cpp b/src/ecore.cpp
index e9605e8..b16ca7a 100644 (file)
--- a/src/ecore.cpp
+++ b/src/ecore.cpp
@@ -61,12 +61,14 @@ namespace ecore
        Eina_Bool Timer::onCallback()
        {
                auto prevTimer = timerHandler;
+               ASSERT(method);
                auto result = method();
 
                ASSERT(prevTimer == timerHandler || result == ecore::TimerRepetitionPolicy::cancel, "Timer changed during callback invokation");
 
                if (result == ecore::TimerRepetitionPolicy::cancel) {
                        timerHandler = (prevTimer != timerHandler) ? timerHandler : nullptr;
+                       if (!timerHandler) method = {};
                        return ECORE_CALLBACK_CANCEL;
                }
 

diff --git a/src/main.cpp b/src/main.cpp
index 88219ab..946f04d 100644 (file)
--- a/src/main.cpp
+++ b/src/main.cpp
@@ -91,5 +91,7 @@ int main(int argc, char *argv[])
        app_event_handler_h handler;
        ui_app_add_event_handler(&handler, APP_EVENT_LANGUAGE_CHANGED, _setting_time_lang_changed, nullptr);
 
-       return ui_app_main(argc, argv, &event_callback, &data);
+       auto result = ui_app_main(argc, argv, &event_callback, &data);
+       DEBUG("main function done");
+       return result;
 }