Issue 5142 - Convert Element Hiding to C++

compiled/ElemHide.cpp

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present 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 19 matching lines @@
 void ElemHide::Clear()
 {
   mFilters.clear();
   mExceptions.clear();
   mFiltersByDomain.clear();
   mKnownExceptions.clear();
 }
 
 namespace
 {
-const ActiveFilter::DomainMap defaultDomains = {

[sergei, 2018/01/25 14:52:40]

Could you please add indentation here?

[hub, 2018/01/25 16:55:09]

Done.

-  { ActiveFilter::DEFAULT_DOMAIN, true }
-};
-}

[sergei, 2018/01/25 14:52:39]

and a new line here.

[hub, 2018/01/25 16:55:07]

Done.

-void ElemHide::AddToFiltersByDomain(ElemHideBase& filter)
-{
-  const auto* domains = filter.GetDomains();
+  const ActiveFilter::DomainMap defaultDomains =
+  {
+    { ActiveFilter::DEFAULT_DOMAIN, true }
+  };
+}
+
+void ElemHide::AddToFiltersByDomain(const ElemHideBasePtr& filter)
+{
+  const auto* domains = filter->GetDomains();
   if (!domains)
     domains = &defaultDomains;
 
-  DependentString text(filter.GetText());
+  DependentString text(filter->GetText());
   for (const auto& domain : *domains)
   {
     auto& filters = mFiltersByDomain[domain.first];
     if (domain.second)
-      filters[text] = ElemHideBasePtr(&filter);
+      filters[text] = filter;
     else
       filters[text] = ElemHideBasePtr();
   }
 }
 
 void ElemHide::Add(ElemHideBase& filter)
 {
   // we must ensure we have the right class.
   // This is an error, but we might get Invalid filters.
   if (!filter.As<ElemHideBase>())
     return;
 
   DependentString text(filter.GetText());
   if (filter.mType == Filter::Type::ELEMHIDEEXCEPTION)
   {
     if (mKnownExceptions.find(text))
       return;
 
     auto selector = filter.GetSelector();
     mExceptions[selector].emplace_back(filter.As<ElemHideException>());
 
     // Selector is no longer unconditional
     auto entry = mUnconditionalSelectors.find(selector);
-    if (entry)
-    {
-      AddToFiltersByDomain(*mFilters[entry->second]);
+    if (entry && entry->second)
+    {
+      AddToFiltersByDomain(entry->second);
       mUnconditionalSelectors.erase(selector);
       mUnconditionalSelectorsCache.reset();
     }
     mKnownExceptions.insert(text);
   }
   else
   {
     if (mFilters.find(text))
       return;
 
     auto selector = filter.GetSelector();
     mFilters[text] = &filter;
     if (!((filter.GetDomains() && filter.GetDomains()->size()) ||
           mExceptions.find(selector)))
     {
       // The new filter's selector is unconditionally applied to all domains
-      mUnconditionalSelectors[selector] = DependentString(filter.GetText());
+      mUnconditionalSelectors[selector] = ElemHideBasePtr(&filter);
       mUnconditionalSelectorsCache.reset();
     }
     else
-      AddToFiltersByDomain(filter);
+      AddToFiltersByDomain(ElemHideBasePtr(&filter));
   }
 }
 
 void ElemHide::Remove(ElemHideBase& filter)
 {
   DependentString text(filter.GetText());
 
   auto selector = filter.GetSelector();
   auto exceptionFilter = filter.As<ElemHideException>();
   if (exceptionFilter)
@@ skipping 22 matching lines @@
     else
     {
       const auto* domains = filter.GetDomains();
       if (!domains)
         domains = &defaultDomains;
 
       for (const auto& domain : *domains)
       {
         auto& list = mFiltersByDomain[domain.first];
         list.erase(text);
       }

[sergei, 2018/01/25 14:52:40]

Just wonder whether we should also remove entries for domains with no filters.

[hub, 2018/01/25 16:55:08]

list is StringMap<> and erase() on it doesn't decrease size(). Until we fix
this, we can't know.

[sergei, 2018/01/26 15:48:47]

I think we should remove it. Despite it's not supported now, it makes sense to
not "leak" the memory for domains which are not used anymore, although it can be
a rare case.

In addition to that I think there is a tricky issue. Let's say we add a
`filter1` with "domain.value", then we add `filter2` with the the same
"domain-value", then we remove `filter1`, so the key, which is used in the
mFiltersByDomain, which is a `DependentString` pointing to a string held by
`filter1`, so it is a dangling `DependentString` now and when it is used next
time in `Map`, in particular as `entry->equals(some-key)` there will be access
violation. Could we do something with this?

[hub, 2018/01/26 20:41:59]

erase will release the filter (it resets to the default value). When we fix
StringMap we can choose to remove the entry in mFiltersByDomain.
Good call.
I should turn them into OwnedStringMap.

I'll need https://codereview.adblockplus.org/29680706/ for this to work though.

[sergei, 2018/01/29 13:47:14]

OK, I also thought that maybe we could just update the key (DependentString) to
the first filter from the "value" and if the size of "value" is zero then just
remove the entry, but the approach with OwnedStringMap looks also fine because
it seems harder to make a mistake :).

     }
 
     mFilters.erase(text);
   }
 }
 
 ElemHideException* ElemHide::GetException(const ElemHideBase& filter,
                                           DependentString& docDomain) const
 {
   auto exception = mExceptions.find(filter.GetSelector());
   if (!exception)
     return nullptr;
 
   auto& list = exception->second;
   for (auto iter = list.rbegin(); iter != list.rend(); iter++)
   {
     DependentString domain(docDomain);
-    if ((*iter)->IsActiveOnDomain(domain, DependentString()))

[sergei, 2018/01/25 14:52:40]

I would recommend to add JIC a test that *iter is not a nullptr, despite it
should never happen.

[sergei, 2018/01/25 14:52:41]

What about adding the sitekey as a function parameter with the default value
DependentString()?

[hub, 2018/01/25 16:55:07]

Good idea.

Done.

[hub, 2018/01/25 16:55:08]

Done.

+    if (*iter && (*iter)->IsActiveOnDomain(domain))
     {
       ElemHideExceptionPtr filter(*iter);
       return filter.release();
     }
   }
 
   return nullptr;
 }
 
 ElemHide_SelectorList* ElemHide::GetUnconditionalSelectors() const
 {
   if (!mUnconditionalSelectorsCache)
   {
     mUnconditionalSelectorsCache =
       intrusive_ptr<ElemHide_SelectorList>(new ElemHide_SelectorList(), false);
     annotate_address(mUnconditionalSelectorsCache.get(), "ElemHide_SelectorList");
     for (const auto& unconditional : mUnconditionalSelectors)
     {
       if (!(unconditional.is_deleted() || unconditional.is_invalid()))
       {
-        auto entry = mFilters.find(unconditional.second);

[sergei, 2018/01/25 14:52:39]

What about storing intrusive_ptr<Filter> in mUnconditionalSelectors instead of
DependenentString from a filter text? In this case there will not be the
necessity in searching among mFilters, the memory usage will be less, because
the size of DependentString is bigger than intrusive_ptr and it will be less
dangerous in regards that a filter can be deleted but the DependentString from
its text is still somewhere here.

[hub, 2018/01/25 16:55:08]

OK

+        auto entry = mFilters.find(unconditional.second->GetText());
         if (entry)
           mUnconditionalSelectorsCache->push_back(entry->second);
       }
     }
   }
   return intrusive_ptr<ElemHide_SelectorList>(mUnconditionalSelectorsCache).release();
 }
 
 ElemHide_SelectorList* ElemHide::GetSelectorsForDomain(const String& domain,
   Criteria criteria) const
@@ skipping 38 matching lines @@
     if (currentDomain.empty())
       break;
 
     auto nextDot = currentDomain.find(u'.');
     currentDomain = nextDot == String::npos ?
       u""_str : DependentString(currentDomain, nextDot + 1);
   }
 
   return selectors.release();
 }