121099 2013-09-10 09:48:12 -0700 [WK2][GTK] Frequent crashes when showing context menus in Debug builds 2013-09-10 11:34:00 -0700 1 1 1 Unclassified WebKit WebKitGTK 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 svillar svillar cgarcia gustavo mrobinson svillar xan.lopez oldest_to_newest 927343 0 svillar 2013-09-10 09:48:12 -0700 It's becoming quite common to get a crash in the UIProcess when showing context menus at least with Debug builds. After debugging it a bit it looks like the problem happens inside WebContextMenuProxyGtk::contextMenuItemVisibilityChanged, here GOwnPtr<GList> items(gtk_container_get_children(GTK_CONTAINER(menu))); Basically the problem is that the menu reference is invalid. That likely means that the menu was freed and then we're trying to use it. Since this is a signal callback the problem is likely that we aren't disconnecting the signals when destroying the context menu. Patch to follow. 927349 1 211208 svillar 2013-09-10 09:52:44 -0700 Created attachment 211208 Patch 927354 2 211208 cgarcia 2013-09-10 09:58:05 -0700 Comment on attachment 211208 Patch I wonder if we could use g_signal_connect_object and we don't need to keep a map of signal handlers. 927365 3 svillar 2013-09-10 10:12:19 -0700 (In reply to comment #2) > (From update of attachment 211208 [details]) > I wonder if we could use g_signal_connect_object and we don't need to keep a map of signal handlers. You mean using the GtkMenu as the object? Yeah I guess that could work as well... 927378 4 cgarcia 2013-09-10 10:39:47 -0700 (In reply to comment #3) > (In reply to comment #2) > > (From update of attachment 211208 [details] [details]) > > I wonder if we could use g_signal_connect_object and we don't need to keep a map of signal handlers. > > You mean using the GtkMenu as the object? Yeah I guess that could work as well... I meant for the GtkAction, but doesn't make sense since it's the menu what is destroyed. 927379 5 211208 cgarcia 2013-09-10 10:42:21 -0700 Comment on attachment 211208 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=211208&action=review LGTM > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:174 > + for (HashMap<unsigned long, GtkAction*>::const_iterator iter = m_signalHandlers.begin(); iter != m_signalHandlers.end(); ++iter) > + g_signal_handler_disconnect(iter->value, iter->key); I'm not C++ expert, but now that we are using C++ 11 features I wonder if we could use auto here. 927385 6 mrobinson 2013-09-10 11:14:33 -0700 (In reply to comment #5) > (From update of attachment 211208 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=211208&action=review > > LGTM > > > Source/WebKit2/UIProcess/gtk/WebContextMenuProxyGtk.cpp:174 > > + for (HashMap<unsigned long, GtkAction*>::const_iterator iter = m_signalHandlers.begin(); iter != m_signalHandlers.end(); ++iter) > > + g_signal_handler_disconnect(iter->value, iter->key); > > I'm not C++ expert, but now that we are using C++ 11 features I wonder if we could use auto here. I think you're right. See Source/JavaScriptCore/runtime/MapData.cpp for instance. 927393 7 svillar 2013-09-10 11:34:00 -0700 Committed r155459: <http://trac.webkit.org/changeset/155459> 211208 2013-09-10 09:52:44 -0700 2013-09-10 10:42:20 -0700 Patch bug-121099-20130910185239.patch text/plain 3690 svillar U3VidmVyc2lvbiBSZXZpc2lvbjogMTU1NDI1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggMDdhY2U1OTZiMjQwYzYy MDIwMTVkYzYzZjI1ZTk1Y2VmODBhOTdmMS4uNmUwN2Y4ZGFhNThhN2E0ZjFkNzI3Y2YwNjRiYjFi Njk0NWE0YjZiNyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDEzLTA5LTEwICBTZXJn aW8gVmlsbGFyIFNlbmluICA8c3ZpbGxhckBpZ2FsaWEuY29tPgorCisgICAgICAgIFtXSzJdW0dU S10gRnJlcXVlbnQgY3Jhc2hlcyB3aGVuIHNob3dpbmcgY29udGV4dCBtZW51cyBpbiBEZWJ1ZyBi dWlsZHMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEy MTA5OQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIERp c2Nvbm5lY3Qgc2lnbmFsIGhhbmRsZXJzIHdoZW4gZGVzdHJveWluZyB0aGUgY29udGV4dCBtZW51 IHByb3h5CisgICAgICAgIG9iamVjdC4gVGhpcyBhdm9pZHMgdXNlLWFmdGVyLWZyZWUgY3Jhc2hl cyB3aGVuIG9wZW5pbmcgc2V2ZXJhbAorICAgICAgICBjb250ZXh0IG1lbnVzIGluIGEgcm93Lgor CisgICAgICAgICogVUlQcm9jZXNzL2d0ay9XZWJDb250ZXh0TWVudVByb3h5R3RrLmNwcDoKKyAg ICAgICAgKFdlYktpdDo6V2ViQ29udGV4dE1lbnVQcm94eUd0azo6YXBwZW5kKToKKyAgICAgICAg KFdlYktpdDo6V2ViQ29udGV4dE1lbnVQcm94eUd0azo6fldlYkNvbnRleHRNZW51UHJveHlHdGsp OgorICAgICAgICAqIFVJUHJvY2Vzcy9ndGsvV2ViQ29udGV4dE1lbnVQcm94eUd0ay5oOgorCiAy MDEzLTA5LTA5ICBEZWFuIEphY2tzb24gIDxkaW5vQGFwcGxlLmNvbT4KIAogICAgICAgICBbV2Vi R0xdIEFsbG93IG11bHRpdGhyZWFkZWQgT3BlbkdMIGNvbnRleHRzCmRpZmYgLS1naXQgYS9Tb3Vy Y2UvV2ViS2l0Mi9VSVByb2Nlc3MvZ3RrL1dlYkNvbnRleHRNZW51UHJveHlHdGsuY3BwIGIvU291 cmNlL1dlYktpdDIvVUlQcm9jZXNzL2d0ay9XZWJDb250ZXh0TWVudVByb3h5R3RrLmNwcAppbmRl eCBkN2QwNjgzNDAzNmQ0NTBkYTg0NzBhNWQ0ODUyYzI4NmZiMGY4NTlkLi5hMmVjMzU3MzBkMmZi YTNkMTEzZTBlYTBmYmRlMmU5MWM5NWU3MWRhIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9V SVByb2Nlc3MvZ3RrL1dlYkNvbnRleHRNZW51UHJveHlHdGsuY3BwCisrKyBiL1NvdXJjZS9XZWJL aXQyL1VJUHJvY2Vzcy9ndGsvV2ViQ29udGV4dE1lbnVQcm94eUd0ay5jcHAKQEAgLTg0LDE2ICs4 NCwxOSBAQCBzdGF0aWMgdm9pZCBjb250ZXh0TWVudUl0ZW1WaXNpYmlsaXR5Q2hhbmdlZChHdGtB Y3Rpb24qIGFjdGlvbiwgR1BhcmFtU3BlYyosIFdlYgogCiB2b2lkIFdlYkNvbnRleHRNZW51UHJv eHlHdGs6OmFwcGVuZChDb250ZXh0TWVudUl0ZW0mIG1lbnVJdGVtKQogeworICAgIHVuc2lnbmVk IGxvbmcgc2lnbmFsSGFuZGxlcklkOwogICAgIEd0a0FjdGlvbiogYWN0aW9uID0gbWVudUl0ZW0u Z3RrQWN0aW9uKCk7CiAgICAgaWYgKGFjdGlvbikgewogICAgICAgICBzd2l0Y2ggKG1lbnVJdGVt LnR5cGUoKSkgewogICAgICAgICBjYXNlIEFjdGlvblR5cGU6CiAgICAgICAgIGNhc2UgQ2hlY2th YmxlQWN0aW9uVHlwZToKICAgICAgICAgICAgIGdfb2JqZWN0X3NldF9kYXRhKEdfT0JKRUNUKGFj dGlvbiksIGdDb250ZXh0TWVudUFjdGlvbklkLCBHSU5UX1RPX1BPSU5URVIobWVudUl0ZW0uYWN0 aW9uKCkpKTsKLSAgICAgICAgICAgIGdfc2lnbmFsX2Nvbm5lY3QoYWN0aW9uLCAiYWN0aXZhdGUi LCBHX0NBTExCQUNLKGNvbnRleHRNZW51SXRlbUFjdGl2YXRlZENhbGxiYWNrKSwgbV9wYWdlKTsK KyAgICAgICAgICAgIHNpZ25hbEhhbmRsZXJJZCA9IGdfc2lnbmFsX2Nvbm5lY3QoYWN0aW9uLCAi YWN0aXZhdGUiLCBHX0NBTExCQUNLKGNvbnRleHRNZW51SXRlbUFjdGl2YXRlZENhbGxiYWNrKSwg bV9wYWdlKTsKKyAgICAgICAgICAgIG1fc2lnbmFsSGFuZGxlcnMuc2V0KHNpZ25hbEhhbmRsZXJJ ZCwgYWN0aW9uKTsKICAgICAgICAgICAgIC8vIEZhbGwgdGhyb3VnaC4KICAgICAgICAgY2FzZSBT dWJtZW51VHlwZToKLSAgICAgICAgICAgIGdfc2lnbmFsX2Nvbm5lY3QoYWN0aW9uLCAibm90aWZ5 Ojp2aXNpYmxlIiwgR19DQUxMQkFDSyhjb250ZXh0TWVudUl0ZW1WaXNpYmlsaXR5Q2hhbmdlZCks IHRoaXMpOworICAgICAgICAgICAgc2lnbmFsSGFuZGxlcklkID0gZ19zaWduYWxfY29ubmVjdChh Y3Rpb24sICJub3RpZnk6OnZpc2libGUiLCBHX0NBTExCQUNLKGNvbnRleHRNZW51SXRlbVZpc2li aWxpdHlDaGFuZ2VkKSwgdGhpcyk7CisgICAgICAgICAgICBtX3NpZ25hbEhhbmRsZXJzLnNldChz aWduYWxIYW5kbGVySWQsIGFjdGlvbik7CiAgICAgICAgICAgICBicmVhazsKICAgICAgICAgY2Fz ZSBTZXBhcmF0b3JUeXBlOgogICAgICAgICAgICAgYnJlYWs7CkBAIC0xNjcsNiArMTcwLDkgQEAg V2ViQ29udGV4dE1lbnVQcm94eUd0azo6V2ViQ29udGV4dE1lbnVQcm94eUd0ayhHdGtXaWRnZXQq IHdlYlZpZXcsIFdlYlBhZ2VQcm94eSoKIAogV2ViQ29udGV4dE1lbnVQcm94eUd0azo6fldlYkNv bnRleHRNZW51UHJveHlHdGsoKQogeworICAgIGZvciAoSGFzaE1hcDx1bnNpZ25lZCBsb25nLCBH dGtBY3Rpb24qPjo6Y29uc3RfaXRlcmF0b3IgaXRlciA9IG1fc2lnbmFsSGFuZGxlcnMuYmVnaW4o KTsgaXRlciAhPSBtX3NpZ25hbEhhbmRsZXJzLmVuZCgpOyArK2l0ZXIpCisgICAgICAgIGdfc2ln bmFsX2hhbmRsZXJfZGlzY29ubmVjdChpdGVyLT52YWx1ZSwgaXRlci0+a2V5KTsKKwogICAgIHdl YmtpdFdlYlZpZXdCYXNlU2V0QWN0aXZlQ29udGV4dE1lbnVQcm94eShXRUJLSVRfV0VCX1ZJRVdf QkFTRShtX3dlYlZpZXcpLCAwKTsKIH0KIApkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdDIvVUlQ cm9jZXNzL2d0ay9XZWJDb250ZXh0TWVudVByb3h5R3RrLmggYi9Tb3VyY2UvV2ViS2l0Mi9VSVBy b2Nlc3MvZ3RrL1dlYkNvbnRleHRNZW51UHJveHlHdGsuaAppbmRleCAxNDQ2NTVmNGQ1ZGUwZjVi ZjkxOTljZmY1YjgyYWFjOWUxZjExMWViLi5kZWQ0YjdhODQ4MjNmYThhMDkyNDI0MGU2ZTY2YTkz YjZlZThmODI2IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvZ3RrL1dlYkNv bnRleHRNZW51UHJveHlHdGsuaAorKysgYi9Tb3VyY2UvV2ViS2l0Mi9VSVByb2Nlc3MvZ3RrL1dl YkNvbnRleHRNZW51UHJveHlHdGsuaApAQCAtMzEsNiArMzEsNyBAQAogI2luY2x1ZGUgIldlYkNv bnRleHRNZW51UHJveHkuaCIKICNpbmNsdWRlIDxXZWJDb3JlL0NvbnRleHRNZW51Lmg+CiAjaW5j bHVkZSA8V2ViQ29yZS9JbnRQb2ludC5oPgorI2luY2x1ZGUgPHd0Zi9IYXNoTWFwLmg+CiAKIG5h bWVzcGFjZSBXZWJLaXQgewogCkBAIC02Miw2ICs2Myw3IEBAIHByaXZhdGU6CiAgICAgV2ViUGFn ZVByb3h5KiBtX3BhZ2U7CiAgICAgV2ViQ29yZTo6Q29udGV4dE1lbnUgbV9tZW51OwogICAgIFdl YkNvcmU6OkludFBvaW50IG1fcG9wdXBQb3NpdGlvbjsKKyAgICBIYXNoTWFwPHVuc2lnbmVkIGxv bmcsIEd0a0FjdGlvbio+IG1fc2lnbmFsSGFuZGxlcnM7CiB9OwogCiAK