Issue 3594 - remove circular references JsEngine-JsValue-JsEngine

- stop sharing of v8::Isolate among tests (test/BaseJsTest.cpp::CreateJsEngine).
- JsValue now holds a weak_ptr<JsEngine> instead of a shared_ptr, so now there is no circular references. ~JsValue is also now guarded by JsContext because we should not try to Dispose underlying value if there is no longer JsEngine.
- add JsEngine::JsEngineNotAvailableException which is thrown when JsEngine is not available.
- Ignore JsEngine::JsEngineNotAvailableException if it's thrown inside Thread::Run.
- IoThread (used in FileSystemJsObject.cpp) and WebRequestJsObject now hold a weak_ptr<JsEngine> instead of shared_ptr. Indirect strong references to JsEngine held by JsValue members are also removed by changes in JsValue. So even if there is a long running IO operation the application is still controlling the lifetime of corresponding instance of JsEngine. Although it's not an issue for current FileSystem, it's vital for tests with LazyWebRequest (FileSystem will likely also have asynchronous interface in a future). LazyWebRequest sleeps for infinity time thus preventing the thread from finishing, so with shared_ptr member the instance of JsEngine is never deleted, weak_ptr allows to free resources. The disadvantage of the solution is that if we must do something else in the callback to stay consistent, there is a possibility that the persistent state will be inconsistent because the callback is not called after finishing of IO operation if there is already no JsEngine.
- add member "JsEnginePtr jsEngine;" with corresponding getter to JsContext and change constructor to accept weak_ptr<JsEngine>. We use JsContext as a scoped object defining when JsEngine can be used by obtaining of certain lock and creating scopes for underlying v8 structures. It seems make sense to include JsEngine into that context and it looks convenient and descriptive in code when JsEngine is actually available only if JsContext is successfully instantiated. If weak_ptr references already destroyed JsEngine then it throws exception JsEngine::JsEngineNotAvailableException.
- move creating of JsContext at the beginning of some methods of JsValue because it avoids additional pairs of release/lock operations.
- add std::shared_ptr<JsEngine> Utils::lockJsEngine(const std::weak_ptr<JsEngine>& jsEngine) which throws JsEngine::JsEngineNotAvailableException if it cannot obtain not nullptr.
- add a couple of tests against regressions involving the leakage of JsEngine.
- move "jsEngine = CreateJsEngine();" into SetUp of the fixture for Prefs tests.

# depends on https://codereview.adblockplus.org/29361582/

[Oleksandr, 2016-11-25 10:38:04]

I think I see the reason for this change. Weird how this hasn't been noticed
before. The solution to always wrap jsEngine in Context seems a bit wonky, but I
couldn't come up with anything better that wouldn't require lots of refactoring.
Some comments below.

https://codereview.adblockplus.org/29361562/diff/29361623/include/AdblockPlus...
File include/AdblockPlus/JsValue.h (right):

https://codereview.adblockplus.org/29361562/diff/29361623/include/AdblockPlus...
include/AdblockPlus/JsValue.h:137: std::weak_ptr<JsEngine> m_jsEngine;
Nit: I don't really see a value in renaming the variable here and below.

https://codereview.adblockplus.org/29361562/diff/29361623/src/JsContext.h
File src/JsContext.h (right):

