Bug 59124 – WebKit2 thinks the web process is unresponsive when a plugin displays a context menu

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Adam Roben (:aroben)]

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!

[Adam Roben (:aroben)]

<rdar://problem/9318600>

[Adam Roben (:aroben)]

See also bug 58943.

[Adam Roben (:aroben)]

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

[Jeff Miller]

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.

[Jeff Miller]

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.

[Jeff Miller]

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

[Jeff Miller]

Created attachment 91709 [details]
Patch

How does this bug relate to bug 58943?

[Jeff Miller]

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

[WebKit Review Bot]

Attachment 91709 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8517900

[Jeff Miller]

Created attachment 91950 [details]
Patch

[Adam Roben (:aroben)]

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.

[Jeff Miller]

Committed r85502: <http://trac.webkit.org/changeset/85502>