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
+ * 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 ADBLOCK_PLUS_JS_ENGINE_H
 #define ADBLOCK_PLUS_JS_ENGINE_H
 
+#include <functional>
 #include <map>
 #include <stdexcept>
 #include <stdint.h>
 #include <string>
 #include <AdblockPlus/AppInfo.h>
-#include <AdblockPlus/tr1_functional.h>
 #include <AdblockPlus/LogSystem.h>
 #include <AdblockPlus/FileSystem.h>
 #include <AdblockPlus/JsValue.h>
 #include <AdblockPlus/WebRequest.h>
-
-#include "tr1_memory.h"
-#include "V8ValueHolder.h"
 
 namespace v8
 {
   class Arguments;
   class Isolate;
   class Value;
   class Context;
   template<class T> class Handle;
   typedef Handle<Value>(*InvocationCallback)(const Arguments &args);
 }
 
 namespace AdblockPlus
 {
   class JsEngine;
 
   /**
    * Shared smart pointer to a `JsEngine` instance.
    */
-  typedef std::tr1::shared_ptr<JsEngine> JsEnginePtr;
+  typedef std::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* Get()
+    {
+      return isolate;
+    }
+  private:
+    ScopedV8Isolate(const ScopedV8Isolate&);
+    ScopedV8Isolate& operator=(const ScopedV8Isolate&);
+
     v8::Isolate* isolate;
   };
 
   /**
+   * Shared smart pointer to ScopedV8Isolate instance;
+   */
+  typedef std::shared_ptr<ScopedV8Isolate> ScopedV8IsolatePtr;
+
+  /**
    * 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>, protected ScopedV8Isolate
+   */
+  class JsEngine : public std::enable_shared_from_this<JsEngine>
   {
     friend class JsValue;
     friend class JsContext;
+
   public:
     /**
      * Event callback function.
      */
-    typedef std::tr1::function<void(JsValueList& params)> EventCallback;
+    typedef std::function<void(JsValueList& params)> EventCallback;
 
     /**
      * Maps events to callback functions.
      */
     typedef std::map<std::string, EventCallback> EventMap;
 
     /**
      * Creates a new JavaScript engine instance.
      * @param appInfo Information about the app.
+     * @param isolate v8::Isolate wrapper. This parameter should be considered
+     *        as a temporary hack for tests, it will go away. Issue #3593.
      * @return New `JsEngine` instance.
      */
-    static JsEnginePtr New(const AppInfo& appInfo = AppInfo());
+    static JsEnginePtr New(const AppInfo& appInfo = AppInfo(), const ScopedV8IsolatePtr& isolate = ScopedV8IsolatePtr(new ScopedV8Isolate()));
 
     /**
      * Registers the callback function for an event.
      * @param eventName Event name. Note that this can be any string - it's a
      *        general purpose event handling mechanism.
      * @param callback Event callback function.
      */
     void SetEventCallback(const std::string& eventName, EventCallback callback);
 
     /**
@@ skipping 115 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);
 
+    /**
+     * Sets a global property that can be accessed by all the scripts.
+     * @param name Name of the property to set.
+     * @param value Value of the property to set.
+     */
+    void SetGlobalProperty(const std::string& name, AdblockPlus::JsValuePtr value);
+
+    /**
+     * Returns a pointer to associated v8::Isolate.
+     */
+    v8::Isolate* GetIsolate()
+    {
+      return isolate->Get();
+    }
+
   private:
-    JsEngine();
+    explicit JsEngine(const ScopedV8IsolatePtr& isolate);
+
+    /// Isolate must be disposed only after disposing of all objects which are
+    /// using it.
+    ScopedV8IsolatePtr isolate;
+
     FileSystemPtr fileSystem;
     WebRequestPtr webRequest;
     LogSystemPtr logSystem;
-    V8ValueHolder<v8::Context> context;
+    std::unique_ptr<v8::Persistent<v8::Context>> context;
     EventMap eventCallbacks;
+    JsValuePtr globalJsObject;
   };
 }
 
 #endif