Issue 4938 - fix race conditions related to LazyFileSystem

test/FilterEngine.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2017 eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 16 matching lines @@
   {
     inline bool BeginsWith(const std::string& str, const std::string& beginning)
     {
       return 0 == str.compare(0, beginning.size(), beginning);
     }
   }
 }
 
 namespace
 {
-  class VeryLazyFileSystem : public LazyFileSystem
+  class NoFilesFileSystem : public LazyFileSystem
   {
   public:
-    StatResult Stat(const std::string& path) const
+    void Stat(const std::string& path, const StatCallback& callback) const override
     {
-      return StatResult();
+      scheduler([callback]
+      {
+        callback(StatResult(), "");
+      });
     }
   };
 
-  template<class FileSystem, class LogSystem>
+  template<class LazyFileSystemT, class LogSystem>
   class FilterEngineTestGeneric : public ::testing::Test
   {
   protected:
     FilterEnginePtr filterEngine;
 
     void SetUp() override
     {
+      LazyFileSystemT* fileSystem;
       JsEngineCreationParameters jsEngineParams;
-      jsEngineParams.fileSystem.reset(new FileSystem());
+      jsEngineParams.fileSystem.reset(fileSystem = new LazyFileSystemT());
       jsEngineParams.logSystem.reset(new LogSystem());
       jsEngineParams.timer.reset(new NoopTimer());
       jsEngineParams.webRequest.reset(new NoopWebRequest());
       auto jsEngine = CreateJsEngine(std::move(jsEngineParams));
-      filterEngine = AdblockPlus::FilterEngine::Create(jsEngine);
-    }
-    void TearDown() override
-    {
-      // Workaround for issue 5198
-      std::this_thread::sleep_for(std::chrono::milliseconds(100));

[sergei, 2017/07/27 09:15:32]

Since there is no class waiting for its internal threads it was already no the
issue #5198, however this sleep might was required to allow FileSystem finish
running operations. Now it may be removed.

+      filterEngine = CreateFilterEngine(*fileSystem, jsEngine);
     }
   };
 
   typedef FilterEngineTestGeneric<LazyFileSystem, AdblockPlus::DefaultLogSystem> FilterEngineTest;
-  typedef FilterEngineTestGeneric<VeryLazyFileSystem, LazyLogSystem> FilterEngineTestNoData;
+  typedef FilterEngineTestGeneric<NoFilesFileSystem, LazyLogSystem> FilterEngineTestNoData;
 
   class FilterEngineWithFreshFolder : public ::testing::Test
   {
   protected:
     FileSystemPtr fileSystem;
     std::weak_ptr<JsEngine> weakJsEngine;
 
     void SetUp() override
     {
       fileSystem = CreateDefaultFileSystem();
@@ skipping 62 matching lines @@
   protected:
     typedef std::vector<std::pair<bool, std::string>> ConnectionTypes;
     DelayedWebRequest::SharedTasks webRequestTasks;
     DelayedTimer::SharedTasks timerTasks;
     FilterEngine::CreationParameters createParams;
     ConnectionTypes capturedConnectionTypes;
     bool isConnectionAllowed;
     std::vector<std::function<void(bool)>> isSubscriptionDownloadAllowedCallbacks;
     FilterEnginePtr filterEngine;
     JsEnginePtr jsEngine;
+    LazyFileSystem* fileSystem;
 
     void SetUp()
     {
       isConnectionAllowed = true;
 
       JsEngineCreationParameters jsEngineParams;
       jsEngineParams.logSystem.reset(new LazyLogSystem());
-      jsEngineParams.fileSystem.reset(new LazyFileSystem());
+      jsEngineParams.fileSystem.reset(fileSystem = new LazyFileSystem());
       jsEngineParams.timer = DelayedTimer::New(timerTasks);
       jsEngineParams.webRequest = DelayedWebRequest::New(webRequestTasks);
       jsEngine = CreateJsEngine(std::move(jsEngineParams));
 
       createParams.preconfiguredPrefs.emplace("first_run_subscription_auto_select", jsEngine->NewValue(false));
 
       createParams.isSubscriptionDownloadAllowedCallback = [this](const std::string* allowedConnectionType,
         const std::function<void(bool)>& isSubscriptionDownloadAllowedCallback){
         capturedConnectionTypes.emplace_back(!!allowedConnectionType, allowedConnectionType ? *allowedConnectionType : std::string());
         isSubscriptionDownloadAllowedCallbacks.emplace_back(isSubscriptionDownloadAllowedCallback);
       };
     }
 
     Subscription EnsureExampleSubscriptionAndForceUpdate(const std::string& apppendToUrl = std::string())
     {
       auto subscriptionUrl = "http://example" + apppendToUrl;
       bool isSubscriptionDownloadStatusReceived = false;
       if (!filterEngine)
       {
-        filterEngine = FilterEngine::Create(jsEngine, createParams);
+        filterEngine = CreateFilterEngine(*fileSystem, jsEngine, createParams);
         filterEngine->SetFilterChangeCallback([&isSubscriptionDownloadStatusReceived, &subscriptionUrl](const std::string& action, JsValue&& item)
         {
           if (action == "subscription.downloadStatus" && item.GetProperty("url").AsString() == subscriptionUrl)
             isSubscriptionDownloadStatusReceived = true;
         });
       }
       auto subscription = filterEngine->GetSubscription(subscriptionUrl);
       EXPECT_EQ(0u, subscription.GetProperty("filters").AsList().size()) << subscriptionUrl;
       EXPECT_TRUE(subscription.GetProperty("downloadStatus").IsNull()) << subscriptionUrl;
       subscription.UpdateFilters();
@@ skipping 393 matching lines @@
 }
 
 TEST_F(FilterEngineTestNoData, FirstRunFlag)
 {
   ASSERT_TRUE(filterEngine->IsFirstRun());
 }
 
 TEST_F(FilterEngineTest, SetRemoveFilterChangeCallback)
 {
   int timesCalled = 0;
-  std::this_thread::sleep_for(std::chrono::milliseconds(200));

[sergei, 2017/07/27 09:15:32]

#4937 is also fixed now

   filterEngine->SetFilterChangeCallback([&timesCalled](const std::string&, AdblockPlus::JsValue&&)
   {
     timesCalled++;
   });
   filterEngine->GetFilter("foo").AddToList();
   EXPECT_EQ(1, timesCalled);
 
   // we want to actually check the call count didn't change.
   filterEngine->RemoveFilterChangeCallback();
   filterEngine->GetFilter("foo").RemoveFromList();
@@ skipping 420 matching lines @@
     std::string testConnection = "test connection";
     filterEngine->SetAllowedConnectionType(&testConnection);
     auto subscription = EnsureExampleSubscriptionAndForceUpdate("subB");
     EXPECT_EQ("synchronize_ok", subscription.GetProperty("downloadStatus").AsString());
     EXPECT_EQ(1u, subscription.GetProperty("filters").AsList().size());
     ASSERT_EQ(1u, capturedConnectionTypes.size());
     EXPECT_TRUE(capturedConnectionTypes[0].first);
     EXPECT_EQ(testConnection, capturedConnectionTypes[0].second);
   }
 }