https://codereview.adblockplus.org/29361562/diff/29361623/src/JsContext.h#new...
src/JsContext.h:30: JsEngine& jsEngine() {
Nit: Methods usually start with a capital letter: "GetJsEngine()"?

https://codereview.adblockplus.org/29361562/diff/29361623/src/WebRequestJsObj...
File src/WebRequestJsObject.cpp (right):

https://codereview.adblockplus.org/29361562/diff/29361623/src/WebRequestJsObj...
src/WebRequestJsObject.cpp:62: auto webRequest =
AdblockPlus::Utils::lockJsEngine(m_jsEngine)->GetWebRequest();
Is there any reason why the line above is not using jsEngine from the context as
well?

https://codereview.adblockplus.org/29361562/diff/29361623/test/FilterEngine.cpp
File test/FilterEngine.cpp (right):

https://codereview.adblockplus.org/29361562/diff/29361623/test/FilterEngine.c...
test/FilterEngine.cpp:541: auto jsEngine = AdblockPlus::JsEngine::New();
Did you want to assign weakJsEngine to jsEngine inside this block for testing
purposes? Otherwise I don't see what this test actually tests for.

https://codereview.adblockplus.org/29361562/diff/29361623/test/JsEngine.cpp
File test/JsEngine.cpp (right):

https://codereview.adblockplus.org/29361562/diff/29361623/test/JsEngine.cpp#n...
test/JsEngine.cpp:146: TEST(NewJsEngineTest, MemoryLeak_NoLeak)
This seems like a one brutal test :). Does this actually raise an exception with
circular dependencies?

[sergei, 2016-11-25 12:04:46]

https://codereview.adblockplus.org/29361562/diff/29361623/include/AdblockPlus...
File include/AdblockPlus/JsValue.h (right):

https://codereview.adblockplus.org/29361562/diff/29361623/include/AdblockPlus...
include/AdblockPlus/JsValue.h:137: std::weak_ptr<JsEngine> m_jsEngine;
On 2016/11/25 10:38:04, Oleksandr wrote:
> Nit: I don't really see a value in renaming the variable here and below.

I thought we finally started to use #pragma once and prefixes for members of
classes in C++.
https://codereview.adblockplus.org/29333474/diff/29345688/compiled/String.h

Actually, it's very helpful in C++. Here in particular it also helped to
eliminate all accidental cases of usage of jsEngine member instead of strong
referenced JsEngine.

m_ is removed.

https://codereview.adblockplus.org/29361562/diff/29361623/src/JsContext.h
File src/JsContext.h (right):

https://codereview.adblockplus.org/29361562/diff/29361623/src/JsContext.h#new...
src/JsContext.h:30: JsEngine& jsEngine() {
On 2016/11/25 10:38:04, Oleksandr wrote:
> Nit: Methods usually start with a capital letter: "GetJsEngine()"?

Done.

https://codereview.adblockplus.org/29361562/diff/29361623/src/WebRequestJsObj...
File src/WebRequestJsObject.cpp (right):

https://codereview.adblockplus.org/29361562/diff/29361623/src/WebRequestJsObj...
src/WebRequestJsObject.cpp:62: auto webRequest =
AdblockPlus::Utils::lockJsEngine(m_jsEngine)->GetWebRequest();
On 2016/11/25 10:38:04, Oleksandr wrote:
> Is there any reason why the line above is not using jsEngine from the context
as
> well? 

Yes, it's in the comment. Let's say we have "locked" JsEngine and call
WebRequest::GET. At the same time we have LazyWebRequest::GET which never
returns. It means that all tests will still prevent JsEngine from destroying.
So, here we lock JsEngine, obtain webRequest and "unlock" JsEngine.
AdblockPlus::Utils::lockJsEngine and AdblockPlus::JsContext throw exception if
they cannot lock JsEngine.

https://codereview.adblockplus.org/29361562/diff/29361623/test/FilterEngine.cpp
File test/FilterEngine.cpp (right):

https://codereview.adblockplus.org/29361562/diff/29361623/test/FilterEngine.c...
test/FilterEngine.cpp:541: auto jsEngine = AdblockPlus::JsEngine::New();
On 2016/11/25 10:38:04, Oleksandr wrote:
> Did you want to assign weakJsEngine to jsEngine inside this block for testing
> purposes? Otherwise I don't see what this test actually tests for.

Of course, cannot believe I have missed it))

https://codereview.adblockplus.org/29361562/diff/29361623/test/JsEngine.cpp
File test/JsEngine.cpp (right):

