Bug 56466 – Popup menu can get stuck in closed state when GtkMenu can't grab mouse.

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin H.]

Created attachment 85934 [details]
patch to fix the bug

gtk_menu_popup show can silent fail to actually open a menu when it can't acquire a mouse pointer grab.
As WebCore and gtk track the opened/visible status separately this can lead to a situation where WebCore believes the menu is open and then tries to close it but gtk never signals back that it closed the menu (because it was not visible to start from gtk's point of view). This way the user can not open for example drop down menus on a website at all.

PopupMenuGtk needs to check if the gtk_menu_popup call actually succeeded and if it didn't call back to WebCore to signal that the menu is closed again.

I experienced this bug with a compositing manager, i didn't manage to reproduce it without, but the problem is likely also triggerable without anything special.


I've make a patch to fix this by calling gtk_widget_get_visible and if it returns false call client()->popupDidHide();

The patch is against an older version but trunk seems to have exactly the same code.


--- webkit-1.2.5.orig/WebCore/platform/gtk/PopupMenuGtk.cpp     2011-03-15 14:54:30.807239931 +0100
+++ webkit-1.2.5/WebCore/platform/gtk/PopupMenuGtk.cpp  2011-03-15 14:54:39.415987337 +0100
@@ -108,6 +108,11 @@
 
     g_list_free(children);
     gtk_menu_popup(m_popup.get(), 0, 0, reinterpret_cast<GtkMenuPositionFunc>(menuPositionFunction), this, 0, gtk_get_current_event_time());
+    if (!gtk_widget_get_visible(GTK_WIDGET(m_popup.get()))) {
+        // gtk can refuse to actually open the menu when grabs fail. Make sure WebCore's state keeps in sync.
+        ASSERT(that->client());
+        client()->popupDidHide();
+    }
 }
 
 void PopupMenu::hide()

[Martin Robinson]

Thanks for your contribution! Do you mind attaching a ChangeLog and making this patch against the version of WebKit in the repository? Please see here for more information: http://www.webkit.org/coding/contributing.html

[Wajahat Siddiqui]

Created attachment 109276 [details]
proposed patch to fix this issue in recent revision

I think this is a trivial issue, attaching patch with fix made in recent revision

[Martin Robinson]

Comment on attachment 109276 [details]
proposed patch to fix this issue in recent revision

View in context: https://bugs.webkit.org/attachment.cgi?id=109276&action=review

> Source/WebCore/platform/gtk/PopupMenuGtk.cpp:95
> +    // GTK can refuse to actually open the menu when mouse grabs fails.
> +    // Ensure WebCore does not go into some pesky state.
> +    if (!gtk_widget_get_visible(m_popup->platformMenu()))
> +        client()->popupDidHide();

You shold also include this fix in Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp.

[Martin Robinson]

Comment on attachment 109276 [details]
proposed patch to fix this issue in recent revision

View in context: https://bugs.webkit.org/attachment.cgi?id=109276&action=review

> Source/WebCore/platform/gtk/PopupMenuGtk.cpp:9
> + * Portions Copyright (c) 2010 Motorola Mobility, Inc.  All rights reserved.

Typically the barrier is ~10 lines of code for inserting your copyright line.

[Wajahat Siddiqui]

(In reply to comment #3)
> (From update of attachment 109276 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109276&action=review
> 
> > Source/WebCore/platform/gtk/PopupMenuGtk.cpp:95
> > +    // GTK can refuse to actually open the menu when mouse grabs fails.
> > +    // Ensure WebCore does not go into some pesky state.
> > +    if (!gtk_widget_get_visible(m_popup->platformMenu()))
> > +        client()->popupDidHide();
> 
> You shold also include this fix in Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp.

Thanks for your comments, 
I need to talk to webcore from UIProcess to do client()->popupDidHide(). 
I see WebProcess calls ShowPopupMenu() in WebPopupMenu::show() and there is no such  m_popupClient available with UIProcess 
to do above task. There could be few ways to do it if i am correct?

1) Send m_popupClient as an argument to ShowPopupMenu()
2) Implement IPC mechanism from WebPopupMenuProxyGtk that will inform webcore if popupmenu is displayed. 
3) Have PopupMenuGtk owned with WebPopupMenuProxyGtk class with this we can access PopupMenuClient

Let me know your suggestions before i start implementing!

[Martin Robinson]

