Bug 96659 - Evas_Object* is a ref'ed structure, so tread it as such
Summary: Evas_Object* is a ref'ed structure, so tread it as such
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit EFL (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kenneth Rohde Christiansen
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-09-13 09:09 PDT by Kenneth Rohde Christiansen
Modified: 2012-09-14 02:27 PDT (History)
10 users (show)

See Also:


Attachments
Patch (14.84 KB, patch)
2012-09-13 10:05 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (14.84 KB, patch)
2012-09-13 10:22 PDT, Kenneth Rohde Christiansen
no flags Details | Formatted Diff | Diff
Patch (19.04 KB, patch)
2012-09-13 13:45 PDT, Kenneth Rohde Christiansen
abarth: review+
kenneth: commit-queue-
Details | Formatted Diff | Diff
Patch (19.07 KB, patch)
2012-09-14 01:48 PDT, Kenneth Rohde Christiansen
gyuyoung.kim: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kenneth Rohde Christiansen 2012-09-13 09:09:58 PDT
Replace the OwnPtr support with that of RefPtr.
Comment 1 Kenneth Rohde Christiansen 2012-09-13 10:05:07 PDT
Created attachment 163897 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-09-13 10:11:55 PDT
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 :-(
Comment 3 Kenneth Rohde Christiansen 2012-09-13 10:22:45 PDT
Created attachment 163902 [details]
Patch
Comment 4 Adam Barth 2012-09-13 10:40:07 PDT
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.
Comment 5 Kenneth Rohde Christiansen 2012-09-13 10:41:17 PDT
Where do you add it then? I need to use a different header?
Comment 6 Kenneth Rohde Christiansen 2012-09-13 10:47:55 PDT
(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?
Comment 7 Adam Barth 2012-09-13 10:54:41 PDT
Yeah, integrating with RefPtr is fine.  It's better to just avoid adding cruft to RefPtr.h.
Comment 8 Kenneth Rohde Christiansen 2012-09-13 11:08:43 PDT
(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 ?
Comment 9 Adam Barth 2012-09-13 11:13:40 PDT
Sure.
Comment 10 Raphael Kubo da Costa (:rakuco) 2012-09-13 11:16:52 PDT
Does it make sense to keep both OwnPtr and RefPtr around?
Comment 11 Benjamin Poulain 2012-09-13 11:51:21 PDT
> 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
Comment 12 Kenneth Rohde Christiansen 2012-09-13 13:45:26 PDT
Created attachment 163955 [details]
Patch
Comment 13 Adam Barth 2012-09-13 13:50:38 PDT
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).
Comment 14 Kenneth Rohde Christiansen 2012-09-13 13:52:57 PDT
Comment on attachment 163955 [details]
Patch

They don't compile if not included
Comment 15 Thiago Marcos P. Santos 2012-09-13 14:06:21 PDT
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.
Comment 16 Kenneth Rohde Christiansen 2012-09-13 14:19:54 PDT
Landed in 128507
Comment 17 Chris Dumez 2012-09-13 23:57:22 PDT
This patch seems to introduce a lot of crashes. The bot is now aborting early after 20 crashes :(
Comment 18 Simon Hausmann 2012-09-14 00:05:26 PDT
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 :)
Comment 19 Gyuyoung Kim 2012-09-14 00:12:37 PDT
Reverted r128507 for reason:

Revert

Committed r128565: <http://trac.webkit.org/changeset/128565>
Comment 20 Chris Dumez 2012-09-14 00:22:29 PDT
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.
Comment 21 Kenneth Rohde Christiansen 2012-09-14 00:45:31 PDT
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
Comment 22 Kenneth Rohde Christiansen 2012-09-14 01:48:18 PDT
Created attachment 164071 [details]
Patch
Comment 23 Gyuyoung Kim 2012-09-14 01:57:45 PDT
Comment on attachment 164071 [details]
Patch

Please land this patch after double checking by Intel guys.
Comment 24 Chris Dumez 2012-09-14 02:07:02 PDT
I cannot reproduce the crash with the latest patch. LGTM. Thanks.
Comment 25 Kenneth Rohde Christiansen 2012-09-14 02:27:08 PDT
Landed in r128573