https://codereview.adblockplus.org/29361562/diff/29361623/test/JsEngine.cpp#n...
test/JsEngine.cpp:146: TEST(NewJsEngineTest, MemoryLeak_NoLeak)
On 2016/11/25 10:38:04, Oleksandr wrote:
> This seems like a one brutal test :). Does this actually raise an exception
with
> circular dependencies?

Yes, it perfectly does crash, i = 939 or 940. It means that only ~60 instances
are instantiated and 2GB/60 ~ 33.3MB what conforms with the expectations. It
fails trying to commit (VirtualAlloc) 32MB, in platform-win32.cc, however the
actual crash happens later in v8, by V8::FatalProcessOutOfMemory("heap setup");

[Eric, 2016-11-28 14:21:13]

This review is addressing concerns outside the scope of issue #3594. The topic
of the review is to get rid of a circular reference, but instead of targeting a
change to that particular problem, it's making a very fundamental change to type
semantics. This is not a conservative change.

As stated in the issue, the problem is a persistent reference cycle that
includes member variable `JsEngine::globalJsObject`. The value of this member is
only ever used once, in `SetGlobalProperty()`, which sets a property on the
global object. The much simpler solution is to recreate this global object in
the form needed in this function. Once that's done, there's no need to keep the
global object *as a `JsValue`*, breaking the circular reference. There was
little difference between explaining this further and just doing it, so I went
ahead and did it. 
    Issue #3594 - Remove `globalJsObject` and add `GetGlobalObject()` 
    https://codereview.adblockplus.org/29365532/

I agree that there are reasons to have weak pointers around for threaded JS
tasks. At the very least, though, this deserves its own issue description
describing exactly what is to be addresses; the present issue is far too narrow
for this change set. Moreover, changing the semantics of `JsValuePtr` to have
weak semantics is not a good way of getting weak pointer semantics. Much better
would be to leave `JsValuePtr` with its existing strong pointer semantics,
introduce a new class `JsValueWeakPtr` with proper weak pointer semantics, and
then use it only where it's appropriate. `JsValueWeakPtr` would have a member
function `JsValuePtr Lock()` that would provide a strong pointer where it's
actually needed, in analogy with the relationship between `weak_ptr` and
`shared_ptr`. This was not simple enough just to go ahead and do, and it needs
to be solving some other identified problem before it worth doing at all.

I've left in a few comments on the code itself.

https://codereview.adblockplus.org/29361562/diff/29364531/src/JsValue.cpp
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/29361562/diff/29364531/src/JsValue.cpp#new...
src/JsValue.cpp:28: , value(new
v8::Persistent<v8::Value>(Utils::lockJsEngine(jsEngine)->GetIsolate(), value))
This use of 'lockJsEngine' has a race condition. The lock keeping the engine
object in existence expires when 'lockJsEngine' returns and is no longer in
place for the dereference operator ->. This is a tiny window for failure, but
it's not thread-safe and may occasionally generate null pointer violations.

https://codereview.adblockplus.org/29361562/diff/29364531/src/JsValue.cpp#new...
src/JsValue.cpp:34: value(std::move(src.value))
For consistency, this should either be (1) `std::move` in both places or (2) a
declaration `using std::move;` and `move` in both places. Personally, `move` is
so fundamental to rvalue semantics that constantly putting `std::` in front of
it is distracting.

This compiles, though, so somehow the compiler is allowing it. A future
compiler, however, might not.

https://codereview.adblockplus.org/29361562/diff/29364531/src/JsValue.cpp#new...
src/JsValue.cpp:52: }
It's excessive to instantiate a `JsContext` object to ensure the existence of
`jsEngine` for the duration of the destructor. Calling `weak_ptr::lock()` is
enough.

Incidentally, there's also no need to call `value.reset()`, since the destructor
for `value` will take care of that, but that was already there.

