Issue #1652, #3456 - Replace 's_asyncWebBrowser2' with thread-safe facilities

src/plugin/Instances.h

 /*
  * 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/>.
  */
 
 #ifndef _INSTANCES_H_
 #define _INSTANCES_H_
 
+#include <map>
 #include <mutex>
 #include <set>
 
 /**
+ * A base class for a synchronized map from threads to BHO instances.
+ *
+ * This is a base class for 'CurrentThreadMap', defined in PluginClass.cpp.
+ * It's separated out here for testability.
+ * This class should not be used as polymorphic base class, as it has no virtual base class.
+ *
+ * The member functions here not simply forwarded versions of the container functions.
+ * Rather, they are specialized for tracking BHO calls to SetSite().
+ * Their semantics allow for verification that the call pattern is as expected.
+ *
+ * The key to the map (in the subclass) is the thread ID, thus operations are serialized on a per-key basis.
+ * Calls to SetSite() bracket all other calls, so on a per-key basis
+ *   the order of operations is always either (insert / find-success / erase) or (find-failure).
+ * The library guarantees for std::map seem to indicate that operations on different keys
+ *   do not interfer with each other, but there's some ambiguity there.
+ * This class is synchronized as a matter of defensive programming.
+ */
+template<class Key, class T, T nullValue>
+class SyncMap
+{
+  typedef std::lock_guard<std::mutex> SentryType;
+
+  /**
+   * Underlying map container
+   */
+  std::map<Key, T> idMap;
+
+  /**
+   * Synchronization primitive
+   */
+  mutable std::mutex mutex;
+
+public:
+  /**
+   * Returns true if (as expected) no key of value 'id' was present.
+   * Returns false otherwise.
+   */
+  bool AddIfAbsent(Key id, T p)
+  {
+    SentryType sentry(mutex);
+    auto it = idMap.insert(std::make_pair(id, p));
+    return it.second;
+    // Assert it.second==true implies the insertion took place,
+    //  which means there was no key of value 'id' already present.
+  }
+
+  /**
+   * Returns true if (as expected) a key of value 'id' was already present.
+   * Returns false otherwise.
+   */
+  bool RemoveIfPresent(Key id)
+  {
+    SentryType sentry(mutex);
+    auto it = idMap.find(id);
+    if (it == idMap.end())
+    {
+      return false;
+    }
+    idMap.erase(it);
+    return true;
+  }
+
+  /**
+   * Returns a non-nullValue if a key of value 'id' is present.
+   * Returns nullValue otherwise.
+   */
+  T Locate(Key id) const
+  {
+    SentryType sentry(mutex);
+    auto it = idMap.find(id);
+    return (it != idMap.end()) ? it->second : nullValue;
+  }
+};
+
+/**
  * Template class for a synchronized set of BHO instances

[sergei, 2016/01/04 15:34:31]

Do you mind to change the comment a bit?

If it's a class for a set of BHO instances then it should not be a template
class. However it can be useful to know that it's used for example to store
pointers to instances of BHO and I don't mind to have it as a template class.

[Eric, 2016/01/04 17:49:20]

I don't see a need, given what you've said so far.
(1) It's a template so that it can be instantiated for unit tests. It's
basically impossible to instantiate 'CPluginClass' outside the whole plugin.
This is a completely sufficient reason to use a template declaration.

(2) We only have one class that acts as a BHO. This class could be used for any
BHO class.

[sergei, 2016/01/29 15:28:55]

I disagree that the description of generic template class SyncSet can say that
it's "set of BHO instances".
I'm pretty sure that it's possible to instantiate `CPluginClass` in tests. I
have just done it:
  ATL::CComObject<CPluginClass>* impl;
  ATL::CComObject<CPluginClass>::CreateInstance(&impl);

[Eric, 2016/01/29 16:16:12]

Your objection here is that you don't like the template. Since you haven't
identified a defect in the code, I'll assume you haven't found one.

You're arguing style. I find it better to use a template class because it
promotes better testing. You don't find value in that.

I reject your request to rewrite this without a template. I consider it a step
backwards.

[Oleksandr, 2016/02/01 18:03:13]

I think Sergei was merely talking about the comment here, not the
implementation. Why say it is a class for BHO instances when it can be so much
more. We are using it here as well https://codereview.adblockplus.org/29330403/
for example.

[Eric, 2016/02/03 14:57:27]

I don't believe so. He has said more that once that he would prefer it not be a
template class. If it's not a template class, you get inferior unit testing.
Because we're never going to use it for anything more.

This class is specifically targeted at the behavior of IE and the way it calls
'SetSite()' on BHO instances. Its behaviors aren't so weird that it couldn't be
used elsewhere, but it was written to deal with BHO semantics, and it's the only
way we'll use it. I see no downside in documenting the class with a
less-than-fully general comment that states very specifically the environment
the class is written for.
In fact we are _not_ using it there. The present class is 'SyncSet'. The one
there is 'SyncMap'. The use different underlying containers. What they do have
in common is similar behaviors specialized to be containers of BHO objects and
managed through 'SetSite()' as called by IE.

- *
  */
 template<class T, T nullValue>
 class SyncSet
 {
   typedef std::lock_guard<std::mutex> SentryType;
 
   /**
    * Underlying set container
    */
   std::set<T> set;
 
   /**
    * Synchronization primitive
    */
-  std::mutex mutex;

[sergei, 2016/01/04 15:34:31]

I guess it should be `mutable` because it can be used from constant methods, see
below the notes regarding which methods can be constant.

[Eric, 2016/01/04 17:49:20]

Done.

The only 'const' method, though, is 'Empty()'. It's conceptually correct,
though, to declare this member as 'mutable'.

+  mutable std::mutex mutex;
 
 public:
   /**
    * \return
    *   true if (as expected) the argument was not already a member of the set
    *   false otherwise
    *
    * \par Postcondition
    *   - argument is a member of the set
    */
   bool InsertIfAbsent(T p)

[sergei, 2016/01/04 15:34:30]

it would be better to have instead methods which accept `const T&` and `T&&`,
but only `const T&` is enough.

[Eric, 2016/01/04 17:49:20]

If this were generic library code, we'd want declarations for all of these. As
it is, this is *not* library code, and a simple declaration is adequate.

There's only one declaration in the present code. In the plugin, 'T' is
instantiated with a pointer, which you want to pass by value. You don't want a
reference here at all.

[sergei, 2016/01/29 15:28:55]

I disagree with that attitude.
I think it should be either BHOSyncSet without any templates or there should be
another interface.

[Eric, 2016/01/29 16:16:12]

You haven't identified a defect here.

You have not made any argument about how using a reference would improve the
code.

I have made an argument about how using a reference would diminish the code. You
have not responded to that point.

Change request rejected.

[Oleksandr, 2016/02/01 18:03:13]

I don't have a strong opinion on this, but I agree that we should treat this
class as a generic one, not BHO specific one.

[Eric, 2016/02/03 14:57:27]

Answered above.

   {
     SentryType sentry(mutex);
     auto it = set.find(p);
     if (it != set.end())
     {
       return false;
     }
     set.insert(p);
     return true;
   }
 
   /**
    * \return
    *   true if (as expected) the argument was already a member of the set
    *   false otherwise
    *
    * \par Postcondition
    *   - argument is not a member of the set
    */
   bool EraseWithCheck(T p)

[sergei, 2016/01/04 15:34:30]

The argument should be `const T&`.

[Eric, 2016/01/04 17:49:20]

See above. Pass by value is desirable.

   {
     SentryType sentry(mutex);
     auto it = set.find(p);
     if (it == set.end())
     {
       return false;
     }
     set.erase(it);
     return true;
   }
 
   /**
    * \param f
    *   Search criterion is a simple predicate function 
    *
    * \return
    *   - If no member of the set meets the search criterion, the null value
    *   - Otherwise some member of the set that meets the search criterion
    */
   T LinearSearch(std::function<bool(T)> f)

[sergei, 2016/01/04 15:34:30]

I would like to have a constant reference to T as the argument of the predicate,
thus `std::function<bool(const T&)>`. In the current use we are working with the
pointers and the copying of the value does not hurt but it would be wise to
avoid copying here.

If we change the method to a constant then we will have to deal with the const
references.

[sergei, 2016/01/04 15:34:30]

Should it be a constant method?

[sergei, 2016/01/04 15:34:31]

it would be also good to pass the predicate as a constant reference to the
function.

[Eric, 2016/01/04 17:49:20]

It's only ever used with lambda functions, so an lvalue reference need not even
work. This is a place where we can let the compiler do the right thing. No need
to constrain it with excessive declarations.

[Eric, 2016/01/04 17:49:20]

No.

If we wanted a second, 'const'-aware definition, then you could do that. We only
need one, and it needs to be non-constant.

[Eric, 2016/01/04 17:49:20]

We're passing 'T' by value everywhere.

[sergei, 2016/01/29 15:28:55]

It's just a dirty code, isn't it clear?

[sergei, 2016/01/29 15:28:55]

This method does not change the main sate of the class, so it should be
constant. Specifically to support it std::mutex has `mutable`.

[Eric, 2016/01/29 16:16:12]

The whole discussion of 'const' declaration here has devolved to bikeshedding.
It does not reveal any defect in the code, merely a way that this class is not
as general as a library class would be.

On 2016/01/29 15:28:55, sergei wrote:
No. You have not sufficiently explained why it's dirty.

We could declare the argument 'f' to have constant effect, which would allow
this function to have constant effect. But then we'd need this function in
addition. What you're recommending just feels like code bloat.

On 2016/01/29 15:28:55, sergei wrote:
The argument 'f' does not have constant effect. Therefore it can change the
state of the members of the container.

   {
     SentryType sentry(mutex);
-    for (auto it : set)

[sergei, 2016/01/04 15:34:30]

JIC, `it` does not refer to an iterator here, I would like to have a different
name, for example `value`.

In general it's already unwrapped iterator, thus a reference to a value, but
this particular construction creates a copy of the value, to avoid it we should
add `&`.

In summary I would write it as `for (const auto& value : set)`.

[Eric, 2016/01/04 17:49:20]

OK.
Again, there's no need to overconstrain the compiler. They're rather good at
picking efficient type instantiations these days.

+    for (auto member : set)
     {
-      bool b = f(it);
+      bool b = f(member);
       if (b)
       {
-        return it;
+        return member;
       }
     }
     return nullValue;
   }
 
   bool Empty() const
   {
+    SentryType sentry(mutex);
     return set.empty();

[sergei, 2016/01/04 15:34:30]

SentryType sentry(mutex);
is missed here

[Eric, 2016/01/04 17:49:20]

This function is only instantiated in the unit tests, but I added the statement
anyway. It doesn't hurt.

   }
 };
 
-#endif
+#endif