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

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

Remove the global object member variable, which is part of the reference cycle
that is the subject of #3594. Replace it with a function that recreates an
equivalent JS variable each time. Change the single use of `globalJsObject` to
call `GetGlobalObject()` instead.

Added member variable and accessor for the v8 context in `JsContext`.

[Eric, 2016-11-28 14:22:32]

This code passes the existing unit test suite. I didn't feel the need to add
tests, although its possible that some might be appropriate.

[sergei, 2016-11-28 15:04:33]

Good change.

https://codereview.adblockplus.org/29365532/diff/29365533/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29365532/diff/29365533/src/JsEngine.cpp#ne...
src/JsEngine.cpp:100: const auto i = GetIsolate();
Why not to use JsContext context(shared_from_this());?

[Eric, 2016-11-28 15:33:24]

https://codereview.adblockplus.org/29365532/diff/29365533/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29365532/diff/29365533/src/JsEngine.cpp#ne...
src/JsEngine.cpp:100: const auto i = GetIsolate();
On 2016/11/28 15:04:33, sergei wrote:
> Why not to use JsContext context(shared_from_this());?

That should work. The code here is a minimal extraction refactoring from the
existing code, which meant that I just copied the existing statements over. I'll
see if this works. If it works here, it's probably also the right thing to do in
New() as well, where I copied it from.

One of the things I've wondered looking at this code the last couple of days is
why there's a separate context class at all. Evidently others are using
`v8::Persistent` to hold a single context, which would get rid of a bunch of
overhead creating it each time it's needed.

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

Added a line in the commit message about `JsContext`.

https://codereview.adblockplus.org/29365532/diff/29365542/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29365532/diff/29365542/src/JsEngine.cpp#ne...
src/JsEngine.cpp:101: return JsValuePtr(new JsValue(shared_from_this(),
context.GetV8Context()->Global()));
`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]

LGTM

https://codereview.adblockplus.org/29365532/diff/29365542/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29365532/diff/29365542/src/JsEngine.cpp#ne...
src/JsEngine.cpp:101: return JsValuePtr(new JsValue(shared_from_this(),
context.GetV8Context()->Global()));
On 2016/11/28 17:51:27, Eric wrote:
> `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.

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.

https://codereview.adblockplus.org/29365532/diff/29365542/src/JsEngine.cpp#ne...
src/JsEngine.cpp:102: };
Nit: no semicolon.

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

https://codereview.adblockplus.org/29365532/diff/29365542/src/JsEngine.cpp
File src/JsEngine.cpp (right):

https://codereview.adblockplus.org/29365532/diff/29365542/src/JsEngine.cpp#ne...
src/JsEngine.cpp:101: return JsValuePtr(new JsValue(shared_from_this(),
context.GetV8Context()->Global()));
On 2016/11/29 09:47:12, sergei wrote:
> I would merely like to clarify here that not "the same v8 context" but "the
same
> local handle of v8 context"

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.

https://codereview.adblockplus.org/29365532/diff/29365542/src/JsEngine.cpp#ne...
src/JsEngine.cpp:102: };
On 2016/11/29 09:47:12, sergei wrote:
> Nit: no semicolon.

Done.

[sergei, 2016-11-30 21:48:58]

lgtm

[Eric, 2016-12-02 16:06:09]

@sergie, when do you want this pushed? Are we waiting on the others?

[sergei, 2016-12-02 16:10:12]

On 2016/12/02 16:06:09, Eric wrote:
> @sergie, when do you want this pushed? Are we waiting on the others?

Yes, I wanted to give others an opportunity to review it, however I think it can
be pushed.

[Eric, 2016-12-02 16:24:27]

On 2016/12/02 16:10:12, sergei wrote:
> Yes, I wanted to give others an opportunity to review it, however I think it
can
> be pushed.

I'll push it sometime after the end of the work day in DE in case there are any
further comments.

[Felix Dahlke, 2016-12-02 16:34:56]

LGTM, but I just had a quick look.