Change the way persistent handles are desctructed. Add tests.

This introduces a correct way of destructing V8's persistent handles. We should rely on GC to release them when there are no more refferences to them. With this fix occasional crashes in libadblockplus seem to be gone. 

However this reveals another bug we have in libadblockplus, which leaks memory pretty bad. Apparently GC never actually cleans persistent handles with primitive types. Those should never be created (https://groups.google.com/forum/#!msg/v8-users/616ZM3UWh2k/W3QhdhhK6XQJ)

The two tests added here more or less prove this point. First (PersistentWeakHandleDisposalComplex) succeedes, and the second one (PersistentWeakHandleDisposalPrimitive) fails.

This means that we might need to rewrite V8ValueHandler to treat primitive types differently.

[Wladimir Palant, 2014-07-11 06:19:42]

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
File include/AdblockPlus/V8ValueHolder.h (right):

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:40: value = NULL;
Note that we generally use 0 rather than NULL.

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:76: this->value->MakeWeak(this->value,
(v8::WeakReferenceCallback)&AdblockPlus::V8ValueHolder<T>::ABPDisposeCallback);
So, why is calling Dispose() an issue here? This V8ValueHolder instance is
supposed to be the only place with a reference to this particular handle.

I see no copy assignment operator here, we should add it (rule of three).
However, it doesn't see like any code is triggering it. My guess is that the
real issue is the copy constructor declared above. It simply copies the v8
handle into a new V8ValueHolder instance. As a result, that handle is owned by
two instances and both will attempt to destroy it. We should create a new handle
there instead.

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:105: v8::Persistent<T>* value;
I don't consider using raw pointers a good idea - as you've probably noticed for
this change, they add a fair amount of additional code and a fair amount of
additional potential failure points. And I don't see anything that's wrong with
auto_ptr here.

[Felix Dahlke, 2014-07-11 09:22:20]

I also don't see how these changes change anything - the copy constructor looks
like the culprit indeed.

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
File include/AdblockPlus/V8ValueHolder.h (right):

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:33: 
Superfluous whitespace.

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:40: value = NULL;
On 2014/07/11 06:19:42, Wladimir Palant wrote:
> Note that we generally use 0 rather than NULL.

Also, if we actually want to move away from auto_ptr (which will default to 0),
this should be in the initialisation list. Same for the other constructors
below.

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:76: this->value->MakeWeak(this->value,
(v8::WeakReferenceCallback)&AdblockPlus::V8ValueHolder<T>::ABPDisposeCallback);
On 2014/07/11 06:19:42, Wladimir Palant wrote:
> So, why is calling Dispose() an issue here? This V8ValueHolder instance is
> supposed to be the only place with a reference to this particular handle.
> 
> I see no copy assignment operator here, we should add it (rule of three).
> However, it doesn't see like any code is triggering it. My guess is that the
> real issue is the copy constructor declared above. It simply copies the v8
> handle into a new V8ValueHolder instance. As a result, that handle is owned by
> two instances and both will attempt to destroy it. We should create a new
handle
> there instead.

I agree, the actual change you made (turning this into a weak reference right
before deleting) shouldn't change anything.

And Wladimir is right about the copy constructor - The cast from V8ValueHolder
to v8::Value is going to return the underlying handle, two V8ValueHolder
instances will take ownership of it.

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:105: v8::Persistent<T>* value;
On 2014/07/11 06:19:42, Wladimir Palant wrote:
> I don't consider using raw pointers a good idea - as you've probably noticed
for
> this change, they add a fair amount of additional code and a fair amount of
> additional potential failure points. And I don't see anything that's wrong
with
> auto_ptr here.

Agreed, auto_ptr is evidently what we want here: Defaults to 0, is deleted
automatically invoking the correct destructor. It doesn't look like this
behaviour has changed with your changes.

[Felix Dahlke, 2014-07-11 09:24:16]

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
File include/AdblockPlus/V8ValueHolder.h (right):

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:76: this->value->MakeWeak(this->value,
(v8::WeakReferenceCallback)&AdblockPlus::V8ValueHolder<T>::ABPDisposeCallback);
On 2014/07/11 09:22:20, Felix H. Dahlke wrote:
> On 2014/07/11 06:19:42, Wladimir Palant wrote:
> > So, why is calling Dispose() an issue here? This V8ValueHolder instance is
> > supposed to be the only place with a reference to this particular handle.
> > 
> > I see no copy assignment operator here, we should add it (rule of three).
> > However, it doesn't see like any code is triggering it. My guess is that the
> > real issue is the copy constructor declared above. It simply copies the v8
> > handle into a new V8ValueHolder instance. As a result, that handle is owned
by
> > two instances and both will attempt to destroy it. We should create a new
> handle
> > there instead.
> 
> I agree, the actual change you made (turning this into a weak reference right
> before deleting) shouldn't change anything.
> 
> And Wladimir is right about the copy constructor - The cast from V8ValueHolder
> to v8::Value is going to return the underlying handle, two V8ValueHolder
> instances will take ownership of it. 

Oh, and we should definitely also ensure to declare the copy assignment operator
- if it's used anywhere, this will cause exactly the same problem as the current
copy constructor. Best try to declare the copy assignment operator as private
without definition, assuming it's not used anywhere.

[sergei, 2014-07-22 16:25:38]

I went through the code of v8, there is indeed a bug. When the copy-ctr is
called, it routes to Handle's ctr and does not do anything with any counter.
<code>
template <class S> V8_INLINE(Persistent(Persistent<S> that))
      : Handle<T>(reinterpret_cast<T*>(*that)) {
    ...
    TYPE_CHECK(T, S);
  }
<code>
I've just opened
https://v8.googlecode.com/svn/branches/bleeding_edge/include/v8.h and there is
even intermediate class PersistentBase, quick overview how it's implemented
shows that it's fixed.

So, firstly, I would like to test it with the newer version. In addition I've
already dropped some comments regarding const references and according to the
documentation `value` should not be a pointer (`v8::Persistent<T> value`). In
our current version it (const&) is really not necessary because internally it is
just a pointer, but in the updated version I expect to see the passing of
arguments via the const reference.

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
File include/AdblockPlus/V8ValueHolder.h (right):

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:40: value = NULL;
All members should be initialized in the ctr (in all constructors), `isolate` as
well.

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:43: V8ValueHolder(V8ValueHolder& value)
Why is not const?

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:60: reset(isolate, v8::Persistent<T>());
Superfluous space at the end of the line.

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:65: v8::Persistent<v8::Value>* objectPointer
= (v8::Persistent<v8::Value>*)parameter;
static cast

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:68: object.Dispose();
In the docs it's said we should use ``Dispose(isolate)`.

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:76: this->value->MakeWeak(this->value,
(v8::WeakReferenceCallback)&AdblockPlus::V8ValueHolder<T>::ABPDisposeCallback);
The recommended version is with `isolate`.

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:93: return **value;
I would like to have at least assert before dereferencing the pointer (it's
related to all methods below). I'm not aware about the usage, may we throw an
exception if null pointer is dereferenced?

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:95: operator v8::Handle<T>() const
I'm not sure whether we want to return the copy or const reference here, now
it's a copy.

[Wladimir Palant, 2014-07-23 11:21:29]

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
File include/AdblockPlus/V8ValueHolder.h (right):

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:40: value = NULL;
On 2014/07/22 16:25:38, sergei wrote:
> All members should be initialized in the ctr (in all constructors), `isolate`
as
> well.

The reset() call below will set the isolate member - this approach was chosen to
avoid duplicating the logic across the constructors.

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:93: return **value;
On 2014/07/22 16:25:38, sergei wrote:
> I would like to have at least assert before dereferencing the pointer (it's
> related to all methods below).

Note that in the original version (which we want to keep I think) value is an
auto_ptr instance which is guaranteed to point to a valid v8::Persistent handle.
So while an assert() certainly wouldn't hurt to ensure code correctness, this
code isn't expected to fail.

[sergei, 2014-10-23 08:58:00]

The idea how it's done in the changeset looks completely correct for me, but
unfortunately it does not work with the latest versions of v8. It crashes when
we have a separate thread, as well as when we have v8::Number with zero value
and there may be other cases. As well as the reference counting, I told before,
is removed.

According to https://groups.google.com/forum/#!topic/v8-users/6kSAbnUb-rQ and
https://groups.google.com/d/topic/v8-users/ta9wkdEY08o/discussion I would say
that we should not rely on it and we need some different mechanism of resource
management for v8.

[Oleksandr, 2014-10-23 10:44:56]

I will close this, if no one objects.

On 2014/10/23 08:58:00, sergei wrote:
> The idea how it's done in the changeset looks completely correct for me, but
> unfortunately it does not work with the latest versions of v8. It crashes when
> we have a separate thread, as well as when we have v8::Number with zero value
> and there may be other cases. As well as the reference counting, I told
before,
> is removed.
> 
> According to https://groups.google.com/forum/#%21topic/v8-users/6kSAbnUb-rQ
and
> https://groups.google.com/d/topic/v8-users/ta9wkdEY08o/discussion I would say
> that we should not rely on it and we need some different mechanism of resource
> management for v8.

[Oleksandr, 2014-10-23 10:48:23]

I had unpublished draft comments pending here, which I didn't publish since
there was no clear solution anyway. I'll just publish them now for future
reference.

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
File include/AdblockPlus/V8ValueHolder.h (right):

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:76: this->value->MakeWeak(this->value,
(v8::WeakReferenceCallback)&AdblockPlus::V8ValueHolder<T>::ABPDisposeCallback);
I think ideally we should let V8 dispose the handle during GC, since it can
probably optimize the process and it's safer. Assuming that there are no other
instances referencing current handle is a bit of a wishful thinking. What if
somewhere in the code the V8ValueHolder will go out of scope, but the handle
itself would still be available? Like:

v8::Handle<v8::Value> someValue = ...;
if (true)
{
  V8ValueHolder vh(isolate, someValue);
}
// This would be problematic
someHandle->Foo();

However, seeing that it raises a whole bunch of problems with primitive types, I
guess it's better to leave that change for future.

On 2014/07/11 06:19:42, Wladimir Palant wrote:
> So, why is calling Dispose() an issue here? This V8ValueHolder instance is
> supposed to be the only place with a reference to this particular handle.
> 
> I see no copy assignment operator here, we should add it (rule of three).
> However, it doesn't see like any code is triggering it. My guess is that the
> real issue is the copy constructor declared above. It simply copies the v8
> handle into a new V8ValueHolder instance. As a result, that handle is owned by
> two instances and both will attempt to destroy it. We should create a new
handle
> there instead.

http://codereview.adblockplus.org/5691839784943616/diff/5629499534213120/incl...
include/AdblockPlus/V8ValueHolder.h:105: v8::Persistent<T>* value;
The reason of a raw pointer here is that we want to delete it in a callback, not
in destructor.

On 2014/07/11 09:22:20, Felix H. Dahlke wrote:
> On 2014/07/11 06:19:42, Wladimir Palant wrote:
> > I don't consider using raw pointers a good idea - as you've probably noticed
> for
> > this change, they add a fair amount of additional code and a fair amount of
> > additional potential failure points. And I don't see anything that's wrong
> with
> > auto_ptr here.
> 
> Agreed, auto_ptr is evidently what we want here: Defaults to 0, is deleted
> automatically invoking the correct destructor. It doesn't look like this
> behaviour has changed with your changes.