Issue #3593, #1197- fix isolate management

include/AdblockPlus/JsEngine.h

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2015 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 34 matching lines @@
 namespace AdblockPlus
 {
   class JsEngine;
 
   /**
    * Shared smart pointer to a `JsEngine` instance.
    */
   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.
      */
     typedef std::tr1::function<void(JsValueList& params)> EventCallback;
 
     /**
      * Maps events to callback functions.
      */
     typedef std::map<std::string, EventCallback> EventMap;
@@ skipping 134 matching lines @@
      * Sets the `LogSystem` implementation used for logging (e.g. to handle
      * `console.log()` calls from JavaScript).
      * Setting this is optional, the engine will use a `DefaultLogSystem`
      * instance by default, which might be sufficient.
      * @param The `LogSystem` instance to use.
      */
     void SetLogSystem(LogSystemPtr val);
 
   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;
   };
 }
 
 #endif