Bug 88800

Summary: [GTK] Showing the context menu in the Web Inspector can crash the browser
Product: WebKit Reporter: arno. <a.renevier>
Component: WebKitGTKAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, mrobinson, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch proposal
none
updated patch to fix ChangeLog style
none
updated patch
none
updated patch none

Description arno. 2012-06-11 13:34:45 PDT
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.
Comment 1 arno. 2012-06-11 13:40:40 PDT
Created attachment 146896 [details]
patch proposal
Comment 2 Martin Robinson 2012-06-26 17:15:37 PDT
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.
Comment 3 arno. 2012-06-26 17:32:01 PDT
Created attachment 149644 [details]
updated patch to fix ChangeLog style
Comment 4 Martin Robinson 2012-06-26 17:33:17 PDT
CCing cgarcia who has worked on context menus more recently than I.
Comment 5 Carlos Garcia Campos 2012-07-04 02:11:55 PDT
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.
Comment 6 arno. 2012-07-05 10:05:37 PDT
Created attachment 150950 [details]
updated patch
Comment 7 WebKit Review Bot 2012-07-05 10:07:31 PDT
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 8 Carlos Garcia Campos 2012-07-05 10:40:53 PDT
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
Comment 9 arno. 2012-07-05 13:55:58 PDT
Created attachment 150980 [details]
updated patch
Comment 10 Carlos Garcia Campos 2012-07-05 23:17:15 PDT
Comment on attachment 150980 [details]
updated patch

Thanks!
Comment 11 WebKit Review Bot 2012-07-05 23:27:46 PDT
Comment on attachment 150980 [details]
updated patch

Clearing flags on attachment: 150980

Committed r121945: <http://trac.webkit.org/changeset/121945>
Comment 12 WebKit Review Bot 2012-07-05 23:27:51 PDT
All reviewed patches have been landed.  Closing bug.