Change the way persistent handles are desctructed. Add tests.

include/AdblockPlus/V8ValueHolder.h

 /*
  * This file is part of Adblock Plus <http://adblockplus.org/>,
  * Copyright (C) 2006-2014 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #ifndef ADBLOCK_PLUS_V8_VALUE_HOLDER_H
 #define ADBLOCK_PLUS_V8_VALUE_HOLDER_H
 
 #include <memory>
 
 namespace v8
 {
   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);
       }
     }
 
     void reset(v8::Isolate* isolate, v8::Handle<T> value)
     {
       reset(isolate, v8::Persistent<T>::New(isolate, value));
     }
 
     T* operator->() const
     {
       return **value;

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

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?

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

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.

     }
     operator v8::Handle<T>() const

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

I'm not sure whether we want to return the copy or const reference here, now
it's a copy.

     {
       return *value;
     }
     operator v8::Persistent<T>() const
     {
       return *value;
     }
   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:

   };
 }
 
 #endif