AdblockPlus::JsValue::~JsValue()
{
  if (value)
  {
    auto engineGuard = jsEngine.lock();
    if (engineGuard)
    {
      value->Dispose();
    }
  }
}

https://codereview.adblockplus.org/29361562/diff/29364531/src/Utils.cpp
File src/Utils.cpp (right):

https://codereview.adblockplus.org/29361562/diff/29364531/src/Utils.cpp#newco...
src/Utils.cpp:114: }
This function doesn't do what its name indicates that it does. The lifespan of
the shared_ptr `retValue` only lasts the duration of the function body. After
the function returns, the engine argument has no more strong references than it
did at function entry. So there's no actually locking except while the function
is executing.

It's not that checking whether the engine exists behind the pointer isn't
valuable. But such a function shouldn't be named "lock" or anything like it. And
it would be more efficient just to call 'expired()' on the weak_ptr.

See comments where this is used for more.

[sergei, 2016-11-29 10:45:19]

On 2016/11/28 14:21:13, Eric wrote:
> This review is addressing concerns outside the scope of issue #3594. The topic
> of the review is to get rid of a circular reference, but instead of targeting
a
> change to that particular problem, it's making a very fundamental change to
type
> semantics. This is not a conservative change.
> 
> As stated in the issue, the problem is a persistent reference cycle that
> includes member variable `JsEngine::globalJsObject`. The value of this member
is
> only ever used once, in `SetGlobalProperty()`, which sets a property on the
> global object. The much simpler solution is to recreate this global object in
> the form needed in this function. Once that's done, there's no need to keep
the
> global object *as a `JsValue`*, breaking the circular reference. There was
> little difference between explaining this further and just doing it, so I went
> ahead and did it. 
>     Issue #3594 - Remove `globalJsObject` and add `GetGlobalObject()` 
>     https://codereview.adblockplus.org/29365532/
> 
> I agree that there are reasons to have weak pointers around for threaded JS
> tasks. At the very least, though, this deserves its own issue description
> describing exactly what is to be addresses; the present issue is far too
narrow
> for this change set. Moreover, changing the semantics of `JsValuePtr` to have
> weak semantics is not a good way of getting weak pointer semantics. Much
better
> would be to leave `JsValuePtr` with its existing strong pointer semantics,
> introduce a new class `JsValueWeakPtr` with proper weak pointer semantics, and
> then use it only where it's appropriate. `JsValueWeakPtr` would have a member
> function `JsValuePtr Lock()` that would provide a strong pointer where it's
> actually needed, in analogy with the relationship between `weak_ptr` and
> `shared_ptr`. This was not simple enough just to go ahead and do, and it needs
> to be solving some other identified problem before it worth doing at all.

Yes, it addresses also other questions but it still solves issue #3594. I will
update the description after landing of
https://codereview.adblockplus.org/29365532/.

I completely agree that the change is not ideal and we should rather use some
kind of WeakJsValue with Lock method and JsValue, but it seems it would be
better to do it in a separate refactoring commit. The change above already does
work automatically for everybody because we have fixed it mostly at level of
internals of JsValue, which is pretty low. It's indeed a very fundamental change
however the public API is still almost the same, except that now almost any
method can throw yet one exception, so we can start to use it without huge
modifications of other at least our products. Significant changes in API will
also likely cause a long discussion, so the review with refactoring can wait but
we can use already use working version.

I propose to create an issue to refactor the approach after committing of the
current changes.

https://codereview.adblockplus.org/29361562/diff/29364531/src/JsValue.cpp
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/29361562/diff/29364531/src/JsValue.cpp#new...
src/JsValue.cpp:28: , value(new
v8::Persistent<v8::Value>(Utils::lockJsEngine(jsEngine)->GetIsolate(), value))
On 2016/11/28 14:21:13, Eric wrote:
> This use of 'lockJsEngine' has a race condition. The lock keeping the engine
> object in existence expires when 'lockJsEngine' returns and is no longer in
> place for the dereference operator ->. This is a tiny window for failure, but
> it's not thread-safe and may occasionally generate null pointer violations.

