Bug 56466 - Popup menu can get stuck in closed state when GtkMenu can't grab mouse.
Summary: Popup menu can get stuck in closed state when GtkMenu can't grab mouse.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-03-16 09:23 PDT by Martin H.
Modified: 2011-12-01 09:39 PST (History)
5 users (show)

See Also:


Attachments
patch to fix the bug (784 bytes, patch)
2011-03-16 09:23 PDT, Martin H.
no flags Details | Formatted Diff | Diff
proposed patch to fix this issue in recent revision (1.79 KB, patch)
2011-09-30 03:28 PDT, Wajahat Siddiqui
mrobinson: review-
Details | Formatted Diff | Diff
reworked patch updated to work in webkit2 as well (8.02 KB, patch)
2011-10-05 03:45 PDT, Wajahat Siddiqui
no flags Details | Formatted Diff | Diff
updated as per comments (8.34 KB, patch)
2011-11-24 00:40 PST, Wajahat Siddiqui
mrobinson: review-
Details | Formatted Diff | Diff
updated patch rev 2 (8.65 KB, patch)
2011-11-28 22:10 PST, Wajahat Siddiqui
mrobinson: review-
Details | Formatted Diff | Diff
updated patch rev 3 (8.37 KB, patch)
2011-11-30 22:08 PST, Wajahat Siddiqui
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin H. 2011-03-16 09:23:38 PDT
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()
Comment 1 Martin Robinson 2011-03-18 14:15:48 PDT
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
Comment 2 Wajahat Siddiqui 2011-09-30 03:28:45 PDT
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
Comment 3 Martin Robinson 2011-09-30 06:34:42 PDT
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.
Comment 4 Martin Robinson 2011-09-30 06:35:14 PDT
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.
Comment 5 Wajahat Siddiqui 2011-10-03 03:13:07 PDT
(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!
Comment 6 Martin Robinson 2011-10-03 08:49:17 PDT
(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.
Comment 7 Wajahat Siddiqui 2011-10-05 03:45:37 PDT
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.
Comment 8 Martin Robinson 2011-10-05 08:09:12 PDT
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.
Comment 9 Wajahat Siddiqui 2011-10-06 22:40:22 PDT
(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?
Comment 10 Martin Robinson 2011-11-23 04:21:10 PST
(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).
Comment 11 Wajahat Siddiqui 2011-11-24 00:40:02 PST
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.
Comment 12 Martin Robinson 2011-11-28 02:15:20 PST
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?
Comment 13 Wajahat Siddiqui 2011-11-28 22:10:50 PST
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
Comment 14 Martin Robinson 2011-11-29 01:47:07 PST
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?
Comment 15 Wajahat Siddiqui 2011-11-29 02:07:20 PST
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); ???
Comment 16 Martin Robinson 2011-11-30 12:44:14 PST
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...
Comment 17 Wajahat Siddiqui 2011-11-30 22:08:48 PST
Created attachment 117337 [details]
updated patch rev 3

reverting m_client check in previous patch.
Comment 18 WebKit Review Bot 2011-12-01 09:39:45 PST
Comment on attachment 117337 [details]
updated patch rev 3

Clearing flags on attachment: 117337

Committed r101678: <http://trac.webkit.org/changeset/101678>
Comment 19 WebKit Review Bot 2011-12-01 09:39:50 PST
All reviewed patches have been landed.  Closing bug.