[GLIB] GVariant floating references are not correctly handled by GRefPtr

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

Created attachment 221582 [details]
Patch

[Adrian Perez]

(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]

You might want to adopt a full reference. We don't want to use adoptGRef with GVariants having a floating reference

[Adrian Perez]

(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]

(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]

Committed r162306: <http://trac.webkit.org/changeset/162306>

[Adrian Perez]

(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]

(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