lockJsEngine returns an object not a reference or pointer, according to the
standard the temporary here will be alive until the finishing of execution of
the whole expression 'value(new
v8::Persistent<v8::Value>(Utils::lockJsEngine(jsEngine)->GetIsolate(), value))'.
So, there is no race condition.

https://codereview.adblockplus.org/29361562/diff/29364531/src/JsValue.cpp#new...
src/JsValue.cpp:34: value(std::move(src.value))
On 2016/11/28 14:21:13, Eric wrote:
> For consistency, this should either be (1) `std::move` in both places or (2) a
> declaration `using std::move;` and `move` in both places. Personally, `move`
is
> so fundamental to rvalue semantics that constantly putting `std::` in front of
> it is distracting.
I have added std:: rather for aesthetic view than for consistency.
> 
> This compiles, though, so somehow the compiler is allowing it. A future
> compiler, however, might not.
Here it works without std:: because of ADL, it's pretty stable and widely used
feature of C++.

https://codereview.adblockplus.org/29361562/diff/29364531/src/JsValue.cpp#new...
src/JsValue.cpp:52: }
On 2016/11/28 14:21:14, Eric wrote:
> It's excessive to instantiate a `JsContext` object to ensure the existence of
> `jsEngine` for the duration of the destructor. Calling `weak_ptr::lock()` is
> enough.
JsContext is not only to ensure that JsEngine is still alive, it also saves from
data race (thanks to JsContext::locker), which happens when `value->Dispose();`
is called from different threads for different values.
> 
> Incidentally, there's also no need to call `value.reset()`, since the
destructor
> for `value` will take care of that, but that was already there.
> 
> AdblockPlus::JsValue::~JsValue()
> {
>   if (value)
>   {
>     auto engineGuard = jsEngine.lock();
>     if (engineGuard)
>     {
>       value->Dispose();
>     }
>   }
> }

Actually there should not be a call of Dispose. That call will be deleted here
after updating of v8. The destructor of v8::UniquePersistent should make that
call, however in the currently used version of v8 it's commented.

https://codereview.adblockplus.org/29361562/diff/29364531/src/Utils.cpp
File src/Utils.cpp (right):

https://codereview.adblockplus.org/29361562/diff/29364531/src/Utils.cpp#newco...
src/Utils.cpp:114: }
On 2016/11/28 14:21:14, Eric wrote:
> This function doesn't do what its name indicates that it does. The lifespan of
> the shared_ptr `retValue` only lasts the duration of the function body. After
> the function returns, the engine argument has no more strong references than
it
> did at function entry. So there's no actually locking except while the
function
> is executing.
That's just wrong.
> 
> It's not that checking whether the engine exists behind the pointer isn't
> valuable. But such a function shouldn't be named "lock" or anything like it.
And
> it would be more efficient just to call 'expired()' on the weak_ptr.
> 
> See comments where this is used for more.

[sergei, 2016-12-01 22:24:38]

https://codereview.adblockplus.org/29361562/diff/29366535/src/JsValue.cpp
File src/JsValue.cpp (right):

https://codereview.adblockplus.org/29361562/diff/29366535/src/JsValue.cpp#new...
src/JsValue.cpp:42: JsContext context(jsEngine);
Although I have not seen a crash on practice there is still a race condition:
if there is already no JsEngine then v8::Persistent::Dispose still can be called
"simultaneously" from different threads. I will look into the code of a newer
version of v8 and check how it works.

[Eric, 2016-12-02 16:41:04]

On 2016/11/29 10:45:19, sergei wrote:
> Yes, it addresses also other questions but it still solves issue #3594. I will
> update the description after landing of
> https://codereview.adblockplus.org/29365532/.

Once that review has landed, #3594 will be resolved and this particular issue
won't provide any justification for the present code.

