173417 2017-06-15 05:48:25 -0700 [GTK] Stop using GtkAction in WebPopupMenuProxyGtk 2017-06-15 07:03:08 -0700 1 1 1 Unclassified WebKit WebKitGTK Other Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 aperez aperez achristensen bburg bugs-noreply cgarcia commit-queue ggaren mcatanzaro oldest_to_newest 1319549 0 aperez 2017-06-15 05:48:25 -0700 There are two reasons to stop using it: - GtkAction has been deprecated in GTK+ 3.10, and most likely it will not be available in GTK+ 4.x; GAction is recommended instead. - The additional functionality from GtkAction is not used. It should be possible to directly construct the needed GtkMenuItems without creating a GtkAction first. 1319550 1 312974 aperez 2017-06-15 05:54:16 -0700 Created attachment 312974 Patch 1319551 2 312974 mcatanzaro 2017-06-15 06:00:57 -0700 Comment on attachment 312974 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312974&action=review Nice! > Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:68 > gtk_menu_shell_append(GTK_MENU_SHELL(m_popup), menuItem); Is this really transfer full? It's not documented that way at all. I suspect it's a bindings error, but otherwise it looks like the GtkMenuItem has always been leaked here. Can you check it? 1319552 3 312974 cgarcia 2017-06-15 06:05:40 -0700 Comment on attachment 312974 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=312974&action=review >> Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:68 >> gtk_menu_shell_append(GTK_MENU_SHELL(m_popup), menuItem); > > Is this really transfer full? It's not documented that way at all. I suspect it's a bindings error, but otherwise it looks like the GtkMenuItem has always been leaked here. Can you check it? Widgets are initially unowned, and adding them to a container always transfers the ownership to the container, the floating ref is consumed by the container. 1319555 4 aperez 2017-06-15 06:28:50 -0700 (In reply to Carlos Garcia Campos from comment #3) > Comment on attachment 312974 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=312974&action=review > > >> Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:68 > >> gtk_menu_shell_append(GTK_MENU_SHELL(m_popup), menuItem); > > > > Is this really transfer full? It's not documented that way at all. I suspect it's a bindings error, but otherwise it looks like the GtkMenuItem has always been leaked here. Can you check it? > > Widgets are initially unowned, and adding them to a container always > transfers the ownership to the container, the floating ref is consumed by > the container. Yes, this is what happens here as well. Just for the sake of completeness, I checked the source code and the chain of function calls inside GTK+, and indeed g_object_ref_sink ends up being called as expected: gtk_menu_shell_append [gtkmenushell.c] gtk_menu_shell_insert gtk_menu_shell_real_insert gtk_widget_set_parent [gtkwidget.c] gtk_widget_insert_after gtk_widget_reposition_after g_object_ref_sink :-) 1319565 5 312974 commit-queue 2017-06-15 07:03:06 -0700 Comment on attachment 312974 Patch Clearing flags on attachment: 312974 Committed r218329: <http://trac.webkit.org/changeset/218329> 1319566 6 commit-queue 2017-06-15 07:03:08 -0700 All reviewed patches have been landed. Closing bug. 312974 2017-06-15 05:54:16 -0700 2017-06-15 07:03:06 -0700 Patch bug-173417-20170615155415.patch text/plain 5588 aperez U3VidmVyc2lvbiBSZXZpc2lvbjogMjE4MzI3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggMzE2YWIyZjE0NDcxNzQ3 MjA5YzNjOWU3ZjY0MjhiY2E3NTBkOGIxNi4uNmY5MWZhMjlhYWUwNDZhNzQ3N2RjZmY4N2Y0OTM0 OGRiNWQ0OTA5ZSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIyIEBACisyMDE3LTA2LTE1ICBBZHJp YW4gUGVyZXogZGUgQ2FzdHJvICA8YXBlcmV6QGlnYWxpYS5jb20+CisKKyAgICAgICAgW0dUS10g U3RvcCB1c2luZyBHdGtBY3Rpb24gaW4gV2ViUG9wdXBNZW51UHJveHlHdGsKKyAgICAgICAgaHR0 cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE3MzQxNworCisgICAgICAgIFJl dmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoaXMgZ2V0cyByaWQgb2YgR3Rr QWN0aW9uIGluIFdlYlBvcHVwTWVudVByb3h5R3RrLCB3aGljaCB3YXMgZGVwcmVjYXRlZCBpbiBH VEsrIDMuMTAKKyAgICAgICAgYW5kIHdpbGwgbm90IGJlIGF2YWlsYWJsZSBpbiBHVEsrIDQuMC4g VXNpbmcgR3RrTWVudUl0ZW0gZGlyZWN0bHkgbWFrZXMgcG9wdWxhdGluZworICAgICAgICBsb25n IG1lbnVzIHNsaWdodGx5IGZhc3Rlciwgd2hpY2ggY2FuIGJlIG5vdGljZWFibGUgaW4gbG93ZXIt ZW5kIG1hY2hpbmVzLgorCisgICAgICAgICogVUlQcm9jZXNzL2d0ay9XZWJQb3B1cE1lbnVQcm94 eUd0ay5jcHA6CisgICAgICAgIChXZWJLaXQ6OldlYlBvcHVwTWVudVByb3h5R3RrOjpwb3B1bGF0 ZVBvcHVwTWVudSk6CisgICAgICAgIChXZWJLaXQ6OldlYlBvcHVwTWVudVByb3h5R3RrOjptZW51 SXRlbUFjdGl2YXRlZCk6CisgICAgICAgIChXZWJLaXQ6OldlYlBvcHVwTWVudVByb3h5R3RrOjpj cmVhdGVHdGtBY3Rpb25Gb3JNZW51SXRlbSk6IERlbGV0ZWQuCisgICAgICAgICogVUlQcm9jZXNz L2d0ay9XZWJQb3B1cE1lbnVQcm94eUd0ay5oOgorICAgICAgICAoV2ViS2l0OjpXZWJQb3B1cE1l bnVQcm94eUd0azo6c2V0Q3VycmVudGx5U2VsZWN0ZWRNZW51SXRlbSk6CisgICAgICAgIEluc3Rh bnRpYXRlIEd0a01lbnVJdGVtIG9iamVjdHMgZGlyZWN0bHkgd2hlbiBwb3B1bGF0aW5nIHRoZSBt ZW51LgorCiAyMDE3LTA2LTE1ICBDYXJsb3MgR2FyY2lhIENhbXBvcyAgPGNnYXJjaWFAaWdhbGlh LmNvbT4KIAogICAgICAgICBVbnJldmlld2VkLiBGaXggY29weS1wYXN0ZSBlcnJvciBpbiBHVEsr IFdFQktJVF9KQVZBU0NSSVBUX0VSUk9SIGRlZmluaXRpb24uCmRpZmYgLS1naXQgYS9Tb3VyY2Uv V2ViS2l0Mi9VSVByb2Nlc3MvZ3RrL1dlYlBvcHVwTWVudVByb3h5R3RrLmNwcCBiL1NvdXJjZS9X ZWJLaXQyL1VJUHJvY2Vzcy9ndGsvV2ViUG9wdXBNZW51UHJveHlHdGsuY3BwCmluZGV4IGVjNzcz MjU3MjBlNTZhYzBkZjY4ZjAwOWVkYjEyMjllN2NjM2Q0NGMuLmY0OGM4ZGVkNTIwODQwMzJmYjg1 NDY0NmQ5NjY1Mjk5YjAwYTIwNDQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vz cy9ndGsvV2ViUG9wdXBNZW51UHJveHlHdGsuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQyL1VJUHJv Y2Vzcy9ndGsvV2ViUG9wdXBNZW51UHJveHlHdGsuY3BwCkBAIC01MCwxNyArNTAsNiBAQCBXZWJQ b3B1cE1lbnVQcm94eUd0azo6fldlYlBvcHVwTWVudVByb3h5R3RrKCkKICAgICBjYW5jZWxUcmFj a2luZygpOwogfQogCi1HdGtBY3Rpb24qIFdlYlBvcHVwTWVudVByb3h5R3RrOjpjcmVhdGVHdGtB Y3Rpb25Gb3JNZW51SXRlbShjb25zdCBXZWJQb3B1cEl0ZW0mIGl0ZW0sIGludCBpdGVtSW5kZXgp Ci17Ci0gICAgR1VuaXF1ZVB0cjxjaGFyPiBhY3Rpb25OYW1lKGdfc3RyZHVwX3ByaW50ZigicG9w dXAtbWVudS1hY3Rpb24tJWQiLCBpdGVtSW5kZXgpKTsKLSAgICBHdGtBY3Rpb24qIGFjdGlvbiA9 IGd0a19hY3Rpb25fbmV3KGFjdGlvbk5hbWUuZ2V0KCksIGl0ZW0ubV90ZXh0LnV0ZjgoKS5kYXRh KCksIGl0ZW0ubV90b29sVGlwLnV0ZjgoKS5kYXRhKCksIDApOwotICAgIGdfb2JqZWN0X3NldF9k YXRhKEdfT0JKRUNUKGFjdGlvbiksICJwb3B1cC1tZW51LWFjdGlvbi1pbmRleCIsIEdJTlRfVE9f UE9JTlRFUihpdGVtSW5kZXgpKTsKLSAgICBnX3NpZ25hbF9jb25uZWN0KGFjdGlvbiwgImFjdGl2 YXRlIiwgR19DQUxMQkFDSyhtZW51SXRlbUFjdGl2YXRlZCksIHRoaXMpOwotICAgIGd0a19hY3Rp b25fc2V0X3NlbnNpdGl2ZShhY3Rpb24sIGl0ZW0ubV9pc0VuYWJsZWQpOwotCi0gICAgcmV0dXJu IGFjdGlvbjsKLX0KLQogdm9pZCBXZWJQb3B1cE1lbnVQcm94eUd0azo6cG9wdWxhdGVQb3B1cE1l bnUoY29uc3QgVmVjdG9yPFdlYlBvcHVwSXRlbT4mIGl0ZW1zKQogewogICAgIGludCBpdGVtSW5k ZXggPSAwOwpAQCAtNzAsMTQgKzU5LDE0IEBAIHZvaWQgV2ViUG9wdXBNZW51UHJveHlHdGs6OnBv cHVsYXRlUG9wdXBNZW51KGNvbnN0IFZlY3RvcjxXZWJQb3B1cEl0ZW0+JiBpdGVtcykKICAgICAg ICAgICAgIGd0a19tZW51X3NoZWxsX2FwcGVuZChHVEtfTUVOVV9TSEVMTChtX3BvcHVwKSwgbWVu dUl0ZW0pOwogICAgICAgICAgICAgZ3RrX3dpZGdldF9zaG93KG1lbnVJdGVtKTsKICAgICAgICAg fSBlbHNlIHsKLSAgICAgICAgICAgIEdSZWZQdHI8R3RrQWN0aW9uPiBhY3Rpb24gPSBhZG9wdEdS ZWYoY3JlYXRlR3RrQWN0aW9uRm9yTWVudUl0ZW0oaXRlbSwgaXRlbUluZGV4KSk7Ci0gICAgICAg ICAgICBHdGtXaWRnZXQqIG1lbnVJdGVtID0gZ3RrX2FjdGlvbl9jcmVhdGVfbWVudV9pdGVtKGFj dGlvbi5nZXQoKSk7Ci0gICAgICAgICAgICBndGtfd2lkZ2V0X3NldF90b29sdGlwX3RleHQobWVu dUl0ZW0sIGd0a19hY3Rpb25fZ2V0X3Rvb2x0aXAoYWN0aW9uLmdldCgpKSk7CisgICAgICAgICAg ICBHdGtXaWRnZXQqIG1lbnVJdGVtID0gZ3RrX21lbnVfaXRlbV9uZXdfd2l0aF9sYWJlbChpdGVt Lm1fdGV4dC51dGY4KCkuZGF0YSgpKTsKKyAgICAgICAgICAgIGd0a193aWRnZXRfc2V0X3Rvb2x0 aXBfdGV4dChtZW51SXRlbSwgaXRlbS5tX3Rvb2xUaXAudXRmOCgpLmRhdGEoKSk7CisgICAgICAg ICAgICBndGtfd2lkZ2V0X3NldF9zZW5zaXRpdmUobWVudUl0ZW0sIGl0ZW0ubV9pc0VuYWJsZWQp OworICAgICAgICAgICAgZ19vYmplY3Rfc2V0X2RhdGEoR19PQkpFQ1QobWVudUl0ZW0pLCAicG9w dXAtbWVudS1pdGVtLWluZGV4IiwgR0lOVF9UT19QT0lOVEVSKGl0ZW1JbmRleCkpOworICAgICAg ICAgICAgZ19zaWduYWxfY29ubmVjdChtZW51SXRlbSwgImFjdGl2YXRlIiwgR19DQUxMQkFDSyht ZW51SXRlbUFjdGl2YXRlZCksIHRoaXMpOwogICAgICAgICAgICAgZ19zaWduYWxfY29ubmVjdCht ZW51SXRlbSwgInNlbGVjdCIsIEdfQ0FMTEJBQ0soc2VsZWN0SXRlbUNhbGxiYWNrKSwgdGhpcyk7 CiAgICAgICAgICAgICBndGtfbWVudV9zaGVsbF9hcHBlbmQoR1RLX01FTlVfU0hFTEwobV9wb3B1 cCksIG1lbnVJdGVtKTsKLQotICAgICAgICAgICAgaWYgKGd0a19hY3Rpb25faXNfdmlzaWJsZShh Y3Rpb24uZ2V0KCkpKQotICAgICAgICAgICAgICAgIGd0a193aWRnZXRfc2hvdyhtZW51SXRlbSk7 CisgICAgICAgICAgICBndGtfd2lkZ2V0X3Nob3cobWVudUl0ZW0pOwogICAgICAgICB9CiAgICAg ICAgIGl0ZW1JbmRleCsrOwogICAgIH0KQEAgLTI3NSwxMSArMjY0LDExIEBAIHZvaWQgV2ViUG9w dXBNZW51UHJveHlHdGs6OnJlc2V0VHlwZUFoZWFkRmluZFN0YXRlKCkKICAgICBtX2N1cnJlbnRT ZWFyY2hTdHJpbmcgPSBlbXB0eVN0cmluZygpOwogfQogCi12b2lkIFdlYlBvcHVwTWVudVByb3h5 R3RrOjptZW51SXRlbUFjdGl2YXRlZChHdGtBY3Rpb24qIGFjdGlvbiwgV2ViUG9wdXBNZW51UHJv eHlHdGsqIHBvcHVwTWVudSkKK3ZvaWQgV2ViUG9wdXBNZW51UHJveHlHdGs6Om1lbnVJdGVtQWN0 aXZhdGVkKEd0a01lbnVJdGVtKiBtZW51SXRlbSwgV2ViUG9wdXBNZW51UHJveHlHdGsqIHBvcHVw TWVudSkKIHsKICAgICBwb3B1cE1lbnUtPm1fZGlzbWlzc01lbnVUaW1lci5zdG9wKCk7CiAgICAg aWYgKHBvcHVwTWVudS0+bV9jbGllbnQpCi0gICAgICAgIHBvcHVwTWVudS0+bV9jbGllbnQtPnZh bHVlQ2hhbmdlZEZvclBvcHVwTWVudShwb3B1cE1lbnUsIEdQT0lOVEVSX1RPX0lOVChnX29iamVj dF9nZXRfZGF0YShHX09CSkVDVChhY3Rpb24pLCAicG9wdXAtbWVudS1hY3Rpb24taW5kZXgiKSkp OworICAgICAgICBwb3B1cE1lbnUtPm1fY2xpZW50LT52YWx1ZUNoYW5nZWRGb3JQb3B1cE1lbnUo cG9wdXBNZW51LCBHUE9JTlRFUl9UT19JTlQoZ19vYmplY3RfZ2V0X2RhdGEoR19PQkpFQ1QobWVu dUl0ZW0pLCAicG9wdXAtbWVudS1pdGVtLWluZGV4IikpKTsKIH0KIAogdm9pZCBXZWJQb3B1cE1l bnVQcm94eUd0azo6ZGlzbWlzc01lbnVUaW1lckZpcmVkKCkKZGlmZiAtLWdpdCBhL1NvdXJjZS9X ZWJLaXQyL1VJUHJvY2Vzcy9ndGsvV2ViUG9wdXBNZW51UHJveHlHdGsuaCBiL1NvdXJjZS9XZWJL aXQyL1VJUHJvY2Vzcy9ndGsvV2ViUG9wdXBNZW51UHJveHlHdGsuaAppbmRleCBlZGQ4ZThmM2M1 Y2VmNzUwNTk2YWM1NzgwMWZmODFiMzQ4ZmM1OTVhLi4yNTJjMzE4NzQ1MTQ4ZTRhZjRmMmFhM2Mx MTQzMTU1ZTdhMjIxMzczIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvZ3Rr L1dlYlBvcHVwTWVudVByb3h5R3RrLmgKKysrIGIvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL2d0 ay9XZWJQb3B1cE1lbnVQcm94eUd0ay5oCkBAIC01NSwxNCArNTUsMTMgQEAgcHJvdGVjdGVkOgog CiBwcml2YXRlOgogICAgIHZvaWQgc2V0Q3VycmVudGx5U2VsZWN0ZWRNZW51SXRlbShHdGtXaWRn ZXQqIGl0ZW0pIHsgbV9jdXJyZW50bHlTZWxlY3RlZE1lbnVJdGVtID0gaXRlbTsgfQotICAgIEd0 a0FjdGlvbiogY3JlYXRlR3RrQWN0aW9uRm9yTWVudUl0ZW0oY29uc3QgV2ViUG9wdXBJdGVtJiwg aW50IGl0ZW1JbmRleCk7CiAgICAgdm9pZCBwb3B1bGF0ZVBvcHVwTWVudShjb25zdCBWZWN0b3I8 V2ViUG9wdXBJdGVtPiYpOwogICAgIHZvaWQgZGlzbWlzc01lbnVUaW1lckZpcmVkKCk7CiAKICAg ICBib29sIHR5cGVBaGVhZEZpbmQoR2RrRXZlbnRLZXkqKTsKICAgICB2b2lkIHJlc2V0VHlwZUFo ZWFkRmluZFN0YXRlKCk7CiAKLSAgICBzdGF0aWMgdm9pZCBtZW51SXRlbUFjdGl2YXRlZChHdGtB Y3Rpb24qLCBXZWJQb3B1cE1lbnVQcm94eUd0ayopOworICAgIHN0YXRpYyB2b2lkIG1lbnVJdGVt QWN0aXZhdGVkKEd0a01lbnVJdGVtKiwgV2ViUG9wdXBNZW51UHJveHlHdGsqKTsKICAgICBzdGF0 aWMgdm9pZCBzZWxlY3RJdGVtQ2FsbGJhY2soR3RrV2lkZ2V0KiwgV2ViUG9wdXBNZW51UHJveHlH dGsqKTsKICAgICBzdGF0aWMgZ2Jvb2xlYW4ga2V5UHJlc3NFdmVudENhbGxiYWNrKEd0a1dpZGdl dCosIEdka0V2ZW50S2V5KiwgV2ViUG9wdXBNZW51UHJveHlHdGsqKTsKICAgICBzdGF0aWMgdm9p ZCBtZW51VW5tYXBwZWRDYWxsYmFjayhHdGtXaWRnZXQqLCBXZWJQb3B1cE1lbnVQcm94eUd0ayop Owo=