Change the way persistent handles are desctructed. Add tests.

include/AdblockPlus/V8ValueHolder.h

Index: include/AdblockPlus/V8ValueHolder.h
===================================================================
--- a/include/AdblockPlus/V8ValueHolder.h
+++ b/include/AdblockPlus/V8ValueHolder.h
@@ -25,46 +25,61 @@
   class Isolate;
   template<class T> class Handle;
   template<class T> class Persistent;
+  class Value;
 }
 
 namespace AdblockPlus
 {
+

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

Superfluous whitespace.

   template<class T>
   class V8ValueHolder
   {
   public:
     V8ValueHolder()
     {
+      value = NULL;

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

Note that we generally use 0 rather than NULL.

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

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.

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

All members should be initialized in the ctr (in all constructors), `isolate` as
well.

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

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

       reset(0, v8::Persistent<T>());
     }
     V8ValueHolder(V8ValueHolder& value)

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

Why is not const?

     {
+      this->value = NULL;
       reset(value.isolate, static_cast<v8::Handle<T> >(value));
     }
     V8ValueHolder(v8::Isolate* isolate, v8::Persistent<T> value)
     {
+      this->value = NULL;
       reset(isolate, value);
     }
     V8ValueHolder(v8::Isolate* isolate, v8::Handle<T> value)
     {
+      this->value = NULL;
       reset(isolate, value);
     }
     ~V8ValueHolder()
     {
       reset(isolate, v8::Persistent<T>()); 

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

Superfluous space at the end of the line.

     }
+
+    static void ABPDisposeCallback(v8::Persistent<v8::Value> object, void* parameter)
+    {
+      v8::Persistent<v8::Value>* objectPointer = (v8::Persistent<v8::Value>*)parameter;

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

static cast

+      delete objectPointer;
+
+      object.Dispose();

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

In the docs it's said we should use ``Dispose(isolate)`.

+      object.Clear();
+    }
+
     void reset(v8::Isolate* isolate, v8::Persistent<T> value)
     {
-      if (this->value.get())
+      if (this->value != NULL)
       {
-        this->value->Dispose(this->isolate);
-        this->value.reset(0);
+        this->value->MakeWeak(this->value, (v8::WeakReferenceCallback)&AdblockPlus::V8ValueHolder<T>::ABPDisposeCallback); 

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

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.

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

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. 

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

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]

The recommended version is with `isolate`.

[Oleksandr, 2014/10/23 10:48:24]

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:

       }
 
       if (!value.IsEmpty())
       {
         this->isolate = isolate;
-        this->value.reset(new v8::Persistent<T>(value));
+        this->value = new v8::Persistent<T>(value);
       }
     }
 
@@ -87,7 +102,7 @@
     }
   private:
     v8::Isolate* isolate;
-    std::auto_ptr<v8::Persistent<T> > value;
+    v8::Persistent<T>* value;

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

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]

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.

[Oleksandr, 2014/10/23 10:48:24]

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:

   };
 }