Page MenuHomePhabricator

eo: fix to free unmanaged handle of language bindings manually in eo
Needs RevisionPublic

Authored by Jaehyun_Cho on Jul 12 2019, 1:26 AM.

Details

Summary

Previously, there were 2 problems on ref/unref of unmanaged handle of
language bindings.

  1. ref and unref may not be matched for unmanaged handle
    • MarshalEo.MarshalManagedToNative with OwnTag (@owned in eo) ref the unmanaged handle but there is no way to unref it later.
  2. unmanaged handle can survive until GC collects the memory

Now, unmanaged handle is unref manually after EFL_EVENT_OWNERSHIP_UNIQUE
is called in efl_unref. Moreover, _efl_unref_internal decreases
user_refcount if it is unmanaged handle of language bindings and it is
supposed to be free.

By the above fix, unmanaged handle of language bindings is free manually
in eo.

Test Plan

Please test the following code with C# language bindings.
When issue happens, BUTTON1 cannot be clicked after BUTTON2 is clicked.

using System;

namespace Spotlight
{

class MainClass
{
    public static void Main (string[] args)
    {
        Efl.All.Init (Efl.Csharp.Components.Ui);

        Efl.Ui.Win win = new Efl.Ui.Win (null);
        win.SetSize (new Eina.Size2D (200, 200));

        Efl.Ui.Spotlight.Container container = new Efl.Ui.Spotlight.Container (win);
        Efl.Ui.Spotlight.ManagerStack vm = new Efl.Ui.Spotlight.ManagerStack (container);
        container.SetSpotlightManager (vm);

        Efl.Ui.Button btn = new Efl.Ui.Button (container);
        btn.SetText ("BUTTON1");
        btn.ClickedEvt += (object sender, Efl.Ui.IClickableClickedEvt_Args e) => 
        {
        };
        container.Push (btn);

        Efl.Ui.Spotlight.Container container2 = new Efl.Ui.Spotlight.Container (container);
        Efl.Ui.Spotlight.ManagerScroll vm2 = new Efl.Ui.Spotlight.ManagerScroll (container2);
        container2.SetSpotlightManager (vm2);

        Efl.Ui.Button btn2 = new Efl.Ui.Button (container2);
        btn2.SetText ("BUTTON2");
        btn2.ClickedEvt += (object sender, Efl.Ui.IClickableClickedEvt_Args e) => 
        {
            container.Pop(true);
        };
        container2.Push (btn2);

        container.Push (container2);

        win.SetContent (container);

        Efl.Ui.Config.Run ();
        Efl.All.Shutdown ();
    }
}

}

Diff Detail

Repository
rEFL core/efl
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12184
Build 8907: arc lint + arc unit
Jaehyun_Cho created this revision.Jul 12 2019, 1:26 AM
Jaehyun_Cho requested review of this revision.Jul 12 2019, 1:26 AM
bu5hm4n requested changes to this revision.Jul 12 2019, 3:07 AM
bu5hm4n added a subscriber: bu5hm4n.
bu5hm4n added inline comments.
src/lib/eo/eo.c
2015

Can you elaborate this ? I do not think that we should unref in eo at any point where we did not take a ref *Before* in eo.

src/lib/eo/eo_private.h
351

Same as above.

Additionally, this seems to be in the wrong function, there are two refcounts in eo the "refcount" and "user_refcount" this here seems to have mixed both of them.

This revision now requires changes to proceed.Jul 12 2019, 3:07 AM
Jaehyun_Cho added inline comments.Jul 12 2019, 4:08 AM
src/lib/eo/eo.c
2015

This case is only for language binding case when the handle actually has no user_refcount.

As you might know, in C# language bindings, C# instance calls efl_ref() for eo handle not to destruct eo handle before C# instance is destructed.
Because C# instance uses eo handle to call efl C functions.
Moreover, ownership_track is set to be true only if OWNERSHIP_UNIQUE callback is added which is only used for language bindings.

Therefore, else if (EINA_UNLIKELY(obj->ownership_track && obj->user_refcount == 1)) case is the same as the above if (EINA_UNLIKELY((obj->user_refcount <= 0))).

But previously, _efl_unref() was not called because we wait for GC collects the C# instance memory.
However, in this patch, I modified to call _efl_unref() in the same way of C code routine.

src/lib/eo/eo_private.h
351

The reason is same as above.

if (EINA_UNLIKELY(obj->refcount <= 0)) and if (obj->ownership_track && (obj->user_refcount == 1)) case means the eo handle is only referenced by C# instance and it should be free.

But still obj->user_refcount is 1 so it cannot be free because of the following conditional expression.
So I decreases user_refcount if it is the case that eo handle is only referenced by C# instance and it should be free.