You say "it addresses also other questions". What are these other questions? I
don't see any issue numbers.

 I completely agree that the change is not ideal and we should rather use some
> kind of WeakJsValue with Lock method and JsValue, but it seems it would be
> better to do it in a separate refactoring commit.

That's backwards. Identify the problem first, then solve it. What you've got
here is a some code that makes a fundamental change with only a vague
justification for it.

As for specifics, using a weak value is a poor solution, since it brings along a
new exception possibilities everywhere in the code. Values should be strong and
always valid. The thing to make weak is the pointer. Locking the pointer brings
a (strong) value into existence, alleviating anything that deals narrowly with
values from any concern over engine lifetime.

If the real problem is that we need to be able to kill background tasks (for
example), then we can write that down as an issue and solve it. If (again for
example) that's the actual issue, then there are better ways of addressing that
particular issue.

> The change above already does
> work automatically for everybody because we have fixed it mostly at level of
> internals of JsValue, which is pretty low.

I really don't know what you mean by "automatically work". What situation do we
have now where an engine (or related) disappears out from under a value?

> Significant changes in API will
> also likely cause a long discussion, so the review with refactoring can wait
but
> we can use already use working version.

Adding a weak pointer class is a much smaller change to the API than changing
the semantics of a value to be arbitrarily invalid. Every piece of code that
relies on JsValue needs to be audited to ensure that this change doesn't induce
new errors. At minimum, this will increase the complexity of the code at the
point of use. The code base as a whole will be smaller, simpler, and cleaner if
we separate out life cycle concerns into a separate class.

> I propose to create an issue to refactor the approach after committing of the
> current changes.

Given all the above reasons, I am against that proposal.

Look, I understand that this review represents a fair bit of work. While I don't
think this code itself is what we want, it's pretty clear that the analysis you
did behind it is useful. Now I didn't look into any of the threading parts
except cursorily once I figured out how to narrowly solve the stated issue, so I
can't speak with any specificity to what's going on there.

What I'd like to see is some issues describing what problems are being solved.
We have issues describing solutions, which can be valid, but for this I believe
its important to know how it will be used.

If you'd like, I can knock out an rough version of JsValueWeakPtr over the
weekend. It doubt it will be a commit-draft, but it certainly can capture the
basic approach.

[sergei, 2016-12-02 18:47:11]

On 2016/12/02 16:41:04, Eric wrote:
> On 2016/11/29 10:45:19, sergei wrote:
> > Yes, it addresses also other questions but it still solves issue #3594. I
will
> > update the description after landing of
> > https://codereview.adblockplus.org/29365532/.
> 
> Once that review has landed, #3594 will be resolved and this particular issue
> won't provide any justification for the present code.
> 
> You say "it addresses also other questions". What are these other questions? I
> don't see any issue numbers.
The actual issue is #3593 and I planned to have for presence the proposed hack
with weak pointers while we properly address #3593 at another level.

> 
>  I completely agree that the change is not ideal and we should rather use some
> > kind of WeakJsValue with Lock method and JsValue, but it seems it would be
> > better to do it in a separate refactoring commit.
> 
> That's backwards. Identify the problem first, then solve it. What you've got
> here is a some code that makes a fundamental change with only a vague
> justification for it.
> 
> As for specifics, using a weak value is a poor solution, since it brings along
a
> new exception possibilities everywhere in the code. Values should be strong
and
> always valid. The thing to make weak is the pointer. Locking the pointer
brings
> a (strong) value into existence, alleviating anything that deals narrowly with
> values from any concern over engine lifetime.
> 
> If the real problem is that we need to be able to kill background tasks (for
> example), then we can write that down as an issue and solve it. If (again for
> example) that's the actual issue, then there are better ways of addressing
that
> particular issue.
We have already issue #3595 and we should never kill threads, they should
gracefully finish.

