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

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
@@ skipping 60 matching lines @@
   };
 }
 
 V8ExecutionScope::V8ExecutionScope(JsEngineInternal* engine)
   : lock(engine->GetIsolate()),
   isolateScope(engine->GetIsolate()),
   handleScope(engine->GetIsolate()),
   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.
    */
   const v8::Context::Scope contextScope(GetContextAsLocal());
   auto globalObject = GetGlobalObject();
   // Timeout
   globalObject->Set(ToV8String("setTimeout"), MakeCallback(::CallbackForSetTimeout));
   // Web request
   auto auxiliaryObject = v8::Object::New();
@@ skipping 13 matching lines @@
 }
 
 /**
  * \par Design Notes
  * It is technically necessary to construct JsEngine instances *only* within a factory.
  * Initialization requires that certain transient v8 scopes be set up 
  *   before initialization and torn down afterwards.
  * 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.

[Eric, 2017/01/12 15:03:15]

Apparently this comment is wrong with respect to the version of v8 that we're
using. Maybe it's true for a more recent version. It does seem from the
documentation that it's not necessary.

In the interim, I've removed the comment. It's not like it's a burden to keep it
in.

+   * 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));
 
   JsEnginePtr result(engine);
   // Establish a context scope for the legacy setup of the global object
   const v8::Context::Scope contextScope(engine->GetContextAsLocal());
   AdblockPlus::GlobalJsObject::Setup(result, appInfo, result->GetGlobalObject());
   return result;
 }
 
 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()
 {
   return JsValuePtr(new JsValue(shared_from_this(), ToInternal(this)->GetGlobalObject()));
 }
 
 v8::Local<v8::Object> JsEngineInternal::GetGlobalObject()
 {
   return GetContextAsLocal()->Global();
@@ skipping 215 matching lines @@
 
 JsEngineInternal* ToInternal(AdblockPlus::JsEnginePtr p)
 {
   return static_cast<JsEngineInternal*>(p.get());
 }
 
 JsEngineInternal* ToInternal(AdblockPlus::JsEngine* p)
 {
   return static_cast<JsEngineInternal*>(p);
 }