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

src/JsEngine.cpp

Index: src/JsEngine.cpp
===================================================================
--- a/src/JsEngine.cpp
+++ b/src/JsEngine.cpp
@@ -91,13 +91,16 @@
 
   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)
 {
@@ -244,8 +247,8 @@
 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);
 }