> 
> > The change above already does
> > work automatically for everybody because we have fixed it mostly at level of
> > internals of JsValue, which is pretty low.
> 
> I really don't know what you mean by "automatically work".
"automatically work" means that we can simply update libadblockplus in other
projects and it will work without going through a code base and adapting to some
new API.
>  What situation do we
> have now where an engine (or related) disappears out from under a value?
I'm not sure that I understood the question correctly. In the proposed changes
JsEngine can go away from JsValue held by working threads.

> 
> > Significant changes in API will
> > also likely cause a long discussion, so the review with refactoring can wait
> but
> > we can use already use working version.
> 
> Adding a weak pointer class is a much smaller change to the API than changing
> the semantics of a value to be arbitrarily invalid. Every piece of code that
> relies on JsValue needs to be audited to ensure that this change doesn't
induce
> new errors. At minimum, this will increase the complexity of the code at the
> point of use. The code base as a whole will be smaller, simpler, and cleaner
if
> we separate out life cycle concerns into a separate class.
> 
> > I propose to create an issue to refactor the approach after committing of
the
> > current changes.
> 
> Given all the above reasons, I am against that proposal.
What about having https://issues.adblockplus.org/ticket/4692 for present instead
of the current changes?

> 
> Look, I understand that this review represents a fair bit of work. While I
don't
> think this code itself is what we want, it's pretty clear that the analysis
you
> did behind it is useful. Now I didn't look into any of the threading parts
> except cursorily once I figured out how to narrowly solve the stated issue, so
I
> can't speak with any specificity to what's going on there.
> 
> What I'd like to see is some issues describing what problems are being solved.
> We have issues describing solutions, which can be valid, but for this I
believe
> its important to know how it will be used.
https://issues.adblockplus.org/ticket/4692
> 
> If you'd like, I can knock out an rough version of JsValueWeakPtr over the
> weekend. It doubt it will be a commit-draft, but it certainly can capture the
> basic approach.
It seems there is no need to have weak JsValue now.

[Eric, 2016-12-02 20:20:22]

On 2016/12/02 18:47:11, sergei wrote:
> The actual issue is #3593 and I planned to have for presence the proposed hack
> with weak pointers while we properly address #3593 at another level.

Ah. I think your idea to use weak pointers in some way might still be useful, so
let's keep it mind. 

> We have already issue #3595 and we should never kill threads, they should
> gracefully finish.

I used "kill" rather loosely. Certain threads might be informed that they needs
to gracefully finish immediately, even if they think they're not done yet. We're
not using C++11 threads in libadblockplus, I see. Do we have build limitations
with <thread>? If we don't, using <mutex> and <condition_variable> would make
all this much easier. If not C++11 threads, we could use Boost threads, which
are almost identical (since that's where they were incubated). I'm fairly sure
that this would work even with an older Boost version, since its thread library
has been around for years.

For example, in TimeoutThread we're currently calling `Sleep()`. If that call
could wait on a condition variable (with timeout) instead, we could terminate
the thread externally. There's some delicacy about ensuring the function and its
argument aren't being used, but nothing too difficult. We'd need the
function/arguments to be weak pointers so the engine could die, but we know how
to do that. 

> >  What situation do we
> > have now where an engine (or related) disappears out from under a value?
> I'm not sure that I understood the question correctly. In the proposed changes
> JsEngine can go away from JsValue held by working threads.

You understood well enough. It sounds like there's no current behavior like
that, so anything like it would be new code. I wanted to make sure we weren't
working on any existing problem here.

> What about having https://issues.adblockplus.org/ticket/4692 for present
instead
> of the current changes?

That's a good start. I think it would be good to state in the description that
the issue refer both to direct references and indirect chains of reference. Not
sure we have any indirect references, but if we do, they'd be part of the
problem.

I'd also suggest a [meta] issue for all these v8 version upgrade issues. It
would make them easier to find. Maybe two, one for code and one for build.