Issue #3593, #1197- fix isolate management

include/AdblockPlus/JsEngine.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
@@ skipping 31 matching lines @@
 namespace AdblockPlus
 {
   class JsEngine;
 
   /**
    * Shared smart pointer to a `JsEngine` instance.
    */
   typedef std::shared_ptr<JsEngine> JsEnginePtr;
 
   /**
+   * Scope based isolate manager. Creates a new isolate instance on
+   * constructing and disposes it on destructing.

[Eric, 2016/01/26 14:48:58]

Better wording in the comment would call this a "resource class". What's not
obvious is that a 'v8::Isolate' does *not* act as a resource, and this class
turns it into one.

[sergei, 2016/01/27 15:06:07]

It seems your terminology is slightly different from the popular one.
v8::Isolate is indeed a resource and ScopedV8Isolate is a wrapper to manage its
lifetime, in particular to initialize it and to release it using C++ concept of
scopes. FYI:
https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Resource_Acquisition_Is_Ini...
https://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization

[Eric, 2016/01/27 17:21:01]

"Resource wrapper" is more common.

+   */
+  class ScopedV8Isolate

[Eric, 2016/01/26 14:48:58]

Call it "V8Isolate". Every class is scoped; there's no need for that in the
name.

[sergei, 2016/01/27 15:06:08]

I would not object to call it V8Isolate if it were an isolate in its essence,
but here it's a wrapper around isolate because its single method is an accessor
to the wrapped resource. It's common to describe the type of wrapper in its
name, e.g. std::unique_ptr, std::shared_ptr, boost::scoped_ptr, in Qt -
QScopedArrayPointer and so on. For example, we have JsValue which acts as
JavaScript value in its essence, while the essence of wrapper (the accessor to
internal resource) is used only by JsValue, so it's not ScopedJsValue.

+  {
+  public:
+    ScopedV8Isolate();
+    ~ScopedV8Isolate();

[Eric, 2016/01/26 14:48:58]

Since you're inlining the definition of "GetIsolate()", you might as well do so
also for the constructor and destructor.

[sergei, 2016/01/27 15:06:09]

Constructor and destructor are enough complicated methods which call some v8
methods. We usually don't declare third party library methods in headers because
it's error-prone. It's also undesirable to include third party library headers
in our headers, because in that case, the clients of our headers have to
configure and care about too much stuff.
Because of that the implementations of constructor and destructor are in c++
file that includes v8.h.
`v8::Isolate` is also declared in this header (JsEngine.h), so we may not move
the implementation of the getter which merely returns an opaque pointer to c++
file.

[Eric, 2016/01/27 17:21:01]

That's reasonable.

+    v8::Isolate* GetIsolate()
+    {
+      return isolate;
+    }
+  protected:
+    v8::Isolate* isolate;

[Eric, 2016/01/26 14:48:57]

This member should be private; the "protected" declaration had me looking for
some derived class.

[sergei, 2016/01/27 15:06:09]

Done.

+  private:
+    ScopedV8Isolate(const ScopedV8Isolate&);
+    ScopedV8Isolate& operator=(const ScopedV8Isolate&);
+  };
+
+  /**
+   * Shared smart pointer to ScopedV8Isolate instance;
+   */
+  typedef std::shared_ptr<ScopedV8Isolate> ScopedV8IsolatePtr;

[Eric, 2016/01/26 14:48:58]

There's no need for this typedef, and 'shared_ptr' is not the right smart
pointer.

1) There's only a single place this type is used; it's as a member of
'JsEngine'. There's no need for a typedef. But this observation is seeing a
symptom of a larger problem.

2) We can use 'unique_ptr' to manage 'v8::Isolate'. 'JsEngine' is only ever
accessed through a 'shared_ptr'. Sharing semantics happen through that type.
There's no need to duplicate that behavior here. We don't have any code where
one isolate is shared between many instance of 'JsEngine'. Using 'unique_ptr' is
sufficient.

3) Allocation of 'v8::Isolate' should happen in 'class V8Isolate'. In the
current patch set of the present review, the only way that this class is
instantiated is by a call to 'make_shared'. There's no need to repeat the
allocation everywhere it's used. That allocation can go in the constructor of
'V8Isolate'

4) The most recent patch set makes this class non-copyable. 'unique_ptr' is
already non-copyable. Changing the isolate member from a raw pointer to
'unique_ptr' will be just fine.

5) 'V8Isolate::GetIsolate()' should call 'get()' on its underlying pointer; this
is for clarity. And the name of this function can be just 'Get()' or even a
conversion operator to the underlying pointer type.

[sergei, 2016/01/27 15:06:08]

Now it's used in several places in JsEngine and once in BaseJsTest.cpp, it's
convenient to have a typedef for it. If it were used only a couple of times,
there certainly would not be typedef for it.
std::unique_ptr with custom deleter requires to declare that deleter in the
variable type, `std::unique_ptr<v8::Isolate, Deleter>`. So we have to either
have a wrapper which calls `isolate->Dispose()` or use our own wrapper that does
it in its destructor. v8::Isolate also requires a special initializer. Thus,
having the wrapper around `v8::Isolate::Dispose` we spread the initialization
and deinitialization around at least constructor of JsEngine and a deleter
wrapper. I decided to create a special class whose responsibility is to
initialize and deinitialize `v8::Isolate`, so we have them located in one unit.
Having that I would like to have rather member ScopedV8Isolate than
std::unique_ptr.
As well as it allows to solve current issue by injecting v8::Isolate into
JsEngine, that in general looks expected for testing purposes, though, it's
rather overengineering here because we are not mocking v8::Isolate.
It's not clear what you mean. Allocation already happens in ScopedV8Isolate
using `v8::Isolate::New()`. For better testing each test should be isolated as
much as possible, so we should allocate what ever we need in each test. There is
shared pointer to ScopedV8Isolate only to figure out the issue with memory
usage. I would like to work on it after updating of v8 because it's seems more
important now.
Explained above, I don't see a reason to spread allocation and deallocation
around, having it in ctr and dtr looks quite clean.
I don't think we need to store v8::Isolate in std::unique_ptr here, it's
overengineering, I have renamed the member method `GetIsolate` to `Get`.

