Bug 59124

Summary: WebKit2 thinks the web process is unresponsive when a plugin displays a context menu
Product: WebKit Reporter: Adam Roben (:aroben) <aroben>
Component: WebKit2Assignee: Jeff Miller <jeffm>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, jeffm, mitz, sam, webkit.review.bot
Priority: P2 Keywords: InRadar, PlatformOnly
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows 7   
URL: http://www.communitymx.com/content/source/E5141/wmodetrans.htm
Attachments:
Description Flags
Patch
none
Patch aroben: review+

Description Adam Roben (:aroben) 2011-04-21 11:41:49 PDT
To reproduce:

1. Go to http://www.communitymx.com/content/source/E5141/wmodetrans.htm
2. Right-click on the plugin and wait 3 seconds

The UI process ends up calling the didBecomeUnresponsive callback. Not good!
Comment 1 Adam Roben (:aroben) 2011-04-21 11:42:40 PDT
<rdar://problem/9318600>
Comment 2 Adam Roben (:aroben) 2011-04-21 11:44:25 PDT
See also bug 58943.
Comment 3 Adam Roben (:aroben) 2011-04-27 01:17:49 PDT
NetscapePlugin::handleMouseEvent in Source/WebKit2/WebProcess/Plugins/Netscape/NetscapePlugin.cpp is probably the right place to start investigating this.

We should also figure out if this only happens for windowless plugins (as in the case in comment 0), or also happens for windowed plugins. I would guess that this only happens for windowless plugins, as windowed plugins will receive mouse events directly from the OS so the UI process should never even see them. (The audio player on <http://frostyschristmastreefarm.com/> is a windowed plugin.)
Comment 4 Jeff Miller 2011-04-27 10:54:18 PDT
The root cause is that the UI process relies on receiving the Messages::WebPageProxy::DidReceiveEvent message in order to stop the responsiveness timer.  This is sent from the web process from WebPage::mouseEvent() after WebKit::handleMouseEvent() returns, which doesn't happen until context menu navigation is complete.

We hook TrackPopupMenu() to we can substitute a dummy window if TrackPopupMenu() is passed a window that is owned by another thread, we could also use this hook to send a new type of message to the UI process that would tell it to stop the responsiveness timer in this case (maybe only if we're in the middle of processing a mouse event?), but it's not clear if it's possible or desirable to send a message from this layer.
Comment 5 Jeff Miller 2011-04-27 11:11:31 PDT
Adam pointed out that maybe we should tell the UI process to stop the responsiveness timer for any event we send to the plugin in a NetscapePlugin::platformHandleXXXEvent() member function, since we have no way of knowing how long the plugin will take to process the event.  The downside of doing this is that if the plugin does become unresponsive, the user won't know this immediately unless they click on the web page again.  At this point, if the web process is unresponsive because of a hung plugin, the responsiveness timer will fire.

Also, the PluginView (which is also the PluginController) does know about its WebPage, so we can call back to the WebPage to send a message to the UI process to stop its responsive timer.
Comment 6 Jeff Miller 2011-04-29 10:53:36 PDT
(In reply to comment #3)
 > We should also figure out if this only happens for windowless plugins (as in the case in comment 0), or also happens for windowed plugins. I would guess that this only happens for windowless plugins, as windowed plugins will receive mouse events directly from the OS so the UI process should never even see them. (The audio player on <http://frostyschristmastreefarm.com/> is a windowed plugin.)

I verified that this is not an issue for windowed plugins, for the reasons Adam mentioned above.
Comment 7 Jeff Miller 2011-04-29 11:21:45 PDT
Created attachment 91709 [details]
Patch
Comment 8 mitz 2011-04-29 11:25:27 PDT
How does this bug relate to bug 58943?
Comment 9 Jeff Miller 2011-04-29 11:28:22 PDT
(In reply to comment #8)
> How does this bug relate to bug 58943?

It's the Windows version of that bug, although as I mentioned Anders fixed it in a completely different way.  Note that we don't run plugins in a separate process on Windows like we do on the Mac.
Comment 10 WebKit Review Bot 2011-04-29 14:12:59 PDT
Attachment 91709 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8517900
Comment 11 Jeff Miller 2011-05-02 11:58:11 PDT
Created attachment 91950 [details]
Patch
Comment 12 Adam Roben (:aroben) 2011-05-02 12:02:05 PDT
Comment on attachment 91950 [details]
Patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:2660
> +void WebPageProxy::stopResponsivenessTimer()
> +{
> +    // If we're sending an event to a plug-in, we can't control how long the plug-in
> +    // takes to process it (e.g. it may display a context menu), so we stop the
> +    // responsiveness timer in this case.
> +    process()->responsivenessTimer()->stop();
> +}

This comment doesn't seem appropriate here. I think you can just leave it out. The function is pretty self-explanatory.
Comment 13 Jeff Miller 2011-05-02 12:24:31 PDT
Committed r85502: <http://trac.webkit.org/changeset/85502>