Hi, when trying to display context menu in web inspector, it does not display (at best) or it crashes (at worst). I could always reproduce it when lauching web inspector from midori (linked with latest webkit trunk), and right clicking on an item in the network panel. I investigated and discovered that GtkMenuItems where created as childs of GtkMenu* menu in JSInspectorFrontendHost::showContextMenu. Then, they are stored in a vector, and appended to a new GtkMenu item in ContextMenuController::showContextMenu Then, at the end of JSInspectorFrontendHost::showContextMenu menu destructor is called, and the menu widget is destroyed (along with all of its children). So, in contextMenuItemActivated (webkitwebview.cpp), the GtkMenuItem has been destroyed, hence the crash.
Created attachment 146896 [details] patch proposal
Comment on attachment 146896 [details] patch proposal View in context: https://bugs.webkit.org/attachment.cgi?id=146896&action=review > Source/WebCore/ChangeLog:5 > + [GTK] Remove ContextMenuItem from its parent before appending it > + again to a new parent > + https://bugs.webkit.org/show_bug.cgi?id=88800 Minor nit here: You should use prepare-ChangeLogs to generate your changelogs, so that the bug title is above the bug number.
Created attachment 149644 [details] updated patch to fix ChangeLog style
CCing cgarcia who has worked on context menus more recently than I.
Comment on attachment 149644 [details] updated patch to fix ChangeLog style View in context: https://bugs.webkit.org/attachment.cgi?id=149644&action=review Thanks for the patch and sorry for the delay to review it. This is indeed a problem of using GtkMenu directly in ContextMenu. > Source/WebCore/platform/gtk/ContextMenuGtk.cpp:56 > + if (parent && parent != GTK_WIDGET(m_platformDescription)) { Adding a widget twice to the same parent is an error too, so you only need to check whether it has parent. > Source/WebCore/platform/gtk/ContextMenuGtk.cpp:62 > + gtk_menu_shell_append(GTK_MENU_SHELL(m_platformDescription), platformItem); > + gtk_widget_show(platformItem); You are leaking the platformItem in case of being reparented. You can fix the leak and simplify the code a bit using GRefPtr, someting like this: GRefPtr<GtkWidget> platformItem = GTK_WIDGET(item.releasePlatformDescription()); ASSERT(platformItem); if (GtkWidget* parent = gtk_widget_get_parent(platformItem.get())) gtk_container_remove(GTK_CONTAINER(parent), platformItem.get()); gtk_menu_shell_append(GTK_MENU_SHELL(m_platformDescription), platformItem.get()); gtk_widget_show(platformItem.get()); If the menu item has a floating reference (it hasn't been added to a container), the RefPtr consumes the floating reference and the reference counter will be 1. Then it's added to the parent, reference counter is increased, and when the function finishes the RefPtr releases its reference and the GtkMenu holds the only reference.
Created attachment 150950 [details] updated patch
Attachment 150950 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/gtk/ContextMenuGtk.cpp:27: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 150950 [details] updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=150950&action=review > Source/WebCore/platform/gtk/ContextMenuGtk.cpp:28 > #include <gtk/gtk.h> This style issue is not actually introduced by your patch, but we can simply fix it now that we are changing these lines
Created attachment 150980 [details] updated patch
Comment on attachment 150980 [details] updated patch Thanks!
Comment on attachment 150980 [details] updated patch Clearing flags on attachment: 150980 Committed r121945: <http://trac.webkit.org/changeset/121945>
All reviewed patches have been landed. Closing bug.