[Eric, 2016/01/27 17:21:01]

It does not need to be used in any of those places. It can be used solely inside
'V8Isolate'.
You don't need a custom deleter. Apparently I was not clear enough. Declare the
member variable thus:

    std::unique_ptr<std::v8::Isolate> isolate;

Replace the 'make_shared' that are made _outside_ of the class with a call to
'new' _inside_ the constructor of the class.
Your motivation is fine, except that you only wrapped one of the two resources.
One is the instance of 'v8::Isolate'. The other is the heap memory where that
instance is located. Do the memory allocation inside the class also.
There are two kinds of allocations here. The one you mention is the set of
everything that 'v8::Isolate::New()' does. The second is a heap memory
allocation for the instance of the isolate.
That's a reasonable plan, but we need more here in order to execute it well. I
don't particularly mind making a step backwards in the API for 'JsEngine', but
if that's how you want to do it, you need (1) inline documentation (comments)
identifying the regression and (2) a ticket to get it fixed in the future.

On the other hand, I don't think you need to regress 'JsEngine' to get what you
want. See comments for "BaseJsTest".
It would be over-engineering to use both unique_ptr and shared_ptr. It is not
over-engineering to replace shared_ptr with unique_ptr.

[sergei, 2016/01/28 14:02:50]

Sorry, what can be used? Are you saying that std::shared_ptr<ScopedV8Isolate>
can be used inside std::shared_ptr<ScopedV8Isolate>?
There is no `std::v8`, I guess `std::` is accidentally added here. How does
`std::unique_ptr<v8::Isolate>` know that it should call v8::Isolate::Dispose()
on the stored pointer without custom deleter?
It's not clear what you suggest to construct with `new` instead of
`std::make_shared`, what particular "the class" is and, consequently, how it
helps.
Sorry, which heap allocation are you talking about? Heap memory allocation for
`v8::Isolate` is performed by `v8::Isolate::New()`.
It's added in the issue. I don't think we need to comment it everywhere in the
code, however, I have added a comment regarding this parameter to the function
documentation.
Answered in BaseJsTest.
It's not clear about what places you talking here already.

[Eric, 2016/01/28 16:42:41]

Of course not.

I'm going to post a review. It will be quicker that trying to explain it yet
again.
That will work. As long as we have documentation a regression that necessary as
a workaround, we're OK.

+
+  /**
    * JavaScript engine used by `FilterEngine`, wraps v8.
    */
   class JsEngine : public std::enable_shared_from_this<JsEngine>
   {
     friend class JsValue;
     friend class JsContext;
 
   public:
     /**
      * Event callback function.
      */
     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.
      * @return New `JsEngine` instance.
      */
-    static JsEnginePtr New(const AppInfo& appInfo = AppInfo());
+    static JsEnginePtr New(const AppInfo& appInfo = AppInfo(), const ScopedV8IsolatePtr& isolate = ScopedV8IsolatePtr());

[Eric, 2016/01/26 14:48:57]

Having an argument for the isolate in the constructor seems like a Bad Idea.
'JsEngine' owns the isolate; it should be the one that creates it.

It seems that the only reason to have an argument here is to support a shared
isolate for the unit tests. I'm not even sure sharing isolates is a good idea,
but if you need sharing for unit test, why not share a single instance of
'JsEngine'?

[sergei, 2016/01/27 15:06:07]

It sounds like "having a clean testable architecture seems like a Bad Idea".
As I told above, here it's indeed overengineering because we don't mock
v8::Isolate, but we need it to figure out the current issue.
Nobody is saying that it's a good idea, I told in the description that it's a
hack with which we have to live so far. Sharing of JsEngine is even worse idea
because in that case we share even bigger state.

[Eric, 2016/01/27 17:21:01]

It's not clean, in my opinion. It's creating an unnecessary dependency.
As long as you're going with a not-good idea, you might just as well go with one
that you don't have to clean up after later. Sharing 'JsEngine' seems like a
better idea if all you want is an interim solution.

I have to say, though, that needing to do any of this smells bad. If the problem
is that we're running out of memory when we allocate-and-then-deallocate what's
supposed to be a resource multiple times, then it's not actually acting as a
resource. I am growing more uncomfortable with unforeseen side effects of this
change.

[sergei, 2016/01/28 14:02:50]

Answered in BaseJsTest
(https://codereview.adblockplus.org/6233220328718336/diff/29334762/test/BaseJs...).

 
     /**
      * 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 122 matching lines @@
      */
     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()

[Eric, 2016/01/26 14:48:58]

Note: returns a raw pointer, not a smart one, even though its member is a smart
pointer. Better to call get() explicitly in such a case.

[sergei, 2016/01/27 15:06:09]

By fact we need raw v8::Isolate for internal purposes and we don't need any
wrapper around it. As it's discussed above, currently we have to have a member
JsEngine::isolate of a shared type. I would certainly like to change it in
future and we won't need to change places which work with JsEngine::GetIsolate.

+    {
+      return isolate->GetIsolate();
+    }
+
   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;
-    v8::Isolate* isolate;
     std::unique_ptr<v8::Persistent<v8::Context>> context;
     EventMap eventCallbacks;
     JsValuePtr globalJsObject;
   };
 }
 
 #endif