Bug 96659 – Evas_Object* is a ref'ed structure, so tread it as such

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kenneth Rohde Christiansen]

Replace the OwnPtr support with that of RefPtr.

[Kenneth Rohde Christiansen]

Created attachment 163897 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 163897 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163897&action=review

> Source/WTF/wtf/efl/RefPtrEfl.cpp:45
> +template<> void refIfNotNull(Evas_Object* ptr)
> +{
> +    if (LIKELY(!!ptr)) {
> +        evas_object_ref(ptr);
> +        evas_object_del(ptr); // Schedule for deletion when ref reaches 0;
> +    }
> +}
> +
> +template<> void derefIfNotNull(Evas_Object* ptr)
> +{
> +    if (LIKELY(!!ptr))
> +        evas_object_unref(ptr);
> +}

I guess this wont be working with adoptRef and then deref.

I guess I need to call _del always in derefIfNotNull :-(

[Kenneth Rohde Christiansen]

Created attachment 163902 [details]
Patch

[Adam Barth]

Comment on attachment 163902 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163902&action=review

> Source/WTF/wtf/RefPtr.h:212
> +#if PLATFORM(EFL)
> +typedef struct _Evas_Object Evas_Object;
> +
> +namespace WTF {
> +
> +template<> void refIfNotNull(Evas_Object* ptr);
> +template<> void derefIfNotNull(Evas_Object* ptr);
> +
> +}
> +#endif

I don't think we should be adding EFL-specific code to RefPtr.h.  Notice that we don't do the same thing for integration with CF ref counting.

[Kenneth Rohde Christiansen]

Where do you add it then? I need to use a different header?

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> Where do you add it then? I need to use a different header?

RefPtr are used like this for Cairo types, but it is only available in WebCore Source/WebCore/platform/graphics/cairo/RefPtrCairo.h

I guess this is an OK solution?

[Adam Barth]

Yeah, integrating with RefPtr is fine.  It's better to just avoid adding cruft to RefPtr.h.

[Kenneth Rohde Christiansen]

(In reply to comment #7)
> Yeah, integrating with RefPtr is fine.  It's better to just avoid adding cruft to RefPtr.h.

It is OK to add this in wtf/efl/RefPtrEfl.h so that it is accessible from say Tools/DumpRenderTree/efl/ImageDiff.cpp ?

[Adam Barth]

Sure.

[Raphael Kubo da Costa (:rakuco)]

Does it make sense to keep both OwnPtr and RefPtr around?

[Benjamin Poulain]

> I don't think we should be adding EFL-specific code to RefPtr.h.  Notice that we don't do the same thing for integration with CF ref counting.

+1

[Kenneth Rohde Christiansen]

Created attachment 163955 [details]
Patch

[Adam Barth]

Comment on attachment 163955 [details]
Patch

This looks great.  I presume things don't compile if you forget to include RefPtrEfl.h (rather than compile but be broken).

[Kenneth Rohde Christiansen]

Comment on attachment 163955 [details]
Patch

They don't compile if not included

[Thiago Marcos P. Santos]

I really like this change. OwnPtr will always compile, but at the risk of leaking memory if deleteOwnedPtr is not implemented for that particular type. As Adam pointed out, by using RefPtr, we add a compile time check.

[Kenneth Rohde Christiansen]

Landed in 128507

[Chris Dumez]

This patch seems to introduce a lot of crashes. The bot is now aborting early after 20 crashes :(

[Simon Hausmann]

Comment on attachment 163955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163955&action=review

> Source/WTF/wtf/efl/RefPtrEfl.cpp:36
> +        evas_object_del(ptr);

Shouldn't this only _unref_ and only delete if the refcount is zero? It looks like it's always deleting it on every deref :)

[Gyuyoung Kim]

Reverted r128507 for reason:

Revert

Committed r128565: <http://trac.webkit.org/changeset/128565>

[Chris Dumez]

Comment on attachment 163955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163955&action=review

>> Source/WTF/wtf/efl/RefPtrEfl.cpp:36
>> +        evas_object_del(ptr);
> 
> Shouldn't this only _unref_ and only delete if the refcount is zero? It looks like it's always deleting it on every deref :)

Just FYI, removing the _del() line seems to fix the issue for me. This is definitely related to it.

[Kenneth Rohde Christiansen]

Comment on attachment 163955 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=163955&action=review

>>> Source/WTF/wtf/efl/RefPtrEfl.cpp:36
>>> +        evas_object_del(ptr);
>> 
>> Shouldn't this only _unref_ and only delete if the refcount is zero? It looks like it's always deleting it on every deref :)
> 
> Just FYI, removing the _del() line seems to fix the issue for me. This is definitely related to it.

The ref counting is like "external" so it starts with a ref of 0, and unref only deletes if del was called before. So there is like an invisible ref :-)

it should be if (evas_object_ref_get() > 0) unref else del

[Kenneth Rohde Christiansen]

Created attachment 164071 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 164071 [details]
Patch

Please land this patch after double checking by Intel guys.

[Chris Dumez]

I cannot reproduce the crash with the latest patch. LGTM. Thanks.

[Kenneth Rohde Christiansen]

Landed in r128573