Bug 121099 – [WK2][GTK] Frequent crashes when showing context menus in Debug builds

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Sergio Villar Senin]

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.

[Sergio Villar Senin]

Created attachment 211208 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 211208 [details]
Patch

I wonder if we could use g_signal_connect_object and we don't need to keep a map of signal handlers.

[Sergio Villar Senin]

(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...

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

Comment on attachment 211208 [details]
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.

[Martin Robinson]

(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.

[Sergio Villar Senin]

Committed r155459: <http://trac.webkit.org/changeset/155459>