Can you tell me if i am understanding you currently:

  • When a eo object is created from C# or C and it is used in C#, the wrapper will get a reference on it?
  • When the eo object is freed from the C side, this patch makes eo automatically unrefs this 1 reference that was earlier taken by C# ?

Can you tell me if i am understanding you currently:

  • When a eo object is created from C# or C and it is used in C#, the wrapper will get a reference on it?

yes, not to free eo before C# instance is destructed.

  • When the eo object is freed from the C side, this patch makes eo automatically unrefs this 1 reference that was earlier taken by C# ?

yes, to free that eo in C code.

Okay, please, take a step back and reevaluate this. If you chain the unref with the unrefing of the previous ref (which this here is doing), then those two references are basically *one* reference. There is no way that the reference that was taken by C# can overlife the other reference taken by anyone else. Which effectively means, you can just remove the initial reference that C# took on this object. So this patch here makes the reftaking of C# completly useless, in the case that this here is resulting in correct behaviour for you in C#, then please remove the efl_ref that is in C#.

Okay, please, take a step back and reevaluate this. If you chain the unref with the unrefing of the previous ref (which this here is doing), then those two references are basically *one* reference. There is no way that the reference that was taken by C# can overlife the other reference taken by anyone else. Which effectively means, you can just remove the initial reference that C# took on this object. So this patch here makes the reftaking of C# completly useless, in the case that this here is resulting in correct behaviour for you in C#, then please remove the efl_ref that is in C#.

Hmm, I don't understand your comment here.

In this case, unref of obj->user_refcount is done only if the obj->user_refcount is 1. When an obj is constructed, obj->refcount = 1 and obj->user_refcount = 1. And C# instance adds obj->user_refcount by 1.
Therefore, if obj->user_refcount is 1, then all the increased obj->user_refcount by user gets already unref and the obj->user_refcount is actually 0 except the C# instance's user_refcount 1.
This means that the obj should be free by the unref so obj->user_refcount is decreased by 1 to finish unref correctly.

The _efl_unref() inside "else if (EINA_UNLIKELY(obj->ownership_track && obj->user_refcount == 1))" is exactly the same as _efl_unref() inside "if (EINA_UNLIKELY((obj->user_refcount <= 0)))".
As you know, the unref work is done by _efl_unref() in efl_unref().

BTW, since I did not design this ref/unref stuff of eo for C# instance and I just tried to find a solution based on this ref/unref stuff, now I do not have other good solutions which do not break the current C# logic (i.e. keeping eo until C# instance is destructed).

Let me give you an example, lets say we have a eo object with ownership_track=1:
Now this:

efl_ref(eo);
efl_ref(eo);
[...]
efl_unref(eo);

is totally equal to this (just without this patch here):

efl_ref(eo);
[...]
efl_unref(eo);

So i totally fail to understand, why this needs a patch to eo itself, and not just removes that efl_ref from C#. Because with this, there is no reason for the initial reference that is taken, it is removed anyways if the second ref is removed.

Additionally:

  • ownership_track=1 is happening when you attach events to a object, attaching a event to a object should never have a sideeffect like *oh yeah, we are gonna remove one random reference at some point*. This is super confusing and unexpected.
  • A reference *must* be taken by the same entity that releases it. Everything else leaves everything undebuggable.
  • _efl_unref deals with refcount which is the internal reference counter, which is used for example for keeping the object alive across function calls. user_refcount however is only used by the user, not by internals of eo.c or eo_private.h.
  • The last counter in refcount belongs to to the user_refcount (check eo.c:1947 & eo.c:2007) with this change, the last refcount is taken away, while the user_refcount is still one, which kills the emittation of EFL_EVENT_NOREF.

-

@Jaehyun_Cho D9306 fixes the issue in the spotlight widget, the underlaying issue is still present, but please not solved like this :)

@Jaehyun_Cho D9306 fixes the issue in the spotlight widget, the underlaying issue is still present, but please not solved like this :)

So, to make the issue clearer:

  • C# keeps a reference to the Eo objects so it can call methods on it, even after passing it to @owned methods.
  • This reference is last released when the C# wrapper is garbage collected, which can take some time to happen.
  • Some Spotlight classes relied on the C destructor being called to actually remove themselves from the screen
  • As the C destructor was not called, the widgets were not cleared.

D9306 fixes this by moving the cleaning process to the Invalidate method of the Spotlight classes, which isn't delayed by the GC.

During the Ownership Shared/Unique original discussion it was raised the point to call Dispose (which releases C# reference) when INVALIDATE is raised but it was decided that the developer could still want to receive some events after Invalidate.