Get rid of V8ValueHolder.h

[sergz]

Background

Inspired by ​http://codereview.adblockplus.org/5691839784943616/ and by https://issues.adblockplus.org/ticket/579 (but it's not a reason of it). I would say that making them weak is not a right direction. At least in the recent v8.

First of all the following snippet works fine

{
  auto v8String = v8::String::NewFromUtf8(isolate, "my string");
  auto uniquePersistent1 = v8::UniquePersistent<v8::String>(isolate, v8String); // the same for our current version of v8 where v8::Persistent is instead of v8::UniquePersistent
  auto uniquePersistent2 = v8::UniquePersistent<v8::String>(isolate, v8String);
} // no crash here because there is no double freeing

I guess that our wrong point was that the heap snippet occupied by that object is cleared when its v8::Persistent::Reset (Dispose in our current version) is called. Debugging a little bit internals of v8 I can say that I'm about 100% sure that there is already a reference counter. There is nothing regarding it in v8.h as well as I would say it's not directly clear from the available documentation and as the result there are questions caused by misunderstanding it in mail list. The constructor of v8::Persistent and v8::Persistent::Reset indeed increment and decrement the usage counter.
MakeWeak does not exactly convert internal object to some analogue of known shared_ptr and the callback passed to MakeWeak is not exactly like a deleter for smart pointers. Actually, constructing persistent handle enables behaviour of shared_ptr.

"A persistent handle can be made weak, using PersistentBase::MakeWeak, to trigger a callback from the garbage collector when the only references to an object are from weak persistent handles."

So that callback may be and may be not called by GC when there is no any strong (not weak) handle, thus all of them have been destroyed. The weakness helps v8 GC to proper manage memory consumption. One can attach a C++ object to the v8 object using SetInternalField and notify GC about consumed memory calling Isolate::AdjustAmountOfExternalAllocatedMemory(sizeof(internal object)). In the callback we can free the memory occupied by the internal object, delete the weak persistent handles, Dispose them and call again Isolate::AdjustAmountOfExternalAllocatedMemory(-sizeof(internal object)) to notify about freed memory.

In addition I would like to describe quite error-prone situation.

<function frame, it's not necessary to be our function>
  ...
  v8::Persistent<v8::Value> persistentA ... // obtained somehow
  std::vector<v8::Local<v8::Value>> localA;
  localArgs.push_back(v8::Local<v8::Value>(persistentA));
  func->Call(thisObj, localA.size(), localA.data()); // it's how we do it
    <several v8 function frames>
    <our callback function frame>
      smart_ptr<v8::Persistent> persistentB = 
      <JsEngine::ConvertArguments>
        return smart_ptr<v8::Persistent>(arguments[i]); // actually it is wrapped by std::shared_ptr of JsValue which holds V8ValueHolder which holds std::auto_ptr<v8::Persistent> which calls Reset
      // use persistentB via JsValue API
      persistentB->Reset(); // ~shared_ptr >> ~JsValue >> ~V8ValueHolder
  persistentArg.Reset(); // The second Reset/Dispose call which, I would say, should lead to undefined behaviour.

If v8::Persistent::Reset were clearing the internal v8 object I think we would faced already with that crash or strange behaviour.

What to change

• remove V8ValueHolder.h
• add JsValue(JsValue&& value)
• replace V8ValueHolder value member of JsValue by v8::UniquePersistent
• create Filter and Subscription from sValue&& instead of JsValuePtr. It makes the code stronger. It is what we really semantically need and what we are doing now. Before we did it implicitly, compiler-generated copy ctr for V8ValueHolder implicitly transferred the owned object because std::auto_ptr was used.

[eric@adblockplus.org]

Typo fix

[sergz]

Set P2 because a lot of things depends on it.

[abpbot]

A commit referencing this issue has landed:
[​https://hg.adblockplus.org/libadblockplus/rev/edecc3855b47 Issue 1550 - Get rid of V8ValueHolder.h

• make JsValue movable
• remove V8ValueHolder.h
• use pointer (std::unique_ptr<v8::Persistent<v8::Context>>) instead of member to avoid including v8.h in the public library header.]