Issue 4612 - enable AA on first run and make automatic adding of any subscription optional

test/FilterEngine.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 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
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #include "BaseJsTest.h"
 #include <thread>
 
 using namespace AdblockPlus;

[Felix Dahlke, 2016/11/22 17:37:16]

Nit: Empty line after this to be in line with the other files?

[sergei, 2016/11/23 14:20:50]

Done.

+
 namespace
 {
   typedef std::shared_ptr<AdblockPlus::FilterEngine> FilterEnginePtr;
 
   class VeryLazyFileSystem : public LazyFileSystem
   {
   public:
     StatResult Stat(const std::string& path) const
     {
       return StatResult();
@@ skipping 77 matching lines @@
     // https://issues.adblockplus.org/ticket/1378.
     int& timesCalled;
   };
 
   // Workaround for https://issues.adblockplus.org/ticket/1397.
   void NoOpUpdaterCallback(const std::string&) {}
 
   class FilterEngineWithFreshFolder : public ::testing::Test
   {
   protected:
-    FileSystemPtr m_fileSystem;

[Felix Dahlke, 2016/11/22 17:37:17]

From https://adblockplus.org/coding-style: "No hungarian notation, no special
variable name prefixes or suffixes denoting type or scope."

[sergei, 2016/11/23 14:20:50]

I thought we finally started to use #pragma once and prefixes for members of
classes in C++.
https://codereview.adblockplus.org/29333474/diff/29345688/compiled/String.h

This two things give a lot of help.

[Felix Dahlke, 2016/12/02 12:26:34]

#pragma is not restricted by the code style and we've started to use it at least
in adblockplusie (plus, I don't understand how that is related). Hungarian
notation is not something we started using to my knowledge (or should use unless
there's a really good reason IMO). But since you addressed it, we can discuss
that elsewhere (and maybe it is time to disconnect our C++ coding style from
Mozilla's, to make it easier to understand and evolve).

-    std::weak_ptr<JsEngine> m_jsEngine;
+    FileSystemPtr fileSystem;
+    std::weak_ptr<JsEngine> weakJsEngine;
 
     void SetUp() override
     {
-      m_fileSystem.reset(new DefaultFileSystem());
-      // Since there are neither in memory FS nor functionality to work with

[Felix Dahlke, 2016/11/22 17:37:16]

Wording nit: "Since there 'is' neither 'in-memory' FS nor functionality to work
with directories',' [...]"

[sergei, 2016/11/23 14:20:50]

Done.

+      fileSystem.reset(new DefaultFileSystem());
+      // Since there is neither in memory FS nor functionality to work with
       // directories use the hack: manually clean the directory.

[Felix Dahlke, 2016/11/22 17:37:16]

Do you mean "functionality to work with _a different directory_", maybe?

[sergei, 2016/11/23 14:20:50]

No, it's possible to configure working directory of DefaultFileSystem, I mean a
cross platform API to work with directories, for instance it would be useful to
have a scoped temporary directory or at least some functions to get a path to
temporary writable directory, create and remove directories and in addition
generate a temporary folder name. I have not added it here because I'm sure it
should be in another commit.

[Felix Dahlke, 2016/12/02 12:26:34]

Acknowledged.

       removeFileIfExists("patterns.ini");
       removeFileIfExists("prefs.json");
     }
     JsEnginePtr createJsEngine(const AppInfo& appInfo = AppInfo())
     {
       auto jsEngine = JsEngine::New(appInfo);

[Felix Dahlke, 2016/11/22 17:37:16]

I guess my C++ might be getting rusty, but why assign this to a temporary local
variable here, rather than directly to the member variable?

[sergei, 2016/11/23 14:20:50]

JsEngine::New returns std::shared_ptr but the type of member is std::weak_ptr,
it's a part of the hack with sleep_for used in removeFileIfExists. It is to deal
with the following race condition.

Currently we have issues with circular references
https://issues.adblockplus.org/ticket/3594 and with detached threads
https://issues.adblockplus.org/ticket/3595. Current version at master allows
different tests to influence another tests, for example, when the next test is
running, writing thread from a previous test can corrupt, e.g. patterns.ini.
Eliminating of circular references relieves a bit but not completely.
Asynchronous operation will be ignored if there is already no JsEngine, however
there is still a race condition, thread of asynchronous operation still can keep
JsEngine alive even when we have already no strong references to it. sleep_for
gives an asynchronous operation some time to finish its work, so afterwards we
can remove files created by current test. I have complicated the code even more
:) because it should take into account not only jsEngine of current test but
also async operations from previous tests (actually it happens too often on my
machine that patterns.ini is blocked by some previous test).

All these hacks will go away as the life span of asynchronous operations is no
longer than the life span of both JsEngine and FilterEngine.

[Felix Dahlke, 2016/12/02 12:26:34]

Acknowledged.

-      m_jsEngine = jsEngine;
-      jsEngine->SetFileSystem(m_fileSystem);
+      weakJsEngine = jsEngine;
+      jsEngine->SetFileSystem(fileSystem);
       jsEngine->SetWebRequest(AdblockPlus::WebRequestPtr(new LazyWebRequest()));
       jsEngine->SetLogSystem(AdblockPlus::LogSystemPtr(new LazyLogSystem()));
       return jsEngine;
     }
     void TearDown() override
     {
       removeFileIfExists("patterns.ini");
       removeFileIfExists("prefs.json");
-      m_fileSystem.reset();
+      fileSystem.reset();
     }
     void removeFileIfExists(const std::string& path)
     {
+      // Hack: allow IO to finish currently running operations, in particular
+      // writing into files. Otherwise we get "Permission denied".
+      auto safeRemove = [this, &path]()->bool
       {
-        // Hack: allow IO to finish currently running operations, in particular
-        // writing into files. Otherwise we get Permission denied.

[Felix Dahlke, 2016/11/22 17:37:16]

Wording nit: Maybe put "Permission denied" in quotes, or use a lower-case "p"?

[sergei, 2016/11/23 14:20:50]

Done.

-        int i = 100;
-        while (i-- > 0 && m_jsEngine.lock()) {

[Felix Dahlke, 2016/11/22 17:37:16]

Nit: Opening brace on its own line please (might as well skip the braces here of
course since there's just one statement in the body).

[sergei, 2016/11/23 14:20:50]

Done.

-          std::this_thread::sleep_for(std::chrono::seconds(2));
+        try
+        {
+          if (fileSystem->Stat(path).exists)
+            fileSystem->Remove(path);
+          return true;
         }
-      }
-      if (m_fileSystem->Stat(path).exists)
-        m_fileSystem->Remove(path);
+        catch (...)
+        {
+          return false;
+        }
+      };
+      int i = 5;
+      while ((i-- > 0 && weakJsEngine.lock()) || !safeRemove())
+        std::this_thread::sleep_for(std::chrono::seconds(2));
     }
   };
 }
 
 TEST_F(FilterEngineTest, FilterCreation)
 {
   AdblockPlus::FilterPtr filter1 = filterEngine->GetFilter("foo");
   ASSERT_EQ(AdblockPlus::Filter::TYPE_BLOCKING, filter1->GetType());
   AdblockPlus::FilterPtr filter2 = filterEngine->GetFilter("@@foo");
   ASSERT_EQ(AdblockPlus::Filter::TYPE_EXCEPTION, filter2->GetType());
@@ skipping 406 matching lines @@
 
   ASSERT_TRUE(filterEngine->IsElemhideWhitelisted(
       "http://example.com",
       documentUrls1));
 
   ASSERT_FALSE(filterEngine->IsElemhideWhitelisted(
       "http://example.co.uk",
       documentUrls1));
 }
 
-TEST(NewFilterEngineTest, MemoryLeak_NoCircularReferences)
-{
-  std::weak_ptr<AdblockPlus::JsEngine> weakJsEngine;
-  {
-    auto jsEngine = AdblockPlus::JsEngine::New();
-    jsEngine->SetFileSystem(AdblockPlus::FileSystemPtr(new LazyFileSystem()));
-    jsEngine->SetWebRequest(AdblockPlus::WebRequestPtr(new LazyWebRequest()));
-    jsEngine->SetLogSystem(AdblockPlus::LogSystemPtr(new LazyLogSystem()));
-    auto filterEngine = FilterEnginePtr(new AdblockPlus::FilterEngine(jsEngine));
-  }
-  EXPECT_FALSE(weakJsEngine.lock());
-}
-
 TEST_F(FilterEngineWithFreshFolder, LangAndAASubscriptionsAreChosenOnFirstRun)
 {
   AppInfo appInfo;
   appInfo.locale = "zh";
   const std::string langSubscriptionUrl = "https://easylist-downloads.adblockplus.org/easylistchina+easylist.txt";
   auto jsEngine = createJsEngine(appInfo);
   auto filterEngine = FilterEnginePtr(new AdblockPlus::FilterEngine(jsEngine));
   const auto subscriptions = filterEngine->GetListedSubscriptions();
   ASSERT_EQ(2u, subscriptions.size());
   const auto aaUrl = filterEngine->GetPref("subscriptions_exceptionsurl")->AsString();

[Felix Dahlke, 2016/11/22 17:37:16]

Hm, maybe we should hard code the URL as well here, same as with the Chinese
subscription? No strong opinion, up to you.

[sergei, 2016/11/23 14:20:50]

I will remove aaUrl and use Subscription::IsAA instead in an upcoming commit.

[Felix Dahlke, 2016/12/02 12:26:34]

Acknowledged.

   SubscriptionPtr aaSubscription;
   SubscriptionPtr langSubscription;
   if (subscriptions[0]->GetProperty("url")->AsString() == aaUrl)
   {
     aaSubscription = subscriptions[0];
     langSubscription = subscriptions[1];
-  } else if (subscriptions[1]->GetProperty("url")->AsString() == aaUrl)

[Felix Dahlke, 2016/11/22 17:37:16]

Nit: Closing brace on its own line please.

[sergei, 2016/11/23 14:20:50]

Done.

+  }
+  else if (subscriptions[1]->GetProperty("url")->AsString() == aaUrl)
   {
     aaSubscription = subscriptions[1];
     langSubscription = subscriptions[0];
   }
   ASSERT_NE(nullptr, aaSubscription);
   ASSERT_NE(nullptr, langSubscription);
   EXPECT_EQ(aaUrl, aaSubscription->GetProperty("url")->AsString());
   EXPECT_EQ(langSubscriptionUrl, langSubscription->GetProperty("url")->AsString());
 }
 
 TEST_F(FilterEngineWithFreshFolder, DisableSubscriptionsAutoSelectOnFirstRun)
 {
   auto jsEngine = createJsEngine();
   FilterEngine::Prefs preSettings;
   preSettings["first_run_subscription_auto_select"] = jsEngine->NewValue(false);
   auto filterEngine = FilterEnginePtr(new AdblockPlus::FilterEngine(jsEngine, preSettings));
   const auto subscriptions = filterEngine->GetListedSubscriptions();
   EXPECT_EQ(0u, subscriptions.size());
 }