[webkit-reviews] review denied: [Bug 88800] [GTK] Showing the context menu in the Web Inspector can crash the browser : [Attachment 149644] updated patch to fix ChangeLog style

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied arno.
<arno at renevier.net>'s request for review:
Bug 88800: [GTK] Showing the context menu in the Web Inspector can crash the
browser
https://bugs.webkit.org/show_bug.cgi?id=88800

Attachment 149644: updated patch to fix ChangeLog style
https://bugs.webkit.org/attachment.cgi?id=149644&action=review

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
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.