Issue 5870 - Implement the new ElemHideEmulation filter type

compiled/filter/ElemHideBase.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 36 matching lines @@
         delta++;
       else
         text[pos - delta] = text[pos];
     }
     selectorStart -= delta;
 
     text.reset(text, 0, len - delta);
   }
 
   static constexpr String::value_type ELEM_HIDE_DELIMITER[] = u"##";
-  static constexpr String::size_type ELEM_HIDE_DELIMITER_LEN = LENGTH_OF(ELEM_HIDE_DELIMITER);
+  static constexpr String::size_type ELEM_HIDE_DELIMITER_LEN = str_length_of(ELEM_HIDE_DELIMITER);
 
   static constexpr String::value_type ELEM_HIDE_EMULATION_DELIMITER[] = u"#?#";
-  static constexpr String::size_type ELEM_HIDE_EMULATION_DELIMITER_LEN = LENGTH_OF(ELEM_HIDE_EMULATION_DELIMITER);
+  static constexpr String::size_type ELEM_HIDE_EMULATION_DELIMITER_LEN = str_length_of(ELEM_HIDE_EMULATION_DELIMITER);
 
   static constexpr String::value_type OLD_PROPS_SELECTOR[] = u"[-abp-properties=";
-  static constexpr String::size_type OLD_PROPS_SELECTOR_LEN = LENGTH_OF(OLD_PROPS_SELECTOR);
+  static constexpr String::size_type OLD_PROPS_SELECTOR_LEN = str_length_of(OLD_PROPS_SELECTOR);
 
   static constexpr String::value_type PROPS_SELECTOR[] = u":-abp-properties(";
