Issue #3593, #1197- fix isolate management

include/AdblockPlus/JsEngine.h

Index: include/AdblockPlus/JsEngine.h
diff --git a/include/AdblockPlus/JsEngine.h b/include/AdblockPlus/JsEngine.h
index c82ae02d5b24cae3c40737c19286968388dc7615..0bb0b89df2d904d1845bb010a72a05bb885d4567 100644
--- a/include/AdblockPlus/JsEngine.h
+++ b/include/AdblockPlus/JsEngine.h
@@ -52,13 +52,28 @@ namespace AdblockPlus
   typedef std::tr1::shared_ptr<JsEngine> JsEnginePtr;
 
   /**
+   * Scope based isolate manager. Creates a new isolate instance on
+   * constructing and disposes it on destructing.
+   */
+  class ScopedV8Isolate

[Eric, 2015/08/05 22:29:16]

There's no need for this class. Member construction order is
declaration-from-first-to-last and destruction order is
declaration-from-last-to-first. Simply declare it first in 'JsEngine'.

See, for example,
http://stackoverflow.com/questions/2254263/order-of-member-constructor-and-de...
which quotes the standard.

Note: construction order is not specified by the order of initializers in the
construction declaration. The rationale for this is that there can be only one
destructor, no matter how the object is constructed. For consistency, all the
constructors must initialize in the same order.

[sergei, 2015/11/16 16:52:09]

Thanks for explanation but this class and construction order are not related.
This class is aimed to manage `v8::Isolate`.

+  {
+  public:
+    ScopedV8Isolate();
+    ~ScopedV8Isolate();
+  protected:
+    v8::Isolate* isolate;
+  };
+
+  /**
    * JavaScript engine used by `FilterEngine`, wraps v8.
+   *
+   * It's inherited from ScopedV8Isolate to have isolate disposed only after
+   * disposing of all objects which are using it.
    */

[Eric, 2015/08/05 22:29:16]

Keep this comment (after a rewrite) at the declaration of the isolate pointer,
and at its new location. It would be easy to rearrange the declaration order
(for some other reason) unless the rationale is recorded.

[sergei, 2015/11/16 16:52:09]

I've inherited because it's more reliable than the order of members. It looks
like there is no any convention and it's very easy to get something using
`v8::Isolate` accidentally declared before isolate manager.

Now it's a member, because it's injected in the constructor.

[Eric, 2015/11/17 21:27:43]

Whatever other reason you might want for it, arguing on the grounds of
reliability is a weak argument. Order of initialization is part of the C++
standard, and all major compilers are conformant.

If you've got some other such reason, document it. What you have documented
could be done better relying upon order of initialization.
"No convention"? If you mean convention for order of initialization, you are
correct that it's not a convention, but rather a mandatory requirement for
conformance with the standard.
I'm not claiming that the method here doesn't work. I am claiming that it is far
more verbose than needed.

-  class JsEngine : public std::tr1::enable_shared_from_this<JsEngine>
+  class JsEngine : public std::tr1::enable_shared_from_this<JsEngine>, protected ScopedV8Isolate
   {
     friend class JsValue;
     friend class JsContext;
-
   public:
     /**
      * Event callback function.
@@ -213,11 +228,9 @@ namespace AdblockPlus
 
   private:
     JsEngine();
-
     FileSystemPtr fileSystem;
     WebRequestPtr webRequest;
     LogSystemPtr logSystem;
-    v8::Isolate* isolate;

[Eric, 2015/08/05 22:29:16]

Declared fourth, not first. Just fix it here.

     V8ValueHolder<v8::Context> context;
     EventMap eventCallbacks;
   };