(In reply to comment #5)

> Thanks for your comments, 
> I need to talk to webcore from UIProcess to do client()->popupDidHide(). 
> I see WebProcess calls ShowPopupMenu() in WebPopupMenu::show() and there is no such  m_popupClient available with UIProcess 
> to do above task. There could be few ways to do it if i am correct?
> 
> 1) Send m_popupClient as an argument to ShowPopupMenu()
> 2) Implement IPC mechanism from WebPopupMenuProxyGtk that will inform webcore if popupmenu is displayed. 
> 3) Have PopupMenuGtk owned with WebPopupMenuProxyGtk class with this we can access PopupMenuClient
> 
> Let me know your suggestions before i start implementing!

My preference is that you create a virtual method in WebPopupMenuProxy::Client called something like failedToShowPopupMenu and implement it in WebPageProxy like the other pop messages. From there you need to an IPC message to pipe it to the WebProcess.

I would double check with andersca or Sam about this approach though. I'll CC them.

[Wajahat Siddiqui]

Created attachment 109769 [details]
reworked patch updated to work in webkit2 as well

Updated as per suggestions & comments ! 
1) Including the fix in context of webkit2.
2) Removing copyright line.

[Martin Robinson]

Comment on attachment 109769 [details]
reworked patch updated to work in webkit2 as well

View in context: https://bugs.webkit.org/attachment.cgi?id=109769&action=review

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2054
> +void WebPage::failedToShowPopupMenu()
> +{
> +    if (!m_activePopupMenu)
> +        return;
> +
> +    m_activePopupMenu->client()->popupDidHide();
> +}
> +

You might want to guard this code with a GTK+ #ifdef. I'm CCing some of the WebKit2 people.

[Wajahat Siddiqui]

(In reply to comment #8)
> (From update of attachment 109769 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=109769&action=review
> 
> > Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2054
> > +void WebPage::failedToShowPopupMenu()
> > +{
> > +    if (!m_activePopupMenu)
> > +        return;
> > +
> > +    m_activePopupMenu->client()->popupDidHide();
> > +}
> > +
> 
> You might want to guard this code with a GTK+ #ifdef. I'm CCing some of the WebKit2 people.

You mean #if PLATFORM(GTK) ? Do I need to submit another patch?

[Martin Robinson]

(In reply to comment #9)

> You mean #if PLATFORM(GTK) ? Do I need to submit another patch?

Yes, I think it makes sense to change the WebKit2 code to be wrapped with #if PLATFORM(GTK).

[Wajahat Siddiqui]

Created attachment 116483 [details]
updated as per comments

updated as per comments, wrapping GTK specific code in #if PLATFORM(GTK) 
for WebKit2 where ever required.

[Martin Robinson]

Comment on attachment 116483 [details]
updated as per comments

View in context: https://bugs.webkit.org/attachment.cgi?id=116483&action=review

> Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:94
> +    if (!gtk_widget_get_visible(m_popup->platformMenu()))
> +       m_client->failedToShowPopupMenu();
> +

Shouldn't you return early instead of going into a main loop here?

[Wajahat Siddiqui]

Created attachment 116895 [details]
updated patch rev 2

Right thanks for pointing it out, Also I see m_client check need to be present before calling popUp

[Martin Robinson]

Comment on attachment 116895 [details]
updated patch rev 2

View in context: https://bugs.webkit.org/attachment.cgi?id=116895&action=review

> Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:90
> +    if (!m_client)
> +        return;

In what cases can the m_client by null before running the event loop?

[Wajahat Siddiqui]

Comment on attachment 116895 [details]
updated patch rev 2

View in context: https://bugs.webkit.org/attachment.cgi?id=116895&action=review

>> Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:90
>> +        return;
> 
> In what cases can the m_client by null before running the event loop?

I felt this check was made after event loop should be present before since m_client is being used in 
m_popup->popUp(rect.size(), menuPosition, size, selectedIndex, m_client->currentlyProcessedMouseDownEvent() ? m_client->currentlyProcessedMouseDownEvent()->nativeEvent() : 0); ???

[Martin Robinson]

Comment on attachment 116895 [details]
updated patch rev 2

View in context: https://bugs.webkit.org/attachment.cgi?id=116895&action=review

> Source/WebKit2/UIProcess/gtk/WebPopupMenuProxyGtk.cpp:114
>  
>      g_signal_handler_disconnect(m_popup->platformMenu(), unmapHandler);
>  
> -    if (!m_client)
> -        return;
> -
>      m_client->valueChangedForPopupMenu(this, m_activeItem);

This worries me. What if the client existed before you ran the main loop, but not after. If the client can be null before and after the main loop run you need to check in both places. I'm not convinced that it can be null before though...

[Wajahat Siddiqui]

Created attachment 117337 [details]
updated patch rev 3

reverting m_client check in previous patch.

[WebKit Review Bot]

Comment on attachment 117337 [details]
updated patch rev 3

Clearing flags on attachment: 117337

Committed r101678: <http://trac.webkit.org/changeset/101678>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.