Bug 127246 - [GLIB] GVariant floating references are not correctly handled by GRefPtr
Summary: [GLIB] GVariant floating references are not correctly handled by GRefPtr
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 125990
  Show dependency treegraph
 
Reported: 2014-01-19 01:37 PST by Carlos Garcia Campos
Modified: 2014-01-20 06:44 PST (History)
5 users (show)

See Also:


Attachments
Patch (3.33 KB, patch)
2014-01-19 01:40 PST, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 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.
Comment 1 Carlos Garcia Campos 2014-01-19 01:40:40 PST
Created attachment 221582 [details]
Patch
Comment 2 Adrian Perez 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?
Comment 3 Carlos Garcia Campos 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
Comment 4 Adrian Perez 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.
Comment 5 Carlos Garcia Campos 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.
Comment 6 Carlos Garcia Campos 2014-01-19 23:57:05 PST
Committed r162306: <http://trac.webkit.org/changeset/162306>
Comment 7 Adrian Perez 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.
Comment 8 Adrian Perez 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