-  static constexpr String::size_type PROPS_SELECTOR_LEN = LENGTH_OF(PROPS_SELECTOR);
+  static constexpr String::size_type PROPS_SELECTOR_LEN = str_length_of(PROPS_SELECTOR);
 }
 
 ElemHideBase::ElemHideBase(Type type, const String& text, const ElemHideData& data)
     : ActiveFilter(type, text, false), mData(data)
 {
   if (mData.HasDomains())
     ParseDomains(mData.GetDomainsSource(mText), u',');
 }
 
 Filter::Type ElemHideBase::Parse(DependentString& text, ElemHideData& data, bool& needConversion)
 {
+  needConversion = false;
+
   StringScanner scanner(text);
 
   // Domains part
   bool seenSpaces = false;
   while (!scanner.done())
   {
     String::value_type next = scanner.next();
     if (next == u'#')
     {
       data.mDomainsEnd = scanner.position();
@@ skipping 72 matching lines @@
     String::size_type byte_len() const
     {
       return len() * sizeof(String::value_type);
     }
   };
 }
 
 // Convert filter from the old syntax to the new.
 DependentString ElemHideBase::ConvertFilter(String& text, String::size_type& at)
 {
+  Range prefix = {at, text.find(OLD_PROPS_SELECTOR, at, OLD_PROPS_SELECTOR_LEN)};
+  if (prefix.end == text.npos)
+    return DependentString(text);
+
   auto length = text.length();
-  Range prefix = {at, 0};
   Range suffix = {at, length};
-  prefix.end = text.find(OLD_PROPS_SELECTOR, at, OLD_PROPS_SELECTOR_LEN);

[sergei, 2018/02/12 12:53:18]

not important but perhaps it would be better to say
Range prefix = {at, text.find(OLD_PROPS_SELECTOR, at, OLD_PROPS_SELECTOR_LEN)};

[hub, 2018/02/12 18:14:39]

Done.

-  if (prefix.end != text.npos)

[sergei, 2018/02/12 12:53:19]

What about returning DependentString(text); here in order to reduce the
indentation below?Subjectively it also improves readability, BTW `suffix` and
`length` will be declared after that `if`.

[hub, 2018/02/12 18:14:39]

Done.

-  {
-    Range properties = { prefix.end + OLD_PROPS_SELECTOR_LEN, 0 };
-    String::value_type quote = 0;
-    bool closing = false;
-    for (auto index = properties.start;
-         index < length && !closing; index++)
-    {
-      auto c = text[index];
-      switch (c)
-      {
-      case u'"':
-      case u'\'':
-        if (quote == 0)
-        {
-          // syntax error: we already have a quoted section.
-          if (properties.end)
-            return DependentString();
-
-          quote = c;
-          properties.start = index + 1;
-        }
-        else if (quote == c)
-        {
-          // end of quoted.
-          quote = 0;
-          properties.end = index;
-        }
-        break;
-      case u']':
-        if (quote == 0)
-        {
-          if (properties.end == 0)
-            return DependentString();
-          suffix.start = index + 1;
-          closing = true;

[sergei, 2018/02/12 12:53:19]

What about using of suffix.start instead of `closing`? We could initialize it to
zero. In addition to that if suffix.start is still zero after finishing the loop
then the filter is malformed because there is no closing ']'. Continuing without
taking it into account can result in bad sizes below.

[hub, 2018/02/12 18:14:39]

suffix.start is initialised to `at`. So I'll use that. And I'll return an
invalid string.

-        }
-        break;
-      default:
-        break;
-      }
-    }
-
-    String::size_type delimiter = text.find(ELEM_HIDE_DELIMITER, 0,
-                                            ELEM_HIDE_DELIMITER_LEN);
-    // +1 for the replacement of "##" by "#?#"
-    if (delimiter != text.npos)
-      at++;

[sergei, 2018/02/12 12:53:20]

BTW, if there is no such delimiter then is it a malformed filter string?

[hub, 2018/02/12 18:14:40]

no. #@# is valid too. We could check if it is #@# eventually but I don't think
it is part of this.

-    auto new_len = prefix.len() + suffix.len() + properties.len()
-      + PROPS_SELECTOR_LEN + 1 + at;

[sergei, 2018/02/12 12:53:19]

What about
at + prefix.len() + PROPS_SELECTOR_LEN + properties.len() + 1 /* ')' */ +
suffix.len()
?

[hub, 2018/02/12 18:14:39]

Done.

-

[sergei, 2018/02/12 12:53:18]

Could you please add the assert here that
new_len + 1 == length || (delimiter == text.npos && new_len + 2 == length)
just in case? Despite there are tests ensuring that nothing is lost, it just
looks scaring :)).

[hub, 2018/02/12 18:14:40]

Done.

-    DependentString converted(text, 0, new_len);
-
-    if (suffix.len())
-    {
-      new_len -= suffix.len();
-      std::memmove(converted.data() + new_len,
-                   text.data() + suffix.start,
-                   suffix.byte_len());
-    }
-    new_len--;
-    // here we need to move the properties before inserting the ')'
-    auto parens = new_len;
-    if (properties.len())
-    {
-      new_len -= properties.len();
-      std::memmove(converted.data() + new_len,
-                   text.data() + properties.start, properties.byte_len());
-    }
-    converted[parens] = u')';
-
-    new_len -= PROPS_SELECTOR_LEN;
+  Range properties = { prefix.end + OLD_PROPS_SELECTOR_LEN, 0 };
+  String::value_type quote = 0;
+  for (auto index = properties.start;
+       index < length && (suffix.start == at); index++)
+  {
+    auto c = text[index];
+    switch (c)
+    {
+    case u'"':
+    case u'\'':
+      if (quote == 0)
+      {
+        // syntax error: we already have a quoted section.
+        if (properties.end)
+          return DependentString();
+
+        if (properties.start != index)
+          return DependentString();
+
+        quote = c;
+        properties.start = index + 1;
+      }
+      else if (quote == c)
+      {
+        // end of quoted.
+        quote = 0;
+        properties.end = index;
+      }
+      break;
+    case u']':
+      if (quote == 0)
+      {
+        if (properties.end == 0)
+          return DependentString();
+        if (properties.end + 1 != index)
+          return DependentString();
+        suffix.start = index + 1;
+      }
+      break;
+    default:
+      break;
+    }
+  }
+
+  if (suffix.start == at)

[sergei, 2018/02/27 10:56:53]

Just for reference, I think it (what the whole for-loop above is doing) can be
written in a simpler form, but since it's some sort of interim function it's
fine.

+    return DependentString();
+
+  String::size_type delimiter = text.find(ELEM_HIDE_DELIMITER, 0,
+                                          ELEM_HIDE_DELIMITER_LEN);
+  // +1 for the replacement of "##" by "#?#"
+  if (delimiter != text.npos)
+    at++;
+  auto new_len = at + prefix.len() + PROPS_SELECTOR_LEN + properties.len() + 1 /* ) */ + suffix.len();
+
+  assert2(new_len + 1 == length || (delimiter == text.npos && new_len + 2 == length), u"Inconsistent length in filter conversion."_str);

[sergei, 2018/02/27 10:56:53]

not important just for reference
length == new_len + (delimiter == text.npos ? 2 : 1)
looks shorted and maybe even more logical.

[hub, 2018/02/27 13:32:30]

Acknowledged.

+
+  DependentString converted(text, 0, new_len);
+
+  if (suffix.len())
+  {
+    new_len -= suffix.len();
     std::memmove(converted.data() + new_len,

[sergei, 2018/02/12 12:53:18]

logically it should be memcpy but since memmove does not modify the memory
accessed by `src` argument it's fine.

The same for ELEM_HIDE_EMULATION_DELIMITER below.

[hub, 2018/02/12 18:14:39]

You are right. Changing it to memcpy().

-                 PROPS_SELECTOR,
-                 PROPS_SELECTOR_LEN * sizeof(String::value_type));
-    if (prefix.len())
-    {
-      new_len -= prefix.len();
-      std::memmove(converted.data() + new_len,
-                   text.data() + prefix.start, prefix.byte_len());
-    }
-
-    if (delimiter != String::npos)
-    {
-      std::memmove(converted.data() + delimiter, ELEM_HIDE_EMULATION_DELIMITER,
-                   ELEM_HIDE_EMULATION_DELIMITER_LEN * sizeof(String::value_type));
-    }
-
-    return converted;
-  }
-
-  return DependentString(text);
+                 text.data() + suffix.start,
+                 suffix.byte_len());
+  }
+  new_len--;
+  // here we need to move the properties before inserting the ')'
+  auto parens = new_len;
+  if (properties.len())
+  {
+    new_len -= properties.len();
+    std::memmove(converted.data() + new_len,
+                 text.data() + properties.start, properties.byte_len());
+  }
+  converted[parens] = u')';
+
+  new_len -= PROPS_SELECTOR_LEN;
+  std::memcpy(converted.data() + new_len,
+              PROPS_SELECTOR,
+              PROPS_SELECTOR_LEN * sizeof(String::value_type));
+  if (prefix.len())
+  {
+    new_len -= prefix.len();
+    std::memmove(converted.data() + new_len,
+                 text.data() + prefix.start, prefix.byte_len());
+  }
+
+  if (delimiter != String::npos)
+  {
+    std::memcpy(converted.data() + delimiter, ELEM_HIDE_EMULATION_DELIMITER,
+                ELEM_HIDE_EMULATION_DELIMITER_LEN * sizeof(String::value_type));
+  }
+
+  return converted;
 }
 
 namespace
 {
   static constexpr String::value_type OPENING_CURLY_REPLACEMENT[] = u"\\7B ";
   static constexpr String::value_type CLOSING_CURLY_REPLACEMENT[] = u"\\7D ";
-  static constexpr String::size_type CURLY_REPLACEMENT_SIZE = LENGTH_OF(OPENING_CURLY_REPLACEMENT);
+  static constexpr String::size_type CURLY_REPLACEMENT_SIZE = str_length_of(OPENING_CURLY_REPLACEMENT);
 
   OwnedString EscapeCurlies(String::size_type replacementCount,
                             const DependentString& str)
   {
     OwnedString result(str.length() + replacementCount * (CURLY_REPLACEMENT_SIZE - 1));
 
     String::value_type* current = result.data();
     for (String::size_type i = 0; i < str.length(); i++)
     {
       switch(str[i])
@@ skipping 43 matching lines @@
       if (item.second && !item.first.empty())
       {
         if (!result.empty())
           result.append(u',');
         result.append(item.first);
       }
     }
   }
   return result;
 }