127246 2014-01-19 01:37:11 -0800 [GLIB] GVariant floating references are not correctly handled by GRefPtr 2014-01-20 06:44:09 -0800 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED Gtk P2 Normal --- 125990 1 cgarcia webkit-unassigned aperez benjamin cmarcelo commit-queue gustavo oldest_to_newest 969583 0 cgarcia 2014-01-19 01:37:11 -0800 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. 969584 1 221582 cgarcia 2014-01-19 01:40:40 -0800 Created attachment 221582 Patch 969589 2 aperez 2014-01-19 08:05:31 -0800 (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? 969594 3 cgarcia 2014-01-19 09:16:48 -0800 You might want to adopt a full reference. We don't want to use adoptGRef with GVariants having a floating reference 969611 4 aperez 2014-01-19 11:08:05 -0800 (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. 969733 5 cgarcia 2014-01-19 23:45:56 -0800 (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. 969735 6 cgarcia 2014-01-19 23:57:05 -0800 Committed r162306: <http://trac.webkit.org/changeset/162306> 969824 7 aperez 2014-01-20 05:58:50 -0800 (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. 969831 8 aperez 2014-01-20 06:44:09 -0800 (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 221582 2014-01-19 01:40:40 -0800 2014-01-19 10:59:43 -0800 Patch wtf-floating-variant.diff text/plain 3407 cgarcia ZGlmZiAtLWdpdCBhL1NvdXJjZS9XVEYvQ2hhbmdlTG9nIGIvU291cmNlL1dURi9DaGFuZ2VMb2cK aW5kZXggZDdiYjcxZS4uMTE1YTM5OSAxMDA2NDQKLS0tIGEvU291cmNlL1dURi9DaGFuZ2VMb2cK KysrIGIvU291cmNlL1dURi9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNyBAQAorMjAxNC0wMS0xOSAg Q2FybG9zIEdhcmNpYSBDYW1wb3MgIDxjZ2FyY2lhQGlnYWxpYS5jb20+CisKKyAgICAgICAgW0dM SUJdIEdWYXJpYW50IGZsb2F0aW5nIHJlZmVyZW5jZXMgYXJlIG5vdCBjb3JyZWN0bHkgaGFuZGxl ZCBieSBHUmVmUHRyCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNn aT9pZD0xMjcyNDYKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAg ICAgICBHUmVmUHRyIHNob3VsZCBhbHdheXMgdXNlIGdfdmFyaWFudF9yZWZfc2luayB0byBkZWFs IHdpdGggR1ZhcmlhbnQKKyAgICAgICAgZmxvYXRpbmcgcmVmZXJlbmNlcy4gSW4gY2FzZSBvZiBm dWxsIHJlZmVyZW5jZXMsCisgICAgICAgIGdfdmFyaWFudF9yZWZfc2luayBjYWxscyBnX3Zhcmlh bnRfcmVmLCBzbyBpdCdzIHNhZmUgdG8gdXNlIGl0IGFsd2F5cy4KKworICAgICAgICAqIHd0Zi9n b2JqZWN0L0dSZWZQdHIuY3BwOgorICAgICAgICAoV1RGOjpyZWZHUHRyKTogVXNlIGdfdmFyaWFu dF9yZWZfc2luaygpLgorCiAyMDE0LTAxLTE3ICBDYXJsb3MgR2FyY2lhIENhbXBvcyAgPGNnYXJj aWFAaWdhbGlhLmNvbT4KIAogICAgICAgICBbR1RLXSBBZGQgR1VuaXF1ZVB0cgpkaWZmIC0tZ2l0 IGEvU291cmNlL1dURi93dGYvZ29iamVjdC9HUmVmUHRyLmNwcCBiL1NvdXJjZS9XVEYvd3RmL2dv YmplY3QvR1JlZlB0ci5jcHAKaW5kZXggYjZhMzFhYy4uNTJhOWEzOSAxMDA2NDQKLS0tIGEvU291 cmNlL1dURi93dGYvZ29iamVjdC9HUmVmUHRyLmNwcAorKysgYi9Tb3VyY2UvV1RGL3d0Zi9nb2Jq ZWN0L0dSZWZQdHIuY3BwCkBAIC05OCw3ICs5OCw3IEBAIHRlbXBsYXRlIDw+IHZvaWQgZGVyZWZH UHRyKEdCeXRlcyogcHRyKQogdGVtcGxhdGUgPD4gR1ZhcmlhbnQqIHJlZkdQdHIoR1ZhcmlhbnQq IHB0cikKIHsKICAgICBpZiAocHRyKQotICAgICAgICBnX3ZhcmlhbnRfcmVmKHB0cik7CisgICAg ICAgIGdfdmFyaWFudF9yZWZfc2luayhwdHIpOwogICAgIHJldHVybiBwdHI7CiB9CiAKZGlmZiAt LWdpdCBhL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxv ZwppbmRleCAwODBmZTZlLi42NzIwNTkwIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFu Z2VMb2cKKysrIGIvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTcgQEAKKzIw MTQtMDEtMTkgIENhcmxvcyBHYXJjaWEgQ2FtcG9zICA8Y2dhcmNpYUBpZ2FsaWEuY29tPgorCisg ICAgICAgIFtHTElCXSBHVmFyaWFudCBmbG9hdGluZyByZWZlcmVuY2VzIGFyZSBub3QgY29ycmVj dGx5IGhhbmRsZWQgYnkgR1JlZlB0cgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9MTI3MjQ2CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BT ISkuCisKKyAgICAgICAgRG8gbm90IGFkb3B0IEdWYXJpYW50IGZsb2F0aW5nIHJlZmVyZW5jZXMs IHRoZXkgd2lsbCBiZSBjb252ZXJ0ZWQKKyAgICAgICAgdG8gYSBmdWxsIHJlZmVyZW5jZSBieSBH UmVmUHRyLgorCisgICAgICAgICogcGxhdGZvcm0vZ3RrL1Bhc3RlYm9hcmRIZWxwZXIuY3BwOgor ICAgICAgICAoV2ViQ29yZTo6UGFzdGVib2FyZEhlbHBlcjo6ZmlsbFNlbGVjdGlvbkRhdGEpOgor ICAgICAgICAoV2ViQ29yZTo6UGFzdGVib2FyZEhlbHBlcjo6ZmlsbERhdGFPYmplY3RGcm9tRHJv cERhdGEpOgorCiAyMDE0LTAxLTE2ICBUaW0gSG9ydG9uICA8dGltb3RoeV9ob3J0b25AYXBwbGUu Y29tPgogCiAgICAgICAgIE9uIGlPUywgem9vbWluZyBpbiB3aXRoIGEgVGlsZUNvbnRyb2xsZXIt YmFja2VkIG1haW4gZnJhbWUgbWFrZXMgaHVuZHJlZHMgb2YgdGlsZXMKZGlmZiAtLWdpdCBhL1Nv dXJjZS9XZWJDb3JlL3BsYXRmb3JtL2d0ay9QYXN0ZWJvYXJkSGVscGVyLmNwcCBiL1NvdXJjZS9X ZWJDb3JlL3BsYXRmb3JtL2d0ay9QYXN0ZWJvYXJkSGVscGVyLmNwcAppbmRleCBhMzM1YTdkLi5m YTdiYzdiIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ndGsvUGFzdGVib2Fy ZEhlbHBlci5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3RrL1Bhc3RlYm9hcmRI ZWxwZXIuY3BwCkBAIC0xOTcsNyArMTk3LDcgQEAgdm9pZCBQYXN0ZWJvYXJkSGVscGVyOjpmaWxs U2VsZWN0aW9uRGF0YShHdGtTZWxlY3Rpb25EYXRhKiBzZWxlY3Rpb25EYXRhLCBndWludAogICAg ICAgICAgICAgZ192YXJpYW50X2J1aWxkZXJfYWRkX3BhcnNlZCgmYnVpbGRlciwgZGljdEl0ZW0u Z2V0KCkpOwogICAgICAgICB9CiAKLSAgICAgICAgR1JlZlB0cjxHVmFyaWFudD4gdmFyaWFudCA9 IGFkb3B0R1JlZihnX3ZhcmlhbnRfYnVpbGRlcl9lbmQoJmJ1aWxkZXIpKTsKKyAgICAgICAgR1Jl ZlB0cjxHVmFyaWFudD4gdmFyaWFudCA9IGdfdmFyaWFudF9idWlsZGVyX2VuZCgmYnVpbGRlcik7 CiAgICAgICAgIEdPd25QdHI8Z2NoYXI+IHNlcmlhbGl6ZWRWYXJpYW50KGdfdmFyaWFudF9wcmlu dCh2YXJpYW50LmdldCgpLCBUUlVFKSk7CiAgICAgICAgIGd0a19zZWxlY3Rpb25fZGF0YV9zZXQo c2VsZWN0aW9uRGF0YSwgdW5rbm93bkF0b20sIDEsIHJlaW50ZXJwcmV0X2Nhc3Q8Y29uc3QgZ3Vj aGFyKj4oc2VyaWFsaXplZFZhcmlhbnQuZ2V0KCkpLCBzdHJsZW4oc2VyaWFsaXplZFZhcmlhbnQu Z2V0KCkpKTsKICAgICB9CkBAIC0yNTYsNyArMjU2LDcgQEAgdm9pZCBQYXN0ZWJvYXJkSGVscGVy OjpmaWxsRGF0YU9iamVjdEZyb21Ecm9wRGF0YShHdGtTZWxlY3Rpb25EYXRhKiBkYXRhLCBndWlu dAogICAgICAgICBpZiAocGllY2VzLnNpemUoKSA+IDEpCiAgICAgICAgICAgICBkYXRhT2JqZWN0 LT5zZXRUZXh0KHBpZWNlc1sxXSk7CiAgICAgfSBlbHNlIGlmICh0YXJnZXQgPT0gdW5rbm93bkF0 b20pIHsKLSAgICAgICAgR1JlZlB0cjxHVmFyaWFudD4gdmFyaWFudCA9IGFkb3B0R1JlZihnX3Zh cmlhbnRfbmV3X3BhcnNlZChyZWludGVycHJldF9jYXN0PGNvbnN0IGNoYXIqPihndGtfc2VsZWN0 aW9uX2RhdGFfZ2V0X2RhdGEoZGF0YSkpKSk7CisgICAgICAgIEdSZWZQdHI8R1ZhcmlhbnQ+IHZh cmlhbnQgPSBnX3ZhcmlhbnRfbmV3X3BhcnNlZChyZWludGVycHJldF9jYXN0PGNvbnN0IGNoYXIq PihndGtfc2VsZWN0aW9uX2RhdGFfZ2V0X2RhdGEoZGF0YSkpKTsKIAogICAgICAgICBHT3duUHRy PGdjaGFyPiBrZXk7CiAgICAgICAgIEdPd25QdHI8Z2NoYXI+IHZhbHVlOwo=