Bug 51351 - UI process should respond to synchronous messages from the web process on a non-main thread by default
Summary: UI process should respond to synchronous messages from the web process on a n...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks: 51352
  Show dependency treegraph
 
Reported: 2010-12-20 13:21 PST by Adam Roben (:aroben)
Modified: 2015-06-18 14:56 PDT (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Roben (:aroben) 2010-12-20 13:21:43 PST
The UI process currently responds to both synchronous and asynchronous messages from the web process on the main thread. Messages are received on the CoreIPC::Connection's work queue and then forwarded to the main thread for processing.

This can cause deadlocks on Windows when windowed plugins are involved. For example, take the case of a YouTube video embedded into some third party web page. Here's what happens:

Web process:
Clicking on the plugin opens a new window so that it can be navigated to the video's YouTube page. In order to open the new window, the web process sends a synchronous WebPageProxy::CreateNewPage message and blocks its main thread to wait for the reply.

UI process:
The UI process receives the WebPageProxy::CreateNewPage message, and ends up calling through to WKPageUIClient::createNewPage. The client app responds to this callback by creating a new window and selecting it. Selecting the new window causes the plugin to lose focus. As part of unfocusing the plugin, Windows sends the plugin a synchronous WM_KILLFOCUS message to it and blocks the main thread while waiting for the message to be handled.

Since the web process's main thread is still blocked waiting for the reply to WebPageProxy::CreateNewPage, it can't process the WM_KILLFOCUS message, so the two processes deadlock. (The web process is waiting for CreateNewPage to be handled, and the UI process is waiting for WM_KILLFOCUS to be handled.)

We should make the UI process respond to synchronous messages on a non-main thread by default. We'll want to continue making WK2 client callbacks (e.g., WKPageUIClient::createNewPage) on the main thread, but those need to happen asynchronously to avoid deadlocks.
Comment 1 Adam Roben (:aroben) 2010-12-20 13:24:13 PST
(In reply to comment #0)
> We should make the UI process respond to synchronous messages on a non-main thread by default.

Messages that end up displaying UI still need to be processed on the main thread. Fixing deadlocks with those messages is covered by bug 51352.
Comment 2 Adam Roben (:aroben) 2010-12-20 13:24:46 PST
<rdar://problem/8790535>
Comment 3 Adam Roben (:aroben) 2011-01-26 17:19:34 PST
We've decided to see if we can get away without this. We'll still have to fix bug 51352.
Comment 4 Adam Roben (:aroben) 2011-01-26 18:08:23 PST
(In reply to comment #3)
> We've decided to see if we can get away without this. We'll still have to fix bug 51352.

Anders and I just talked some more and realized that we will probably need to fix this eventually. Consider the following situation:

Web process:
Sends some asynchronous messages to the UI process, then sends a synchronous message (that doesn't present UI and thus is not covered by bug 51352, e.g., GetToolbarsAreVisible), and blocks waiting for a reply.

UI process:
While processing one of the asynchronous messages, touches the window hierarchy, resulting in a synchronous message to the web process.

So we've ended up in a deadlock again even if bug 51352 is fixed.

Our plan is threefold:
1) Make the web process spin a run loop while waiting for a reply to any synchronous message, not just those that end up presenting UI in the UI process.
2) Add a test mode to WebKit2 that exercises the reentrancy that spinning a nested run loop enables.
3) Eventually change WebKit2 only to spin a run loop while waiting for a reply to a synchronous message that will present UI, and move all other synchronous message handling to a non-main thread, as described in comment 1.

If the testing from (2) uncovers many intractable issues, we may do (3) sooner rather than later.
Comment 5 Adam Roben (:aroben) 2011-01-26 18:11:14 PST
Whoops, forgot to reopen the bug.
Comment 6 Adam Roben (:aroben) 2011-02-10 12:37:57 PST
The deadlocks caused by this bug are even more frequent now that decidePolicyForNavigationAction is synchronous.
Comment 7 Adam Roben (:aroben) 2011-04-11 09:07:15 PDT
(In reply to comment #4)
> 1) Make the web process spin a run loop while waiting for a reply to any synchronous message, not just those that end up presenting UI in the UI process.

This is now bug 58239.

> 2) Add a test mode to WebKit2 that exercises the reentrancy that spinning a nested run loop enables.

This is bug 53218.

> 3) Eventually change WebKit2 only to spin a run loop while waiting for a reply to a synchronous message that will present UI, and move all other synchronous message handling to a non-main thread, as described in comment 1.

This is bug 51352.
Comment 8 Anders Carlsson 2015-06-18 14:56:20 PDT
We got rid of WK2 for Windows so this no longer applies.