213094 2020-06-11 15:09:39 -0700 [GTK] Use GRefPtr for ManetteMonitor 2020-06-15 09:42:35 -0700 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 133847 1 changseok changseok alex bugs-noreply calvaris cgarcia oldest_to_newest 1661748 0 changseok 2020-06-11 15:09:39 -0700 ManetteMonitor is a GObject that has a reference counter. So, GRefPtr is used instead of GUniquePtr. 1661771 1 401688 changseok 2020-06-11 15:59:24 -0700 Created attachment 401688 Patch 1661869 2 calvaris 2020-06-11 23:36:59 -0700 At a first glance the doc is confusing and I cannot see if manette_monitor_new returns transfer full or floating. If it's floating, no problem. If it's full, you need to change ManetteGamepadProvider.cpp:59 to adopt. 1661874 3 401688 cgarcia 2020-06-12 01:06:37 -0700 Comment on attachment 401688 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=401688&action=review > Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.h:72 > - GUniquePtr<ManetteMonitor> m_monitor; > + GRefPtr<ManetteMonitor> m_monitor; This is not enough, you should adopt the ref in the constructor. 1662351 4 changseok 2020-06-13 07:45:52 -0700 (In reply to Carlos Garcia Campos from comment #3) > Comment on attachment 401688 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=401688&action=review > > > Source/WebCore/platform/gamepad/manette/ManetteGamepadProvider.h:72 > > - GUniquePtr<ManetteMonitor> m_monitor; > > + GRefPtr<ManetteMonitor> m_monitor; > > This is not enough, you should adopt the ref in the constructor. I am confused with a refcount of the ManetteMonitor instance here. manette_monitor_new returns a full object. As my understanding the gobject in terms of its refcount, it looks like... manette_monitor_new // its refcount is 1 when the monitor is created. adoptGRef(manette_monitor_new()) // refcount is still 1 since adoptGRef does not increase the refcount. m_monitor(adoptGRef(manette_monitor_new())) // refcount is 2 since the copy constructor increases the refcount by 1. In the destructor of ManetteGamepadProvider: the refcount of m_monitor will be decreased by 1 so the monitor would be eventually leaked since its refcount would be still 1? What am I missing? 1662353 5 changseok 2020-06-13 08:06:59 -0700 (In reply to ChangSeok Oh from comment #4) > m_monitor(adoptGRef(manette_monitor_new())) // refcount is 2 since the copy constructor increases the refcount by 1. Aha.. maybe rvalue reference copy constructor is called here? i.e., GRefPtr(GRefPtr&& o) : m_ptr(o.leakRef()) { } 1662354 6 401836 changseok 2020-06-13 08:17:03 -0700 Created attachment 401836 Patch 1662600 7 calvaris 2020-06-15 02:54:08 -0700 (In reply to ChangSeok Oh from comment #5) > (In reply to ChangSeok Oh from comment #4) > > m_monitor(adoptGRef(manette_monitor_new())) // refcount is 2 since the copy constructor increases the refcount by 1. > > Aha.. maybe rvalue reference copy constructor is called here? i.e., > GRefPtr(GRefPtr&& o) : m_ptr(o.leakRef()) { } Yes. 1662674 8 ews-feeder 2020-06-15 09:42:34 -0700 Committed r263040: <https://trac.webkit.org/changeset/263040> All reviewed patches have been landed. Closing bug and clearing flags on attachment 401836. 401688 2020-06-11 15:59:24 -0700 2020-06-13 08:17:00 -0700 Patch bug-213094-20200611185924.patch text/plain 1656 changseok U3VidmVyc2lvbiBSZXZpc2lvbjogMjYyOTI1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYWI2NjVjODViZjNiNjgw ZGRhMjY3YjQ4MDg2MjI5YmQ1NWRhZWY2Ny4uYzM2NGNiYzEyMjU1MGQyOTY4Y2IxM2Y5MjA3MGY5 Y2FlMjVjNjQxNiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDIwLTA2LTExICBDaGFu Z1Nlb2sgT2ggIDxjaGFuZ3Nlb2tAd2Via2l0Lm9yZz4KKworICAgICAgICBbR1RLXSBVc2UgR1Jl ZlB0ciBmb3IgTWFuZXR0ZU1vbml0b3IKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTIxMzA5NAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIE1hbmV0dGVNb25pdG9yIGlzIGEgZ29iamVjdCB0aGF0IHJldGFpbnMg YSByZWZlcmVuY2UgY291bnQgaW5zaWRlLiBTbywgd2Ugc2hvdWxkIHVzZSBHUmVmUHRyLCBub3Qg R1VuaXF1ZVB0ci4KKworICAgICAgICBObyBuZXcgdGVzdHMgYmVjYXVzZSBvZiBubyBmdW5jdGlv bmFsaXR5IGNoYW5nZWQuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9nYW1lcGFkL21hbmV0dGUvTWFu ZXR0ZUdhbWVwYWRQcm92aWRlci5oOgorCiAyMDIwLTA2LTExICBSb2IgQnVpcyAgPHJidWlzQGln YWxpYS5jb20+CiAKICAgICAgICAgUkVHUkVTU0lPTiAocjI2Mjc3Nik6IEZsYWt5IGNyYXNoIHVu ZGVyIC1bV2ViQ29yZVJlc291cmNlSGFuZGxlQXNPcGVyYXRpb25RdWV1ZURlbGVnYXRlIGNvbm5l Y3Rpb246d2lsbFNlbmRSZXF1ZXN0OnJlZGlyZWN0UmVzcG9uc2U6XQpkaWZmIC0tZ2l0IGEvU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ2FtZXBhZC9tYW5ldHRlL01hbmV0dGVHYW1lcGFkUHJvdmlk ZXIuaCBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dhbWVwYWQvbWFuZXR0ZS9NYW5ldHRlR2Ft ZXBhZFByb3ZpZGVyLmgKaW5kZXggMjllMjgxOTYyMTYwMzU1ZmRlYWM5Yzg3ZmI2Y2YwMWZiMDE0 MjQzYy4uYmY3YTdkZGVkMzZkZTUwM2RhMjgxODNkZDIxZTczZmVjYmZhZGE2OSAxMDA2NDQKLS0t IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ2FtZXBhZC9tYW5ldHRlL01hbmV0dGVHYW1lcGFk UHJvdmlkZXIuaAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9nYW1lcGFkL21hbmV0dGUv TWFuZXR0ZUdhbWVwYWRQcm92aWRlci5oCkBAIC02OSw3ICs2OSw3IEBAIHByaXZhdGU6CiAgICAg SGFzaE1hcDxNYW5ldHRlRGV2aWNlKiwgc3RkOjp1bmlxdWVfcHRyPE1hbmV0dGVHYW1lcGFkPj4g bV9nYW1lcGFkTWFwOwogICAgIGJvb2wgbV9zaG91bGREaXNwYXRjaENhbGxiYWNrcyB7IGZhbHNl IH07CiAKLSAgICBHVW5pcXVlUHRyPE1hbmV0dGVNb25pdG9yPiBtX21vbml0b3I7CisgICAgR1Jl ZlB0cjxNYW5ldHRlTW9uaXRvcj4gbV9tb25pdG9yOwogICAgIFJ1bkxvb3A6OlRpbWVyPE1hbmV0 dGVHYW1lcGFkUHJvdmlkZXI+IG1fY29ubmVjdGlvbkRlbGF5VGltZXI7CiAgICAgUnVuTG9vcDo6 VGltZXI8TWFuZXR0ZUdhbWVwYWRQcm92aWRlcj4gbV9pbnB1dE5vdGlmaWNhdGlvblRpbWVyOwog fTsK 401836 2020-06-13 08:17:03 -0700 2020-06-15 09:42:34 -0700 Patch bug-213094-20200613111702.patch text/plain 2501 changseok U3VidmVyc2lvbiBSZXZpc2lvbjogMjYyOTI1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYWI2NjVjODViZjNiNjgw ZGRhMjY3YjQ4MDg2MjI5YmQ1NWRhZWY2Ny4uZjQ1ZDQ4NzRjNjgxM2NhMWE2OTNiZTIwNjk1MWMy ODc4MzY2YmY1MyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDIwLTA2LTExICBDaGFu Z1Nlb2sgT2ggIDxjaGFuZ3Nlb2tAd2Via2l0Lm9yZz4KKworICAgICAgICBbR1RLXSBVc2UgR1Jl ZlB0ciBmb3IgTWFuZXR0ZU1vbml0b3IKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcv c2hvd19idWcuY2dpP2lkPTIxMzA5NAorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09Q UyEpLgorCisgICAgICAgIFdlIHNob3VsZCB1c2UgR1JlZlB0ciBmb3IgTWFuZXR0ZU1vbml0b3Is IGJ1dCBub3QgR1VuaXF1ZVB0ci4gQmVjYXVzZSBpdCBpcyBhIGdvYmplY3QgaW5zdGFuY2UuCisK KyAgICAgICAgTm8gbmV3IHRlc3RzIGJlY2F1c2Ugb2Ygbm8gZnVuY3Rpb25hbGl0eSBjaGFuZ2Vk LgorCisgICAgICAgICogcGxhdGZvcm0vZ2FtZXBhZC9tYW5ldHRlL01hbmV0dGVHYW1lcGFkUHJv dmlkZXIuaDoKKwogMjAyMC0wNi0xMSAgUm9iIEJ1aXMgIDxyYnVpc0BpZ2FsaWEuY29tPgogCiAg ICAgICAgIFJFR1JFU1NJT04gKHIyNjI3NzYpOiBGbGFreSBjcmFzaCB1bmRlciAtW1dlYkNvcmVS ZXNvdXJjZUhhbmRsZUFzT3BlcmF0aW9uUXVldWVEZWxlZ2F0ZSBjb25uZWN0aW9uOndpbGxTZW5k UmVxdWVzdDpyZWRpcmVjdFJlc3BvbnNlOl0KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL3Bs YXRmb3JtL2dhbWVwYWQvbWFuZXR0ZS9NYW5ldHRlR2FtZXBhZFByb3ZpZGVyLmNwcCBiL1NvdXJj ZS9XZWJDb3JlL3BsYXRmb3JtL2dhbWVwYWQvbWFuZXR0ZS9NYW5ldHRlR2FtZXBhZFByb3ZpZGVy LmNwcAppbmRleCA1ZGI0ZGM0M2Y0MjAxNmRkNjI4MDZjYmIxNWQ1YjA0ZTlmMTViOTc5Li4wMDgy NDkwZWNmZjg0MTgwZDc5YjkxMTM2ZjFmNmVjNTcyZmJiZmYxIDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9nYW1lcGFkL21hbmV0dGUvTWFuZXR0ZUdhbWVwYWRQcm92aWRlci5j cHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ2FtZXBhZC9tYW5ldHRlL01hbmV0dGVH YW1lcGFkUHJvdmlkZXIuY3BwCkBAIC01Nyw3ICs1Nyw3IEBAIHN0YXRpYyB2b2lkIG9uRGV2aWNl RGlzY29ubmVjdGVkKE1hbmV0dGVNb25pdG9yKiwgTWFuZXR0ZURldmljZSogZGV2aWNlLCBNYW5l dHRlCiB9CiAKIE1hbmV0dGVHYW1lcGFkUHJvdmlkZXI6Ok1hbmV0dGVHYW1lcGFkUHJvdmlkZXIo KQotICAgIDogbV9tb25pdG9yKG1hbmV0dGVfbW9uaXRvcl9uZXcoKSkKKyAgICA6IG1fbW9uaXRv cihhZG9wdEdSZWYobWFuZXR0ZV9tb25pdG9yX25ldygpKSkKICAgICAsIG1fY29ubmVjdGlvbkRl bGF5VGltZXIoUnVuTG9vcDo6Y3VycmVudCgpLCB0aGlzLCAmTWFuZXR0ZUdhbWVwYWRQcm92aWRl cjo6Y29ubmVjdGlvbkRlbGF5VGltZXJGaXJlZCkKICAgICAsIG1faW5wdXROb3RpZmljYXRpb25U aW1lcihSdW5Mb29wOjpjdXJyZW50KCksIHRoaXMsICZNYW5ldHRlR2FtZXBhZFByb3ZpZGVyOjpp bnB1dE5vdGlmaWNhdGlvblRpbWVyRmlyZWQpCiB7CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29y ZS9wbGF0Zm9ybS9nYW1lcGFkL21hbmV0dGUvTWFuZXR0ZUdhbWVwYWRQcm92aWRlci5oIGIvU291 cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ2FtZXBhZC9tYW5ldHRlL01hbmV0dGVHYW1lcGFkUHJvdmlk ZXIuaAppbmRleCAyOWUyODE5NjIxNjAzNTVmZGVhYzljODdmYjZjZjAxZmIwMTQyNDNjLi5iZjdh N2RkZWQzNmRlNTAzZGEyODE4M2RkMjFlNzNmZWNiZmFkYTY5IDEwMDY0NAotLS0gYS9Tb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9nYW1lcGFkL21hbmV0dGUvTWFuZXR0ZUdhbWVwYWRQcm92aWRlci5o CisrKyBiL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dhbWVwYWQvbWFuZXR0ZS9NYW5ldHRlR2Ft ZXBhZFByb3ZpZGVyLmgKQEAgLTY5LDcgKzY5LDcgQEAgcHJpdmF0ZToKICAgICBIYXNoTWFwPE1h bmV0dGVEZXZpY2UqLCBzdGQ6OnVuaXF1ZV9wdHI8TWFuZXR0ZUdhbWVwYWQ+PiBtX2dhbWVwYWRN YXA7CiAgICAgYm9vbCBtX3Nob3VsZERpc3BhdGNoQ2FsbGJhY2tzIHsgZmFsc2UgfTsKIAotICAg IEdVbmlxdWVQdHI8TWFuZXR0ZU1vbml0b3I+IG1fbW9uaXRvcjsKKyAgICBHUmVmUHRyPE1hbmV0 dGVNb25pdG9yPiBtX21vbml0b3I7CiAgICAgUnVuTG9vcDo6VGltZXI8TWFuZXR0ZUdhbWVwYWRQ cm92aWRlcj4gbV9jb25uZWN0aW9uRGVsYXlUaW1lcjsKICAgICBSdW5Mb29wOjpUaW1lcjxNYW5l dHRlR2FtZXBhZFByb3ZpZGVyPiBtX2lucHV0Tm90aWZpY2F0aW9uVGltZXI7CiB9Owo=