Issue #3593, #1197- fix isolate management

src/JsEngine.cpp

 /*
  * 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/>.
  */
 
 #include <AdblockPlus.h>
-
 #include "GlobalJsObject.h"
 #include "JsContext.h"
 #include "JsError.h"
 #include "Utils.h"
 
 namespace
 {
   v8::Handle<v8::Script> CompileScript(v8::Isolate* isolate,
     const std::string& source, const std::string& filename)
   {
@@ skipping 28 matching lines @@
   public:
     static void Init()
     {
       // it's threadsafe since C++11 and it will be instantiated only once and
       // destroyed at the application exit
       static V8Initializer initializer;
     }
   };
 }
 
-AdblockPlus::JsEngine::JsEngine()
-  : isolate(v8::Isolate::GetCurrent())
+AdblockPlus::ScopedV8Isolate::ScopedV8Isolate()
+  : isolate(v8::Isolate::New())
 {
 }
 
-AdblockPlus::JsEnginePtr AdblockPlus::JsEngine::New(const AppInfo& appInfo)
+AdblockPlus::ScopedV8Isolate::~ScopedV8Isolate()
+{
+  isolate->Dispose();
+  isolate = nullptr;
+}
+
+AdblockPlus::JsEngine::JsEngine(const ScopedV8IsolatePtr& isolate)
+  : isolate(isolate ? isolate : std::make_shared<ScopedV8Isolate>())

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

The hidden default in this initializer is both unnecessary and a bad idea. This
constructor is only being called twice.
1) In 'CreateJsEngine()', where 'make_shared' is already being called.
2) in 'JsEngine()' right below, which relies on this default.
Just pass the argument through.

But, as I've said elsewhere, having an argument for the isolate here is a bad
idea in the first place.

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

Done.

+{
+}
+
+AdblockPlus::JsEnginePtr AdblockPlus::JsEngine::New(const AppInfo& appInfo, const ScopedV8IsolatePtr& isolate)
 {
   V8Initializer::Init();
-  JsEnginePtr result(new JsEngine());
+  JsEnginePtr result(new JsEngine(isolate));
 
-  const v8::Locker locker(result->isolate);
-  const v8::HandleScope handleScope;
+  const v8::Locker locker(result->GetIsolate());
+  const v8::Isolate::Scope isolateScope(result->GetIsolate());
+  const v8::HandleScope handleScope(result->GetIsolate());
 
-  result->context.reset(new v8::Persistent<v8::Context>(result->isolate,
-    v8::Context::New(result->isolate)));
+  result->context.reset(new v8::Persistent<v8::Context>(result->GetIsolate(),
+    v8::Context::New(result->GetIsolate())));
   v8::Local<v8::Object> globalContext = v8::Local<v8::Context>::New(
-    result->isolate, *result->context)->Global();
+    result->GetIsolate(), *result->context)->Global();
   result->globalJsObject = JsValuePtr(new JsValue(result, globalContext));
   AdblockPlus::GlobalJsObject::Setup(result, appInfo, result->globalJsObject);
   return result;
 }
 
 AdblockPlus::JsValuePtr AdblockPlus::JsEngine::Evaluate(const std::string& source,
     const std::string& filename)
 {
   const JsContext context(shared_from_this());
   const v8::TryCatch tryCatch;
-  const v8::Handle<v8::Script> script = CompileScript(isolate, source,
+  const v8::Handle<v8::Script> script = CompileScript(GetIsolate(), source,
     filename);
   CheckTryCatch(tryCatch);
   v8::Local<v8::Value> result = script->Run();
   CheckTryCatch(tryCatch);
   return JsValuePtr(new JsValue(shared_from_this(), result));
 }
 
 void AdblockPlus::JsEngine::SetEventCallback(const std::string& eventName,
     AdblockPlus::JsEngine::EventCallback callback)
 {
@@ skipping 14 matching lines @@
 
 void AdblockPlus::JsEngine::Gc()
 {
   while (!v8::V8::IdleNotification());
 }
 
 AdblockPlus::JsValuePtr AdblockPlus::JsEngine::NewValue(const std::string& val)
 {
   const JsContext context(shared_from_this());
   return JsValuePtr(new JsValue(shared_from_this(),
-    Utils::ToV8String(isolate, val)));
+    Utils::ToV8String(GetIsolate(), val)));
 }
 
 AdblockPlus::JsValuePtr AdblockPlus::JsEngine::NewValue(int64_t val)
 {
   const JsContext context(shared_from_this());
   return JsValuePtr(new JsValue(shared_from_this(),
-    v8::Number::New(isolate, val)));
+    v8::Number::New(GetIsolate(), val)));
 }
 
 AdblockPlus::JsValuePtr AdblockPlus::JsEngine::NewValue(bool val)
 {
   const JsContext context(shared_from_this());
   return JsValuePtr(new JsValue(shared_from_this(), v8::Boolean::New(val)));
 }
 
 AdblockPlus::JsValuePtr AdblockPlus::JsEngine::NewObject()
 {
@@ skipping 83 matching lines @@
 
 
 void AdblockPlus::JsEngine::SetGlobalProperty(const std::string& name, 
                                               AdblockPlus::JsValuePtr value)
 {
   if (!globalJsObject)
     throw std::runtime_error("Global object cannot be null");
 
   globalJsObject->SetProperty(name, value);
 }