WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
127246
[GLIB] GVariant floating references are not correctly handled by GRefPtr
https://bugs.webkit.org/show_bug.cgi?id=127246
Summary
[GLIB] GVariant floating references are not correctly handled by GRefPtr
Carlos Garcia Campos
Reported
2014-01-19 01:37:11 PST
GRefPtr should always use g_variant_ref_sink to deal with GVariant floating references. In case of full references, g_variant_ref_sink calls g_variant_ref, so it's safe to use it always.
Attachments
Patch
(3.33 KB, patch)
2014-01-19 01:40 PST
,
Carlos Garcia Campos
mrobinson
: review+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Carlos Garcia Campos
Comment 1
2014-01-19 01:40:40 PST
Created
attachment 221582
[details]
Patch
Adrian Perez
Comment 2
2014-01-19 08:05:31 PST
(In reply to
comment #0
)
> GRefPtr should always use g_variant_ref_sink to deal with GVariant floating references. In case of full references, g_variant_ref_sink calls g_variant_ref, so it's safe to use it always.
Being this the case, if I am understanding GRefPtr correctly then we should never use adoptGRef() for GVariants, because it does *not* sink the floating reference or add a reference. If that's the case it would be good to add a specialization of the adoptGRef() template to make code that uses it work properly with GVariants. I see two possible ways: a. Specialize adoptGRef<GVariant>() to *not* pass GRefPtrAdopt as second argument. This would make adoptGRef() sink the floating reference: template<> GRefPtr<GVariant> adoptGRef(GVariant* p) { return GRefPtr<GVariant>(p); } b. Make the compiler stop with error when using adoptGRef<GVariant>(). For example using a “static_assert” depending on the type used for GRefPtr. This would prevent using adoptGRef() for GVariants. For example the following would cause compiler errors when instantiating adoptGRef() for GVariants: // In general, adoptGRef() can be used for any type. template <typename T> class GRefPtr { // ... static const bool canUseAdoptGRef = true; }; // But not for GVariant, so specialize the value of the flag. template <> class GRefPtr<GVariant> { static const bool canUseAdoptGRef = false; }; template <typename T> GRefPtr<T> adoptGRef(T* p) { static_assert(GRefPtr<T>::canUseAdoptGRef, "Can't use adoptGRef() for this type"); return GRefPtr<T>(p, GRefPtrAdopt); } Probably option (a) is the less intrusive, and it would avoid having to go through the existing code chasing the existing uses of adoptGRef(). WDYT?
Carlos Garcia Campos
Comment 3
2014-01-19 09:16:48 PST
You might want to adopt a full reference. We don't want to use adoptGRef with GVariants having a floating reference
Adrian Perez
Comment 4
2014-01-19 11:08:05 PST
(In reply to
comment #3
)
> You might want to adopt a full reference. We don't want to use adoptGRef with GVariants having a floating reference
Then, could you please add something like the solution outlined in (b) to make it a compiler error to use adoptGRef() on a GVariant? That would prevent all of us of making the mistake.
Carlos Garcia Campos
Comment 5
2014-01-19 23:45:56 PST
(In reply to
comment #4
)
> (In reply to
comment #3
) > > You might want to adopt a full reference. We don't want to use adoptGRef with GVariants having a floating reference > > Then, could you please add something like the solution outlined in (b) > to make it a compiler error to use adoptGRef() on a GVariant? That would > prevent all of us of making the mistake.
Using adoptGRef with a GVariant is not a mistake, if the GVariant has a full reference and you want to adopt it. That's what happens with some methods like g_dbus_proxy_call_sync that return a full reference that you want to adopt. We might check if the GVariant has a floating reference and assert (or warning) in that case, but I don't think it's worth it, TBH. You are supposed to know how it works, like with GtkWidget, even when not using smart pointers. If it helps I can document in the wiki how to use initially unowned references with our smart pointers.
Carlos Garcia Campos
Comment 6
2014-01-19 23:57:05 PST
Committed
r162306
: <
http://trac.webkit.org/changeset/162306
>
Adrian Perez
Comment 7
2014-01-20 05:58:50 PST
(In reply to
comment #5
)
> (In reply to
comment #4
) > > (In reply to
comment #3
) > > > You might want to adopt a full reference. We don't want to use adoptGRef with GVariants having a floating reference > > > > Then, could you please add something like the solution outlined in (b) > > to make it a compiler error to use adoptGRef() on a GVariant? That would > > prevent all of us of making the mistake. > > Using adoptGRef with a GVariant is not a mistake, if the GVariant has a full reference and you want to adopt it. That's what happens with some methods like g_dbus_proxy_call_sync that return a full reference that you want to adopt. We might check if the GVariant has a floating reference and assert (or warning) in that case, but I don't think it's worth it, TBH. You are supposed to know how it works, like with GtkWidget, even when not using smart pointers. If it helps I can document in the wiki how to use initially unowned references with our smart pointers.
Ah, understood. I am writing a wiki page explaining how to use GRefPtr so it is easier for people starting to hack in WebKitGTK+ to use it properly.
Adrian Perez
Comment 8
2014-01-20 06:44:09 PST
(In reply to
comment #7
)
> (In reply to
comment #5
) > > (In reply to
comment #4
) > > > (In reply to
comment #3
) > > > > You might want to adopt a full reference. We don't want to use adoptGRef with GVariants having a floating reference > > > > > > Then, could you please add something like the solution outlined in (b) > > > to make it a compiler error to use adoptGRef() on a GVariant? That would > > > prevent all of us of making the mistake. > > > > Using adoptGRef with a GVariant is not a mistake, if the GVariant has a full reference and you want to adopt it. That's what happens with some methods like g_dbus_proxy_call_sync that return a full reference that you want to adopt. We might check if the GVariant has a floating reference and assert (or warning) in that case, but I don't think it's worth it, TBH. You are supposed to know how it works, like with GtkWidget, even when not using smart pointers. If it helps I can document in the wiki how to use initially unowned references with our smart pointers. > > Ah, understood. I am writing a wiki page explaining how to use GRefPtr > so it is easier for people starting to hack in WebKitGTK+ to use it > properly.
Done:
https://trac.webkit.org/wiki/GRefPtr
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug