Issue #3593 - Make isolate a fully internal member of the engine

src/JsEngine.cpp

Index: src/JsEngine.cpp
===================================================================
--- a/src/JsEngine.cpp
+++ b/src/JsEngine.cpp
@@ -78,25 +78,18 @@
   contextScope(engine->GetContextAsLocal())
 {}
 
-AdblockPlus::ScopedV8Isolate::ScopedV8Isolate()
-{
-  V8Initializer::Init();
-  isolate = v8::Isolate::New();
-}
-
-AdblockPlus::ScopedV8Isolate::~ScopedV8Isolate()
-{
-  isolate->Dispose();
-  isolate = nullptr;
-}
-
-AdblockPlus::JsEngine::JsEngine(const ScopedV8IsolatePtr& isolate)
+AdblockPlus::JsEngine::JsEngine(v8::Isolate* isolate)
   : isolate(isolate)
 {}
 
-JsEngineInternal::JsEngineInternal(const AdblockPlus::ScopedV8IsolatePtr& isolate)
+AdblockPlus::JsEngine::~JsEngine()
+{
+  isolate->Dispose();
+}
+
+JsEngineInternal::JsEngineInternal(v8::Isolate* isolate)
   : AdblockPlus::JsEngine(isolate),
-  context(isolate->Get(),v8::Context::New(isolate->Get()))
+  context(isolate,v8::Context::New(isolate))
 {
   /*
    * Enter v8 scope for our context so that we can initialize its global object.
@@ -130,25 +123,28 @@
  * C++ has no syntax to use anything like a sentry object in the constructor itself.
  * Thus we need to establish v8 scope within every C++ scope that constructs an object.
  */
-AdblockPlus::JsEnginePtr AdblockPlus::JsEngine::New(const AppInfo& appInfo, const ScopedV8IsolatePtr& isolate)
+AdblockPlus::JsEnginePtr AdblockPlus::JsEngine::New(const AppInfo& appInfo)
 {
-  auto isolateP = isolate->Get();
+  /* Global initialization for v8 required before first construction of isolate
+   */
+  V8Initializer::Init();
+
   /*
-   * TODO: Remove `locker`.
-   * Until #3595 is fixed, unit tests allocate isolates outside of this class.
-   * Once we're no longer doing that, we may assume that this class holds
-   *   exclusive access to the nascent isolate.
-   * The factory and constructor constitute a single-threaded usage,
-   *   and there will be no need to lock the isolate.
+   * The constructors require v8 scopes in order to work, and we only need
+   * these scopes for the duration of construction, not the entire duration
+   * of the engine. We create the isolate outside the constructors so that
+   * these v8 scopes end when the factory returns.
    */
-  const v8::Locker locker(isolateP);
+  auto isolate(v8::Isolate::New());
+
   /*
    * Set up v8 scopes for isolate and handle.
    * We cannot set up the v8 scope for context because it doesn't exist
    *   until after the constructor for `JsEngineInternal` returns.
    */
-  const v8::Isolate::Scope isolateScope(isolateP);
-  const v8::HandleScope handleScope(isolateP);
+  const v8::Locker locker(isolate);
+  const v8::Isolate::Scope isolateScope(isolate);
+  const v8::HandleScope handleScope(isolate);
 
   std::shared_ptr<JsEngineInternal> engine(std::make_shared<JsEngineInternal>(isolate));
 
@@ -161,7 +157,7 @@
 
 v8::Local<v8::Context> JsEngineInternal::GetContextAsLocal() const
 {
-  return v8::Local<v8::Context>::New(isolate->Get(), context);
+  return v8::Local<v8::Context>::New(isolate, context);
 }
 
 AdblockPlus::JsValuePtr AdblockPlus::JsEngine::GetGlobalObject()