Issue #3594 - Remove `globalJsObject` and add `GetGlobalObject()`

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 73 matching lines @@
 AdblockPlus::JsEnginePtr AdblockPlus::JsEngine::New(const AppInfo& appInfo, const ScopedV8IsolatePtr& isolate)
 {
   JsEnginePtr result(new JsEngine(isolate));
 
   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->GetIsolate(),
     v8::Context::New(result->GetIsolate())));
-  v8::Local<v8::Object> globalContext = v8::Local<v8::Context>::New(
-    result->GetIsolate(), *result->context)->Global();
-  result->globalJsObject = JsValuePtr(new JsValue(result, globalContext));
-  AdblockPlus::GlobalJsObject::Setup(result, appInfo, result->globalJsObject);
+  AdblockPlus::GlobalJsObject::Setup(result, appInfo, result->GetGlobalObject());
   return result;
 }
 
+AdblockPlus::JsValuePtr AdblockPlus::JsEngine::GetGlobalObject()
+{
+  JsContext context(shared_from_this());
+  return JsValuePtr(new JsValue(shared_from_this(), context.GetV8Context()->Global()));

[Eric, 2016/11/28 17:51:27]

`JsContext` creates a scope that's entered upon construction. To make this a
conservative change, I decided to use the same v8 context as the v8 execution
scope uses. I'm not sure this is strictly necessary, but it seems less likely to
create complications as our v8 implementation changes.

[sergei, 2016/11/29 09:47:12]

I would merely like to clarify here that not "the same v8 context" but "the same
local handle of v8 context", indeed there is no need to create yet one handle.
We use one context object per JsEngine in v8's heap, however it can have many
local handles, even more, as far as I remember in new v8 one can use any v8
object only through a local handle.
So, that's totally fine.

[Eric, 2016/11/30 17:59:46]

Right. I had a problem on an earlier iteration of this code where creating new
handles was throwing an error inside v8 because of a conflict with the isolate.
Given that, I figure it was safer to reuse the handle.

+};

[sergei, 2016/11/29 09:47:12]

Nit: no semicolon.

[Eric, 2016/11/30 17:59:46]

Done.

+
 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(GetIsolate(), source,
     filename);
   CheckTryCatch(tryCatch);
   v8::Local<v8::Value> result = script->Run();
   CheckTryCatch(tryCatch);
@@ skipping 126 matching lines @@
   if (!val)
     throw std::runtime_error("LogSystem cannot be null");
 
   logSystem = val;
 }
 
 
 void AdblockPlus::JsEngine::SetGlobalProperty(const std::string& name, 
                                               AdblockPlus::JsValuePtr value)
 {
-  if (!globalJsObject)
+  auto global = GetGlobalObject();
+  if (!global)
     throw std::runtime_error("Global object cannot be null");
-
-  globalJsObject->SetProperty(name, value);
+  global->SetProperty(name, value);
 }