Should never be able to call `get_ref` on an object with a refcount of 0

[dymk]

Specific case: Should never be able to get a reference to this in a constructor, because we might try to deallocate ourselves in the constructor. We can check if the refcount is 0 in get_ref, and fail loudly at the callsite.

[dymk]

see #666

[phoad]

It looks like we have a flaw in RefCounted class:

#666
"The problem might be that, which is similar to shared_ptr's.
http://en.cppreference.com/w/cpp/memory/enable_shared_from_this
Check this link. In there it says that, if we call shared_from_this, from an instance which is not a shared_ptr, it causes undefined behavior.

So I need to ask, if we call get_ref(this), but if there is no Reference outside of this, then if we call get_ref - increase count to 1, when we lose this ref, count goes to 0, which deletes the object.

So the problem is somewhere we create an instance of the Emitter without a reference! Which is the constructor of the Emitter already. Namely even if we have Reference, at the constructor of the Emitter, there is nothing that is incrementing the count of the reference.

I believe the inc_count and dec_count in the code in other places were specifically for this purpose. So we need inc_cnt / dec_cnt in here too. Perhaps we need to revert the old update that has removed these two calls from another class."

Even if we call make_ref to create an instance, if we call get_ref in the constructor, it will cause issue, which should not cause the issue at least, which is why we have a bug in here!

Given class T, whose constructor is:

T::T() {  yarpl::get_ref(this); }

will always fail if we do:

Reference<T> ref;
ref->foo();

ref will always be deleted when its constructed.
So we need to throw when we call get_ref on an object which has no reference to it already.

But we should not be failing if we call make_ref, though we still fail :(. I will check how this is done correctly for std::shared_ptr.

[phoad]

I have thought a bit about the issue and I have a proposal about the solution:

• Setting the count to 1 at the constructor of Refcounted and setting it to 0 at the destructor.
• Moving the T object at the constructor of the Reference object, or at least don't call inc() if T is type of Refcounted which is a must.

  explicit Reference(T* pointer) : pointer_(pointer) {
    inc();
  }

This should be converted to std::move, and inc() should not be called. At least, even if we don't move, we should not call inc() in here. Why: because if we create two shared_ptr objects to the same pointer, and if we delete one of the shared_ptr objects, it will go and delete the original object.

* Constructing more than one std::shared_ptr from a single raw pointer causes undefined behavior!
auto pw = new Widget;
std::shared_ptr<Widget> sp1(pw, loggingDel);
std::shared_ptr<Widget> sp2(pw, loggingDel); // second destruction of pw!! -> undefined behavior

• Already the assignment operator or copy constructor of the Reference is incrementing the count.

Why:

At the constructor of Refcounted, we can set the initial count to 1. At the destructor of it we can set the counter to 0, as it is getting destroyed.
So if we don't reference an object

{
T t1(3);
t1.foo();
}

it will be pure construction and destruction. The count is not used already. If we call get_ref at the inside of the constructor, then it will increment the count to 2, as that reference is deleted it will get back to 1, so the object will not be deleted at the constructor because of calling get_ref at the constructor.

If we create reference to it, we will move it:

{
Reference<T> t1(new T());
t1->foo();
}

as we move it, the count will still be 1. As the Reference is deleted, the count will go to 0 and the object will be deleted.

But if we create another reference from the old reference, then count should increase

Reference<T> t2 = t1;
delete t1; // t2 still references

So the count became 2 and it is decremented to 1. The object is still alive.

• If we call get_ref, even if we are at the constructor, the count was 1, it will be incremented to 2. Ok!
• If we call make_ref(T*), we should still move the T, which will keep the count value still at 1.

[dymk]

fixed with #672