Issue 4125 - [emscripten] Convert filter classes to C++

compiled/Filter.cpp

 #include "Filter.h"
 #include "CommentFilter.h"
 #include "InvalidFilter.h"
 #include "RegExpFilter.h"
 #include "BlockingFilter.h"
 #include "WhitelistFilter.h"
 #include "ElemHideBase.h"
 #include "ElemHideFilter.h"
 #include "ElemHideException.h"
 #include "CSSPropertyFilter.h"
 #include "StringMap.h"
 
 namespace
 {
   StringMap<Filter*> knownFilters(8192);

[sergei, 2016/06/16 21:16:28]

What about having another object which holds knownFilters and has
MakeFilterFromText which is currently static Filter::FromText?

[sergei, 2016/06/16 21:16:31]

Does it add a big overhead to store intrusive_ptr instead of a raw pointer
there?

[Wladimir Palant, 2016/12/06 10:47:22]

Sure, we can probably do that - but at the moment I'd rather replicate the
existing APIs as much as possible, we'll need significant adjustments to
JavaScript code as it is already.

On 2016/06/16 21:16:31, sergei wrote:
Overhead is not the point - if we store intrusive_ptr here we will leak the
filter. The idea is that we only store the filter here for as long as it is
being used. Once no references to that filter are remaining (meaning no
references from FilterStorage in particular) and the filter is released it
should be removed from knownFilters (see TODO comment in the destructor below).

 
   void NormalizeWhitespace(DependentString& text)
   {
     String::size_type start = 0;
     String::size_type end = text.length();
 
     // Remove leading spaces and special characters like line breaks
     for (; start < end; start++)
       if (text[start] > ' ')
         break;
@@ skipping 22 matching lines @@
     // Remove trailing spaces
     for (; end > 0; end--)
       if (text[end - 1] != ' ')
         break;
 
     // Set new string boundaries
     text.reset(text, start, end - start);
   }
 }
 
-Filter::Filter(const String& text)
-    : mText(text)
+Filter::Filter(Type type, const String& text)
+    : mType(type), mText(text)
 {
   annotate_address(this, "Filter");
 }
 
 Filter::~Filter()
 {
-  // TODO: This should be removing from knownFilters

[Wladimir Palant, 2016/12/06 10:47:24]

I addressed this TODO comment so that we can stop leaking filters below.

+  knownFilters.erase(mText);
 }
 
 OwnedString Filter::Serialize() const
 {
   OwnedString result(u"[Filter]\ntext="_str);
   result.append(mText);
   result.append(u'\n');
   return result;
 }
 
@@ skipping 13 matching lines @@
   DependentString error;
 
   Filter::Type type = CommentFilter::Parse(text);
   if (type == Filter::Type::UNKNOWN)
     type = ElemHideBase::Parse(text, data.elemhide);
   if (type == Filter::Type::UNKNOWN)
     type = RegExpFilter::Parse(text, error, data.regexp);
 
   auto knownFilter = knownFilters.find(text);
   if (knownFilter)
+  {
+    knownFilter->second->AddRef();
     return knownFilter->second;
+  }
 
   FilterPtr filter;
   switch (type)
   {
     case Filter::Type::COMMENT:
       filter = new CommentFilter(text);
       break;
     case Filter::Type::INVALID:
       filter = new InvalidFilter(text, error);
       break;
@@ skipping 19 matching lines @@
       return nullptr;
   }
 
   // This is a hack: we looked up the entry using text but create it using
   // filter->mText. This works because both are equal at this point. However,
   // text refers to a temporary buffer which will go away.
   enter_context("Adding to known filters");
   knownFilter.assign(filter->mText, filter.get());
   exit_context();
 
-  // TODO: We intentionally leak the filter here - currently it won't be used
-  // for anything and would be deleted immediately.

[sergei, 2016/06/16 21:16:33]

Actually, we should have a convention that when we return a raw pointer the
caller is responsible for freeing it, we should call AddRef above for "return
knownFilter->second;" and GetKnownFilter should also call AddRef.

In addition we could have intrusive_ptr::detach() which returns raw pointer and
sets internal pointer member to nullptr and intrusive_ptr::attach which takes
the ownership without calling of AddRef (of course attach calls ReleaseRef for
the previously held value if it's valid).

BTW, here is one of the case when intrusive_ptr::operator T*() is dangerous.

[Wladimir Palant, 2016/12/06 10:47:20]

Ok, let's implement proper semantics.
Actually, if our convention is that raw pointers are already addref'ed then we
just should never addref them when taking ownership of them. I called the other
method intrusive_ptr::release() for consistency with unique_ptr.
Yes, we have intrusive_ptr::release() now so let's get rid of it.

-  filter->AddRef();
-
-  return filter;
+  return filter.release();
 }
-
-Filter* Filter::GetKnownFilter(const String& text)

[sergei, 2016/06/16 21:16:30]

It seems this method is not used, do we really need it?

[Wladimir Palant, 2016/12/06 10:47:18]

It isn't used in this form of course - the original code was exposing
Filter.knownFilters so you could do `text in Filter.knownFilters` instead. I see
however that no code appears to be doing that, everything is rather calling
Filter.fromText() blindly. So let's remove this method and see how far we get
with it.

-{
-  auto it = knownFilters.find(text);
-  if (it)
-    return it->second;
-  else
-    return nullptr;
-}