Bug 88800 – [GTK] Showing the context menu in the Web Inspector can crash the browser

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[arno.]

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.

[arno.]

Created attachment 146896 [details]
patch proposal

[Martin Robinson]

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.

[arno.]

Created attachment 149644 [details]
updated patch to fix ChangeLog style

[Martin Robinson]

CCing cgarcia who has worked on context menus more recently than I.

[Carlos Garcia Campos]

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.

[arno.]

Created attachment 150950 [details]
updated patch

[WebKit Review Bot]

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.

[Carlos Garcia Campos]

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

[arno.]

Created attachment 150980 [details]
updated patch

[Carlos Garcia Campos]

Comment on attachment 150980 [details]
updated patch

Thanks!

[WebKit Review Bot]

Comment on attachment 150980 [details]
updated patch

Clearing flags on attachment: 150980

Committed r121945: <http://trac.webkit.org/changeset/121945>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.