Bug 195475 – REGRESSION (r242551): Sporadic hangs when tapping to change selection on iOS

• Component: WebKit2
• Status: RESOLVED FIXED

[Wenson Hsieh]

Reproduces if you tap to change selection quickly in a contenteditable area with lots of text.

What's happening here is that sync spellchecking IPC from web -> UI process is deadlocked with sync autocorrection request from UI -> web process :(

[Radar WebKit Bug Importer]

<rdar://problem/48721153>

[Wenson Hsieh]

Created attachment 364047 [details]
WIP

[Wenson Hsieh]

Created attachment 364060 [details]
Alternate approach (WIP)

[Wenson Hsieh]

Created attachment 364105 [details]
Patch

[EWS Watchlist]

Comment on attachment 364105 [details]
Patch

Attachment 364105 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11436959

New failing tests:
compositing/video/video-clip-change-src.html

[EWS Watchlist]

Created attachment 364122 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Chris Dumez]

Comment on attachment 364105 [details]
Patch

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

> Source/WebKit/Platform/IPC/Connection.cpp:514
> +    SyncMessageState::singleton().dispatchMessages(nullptr);

I am on board with the changes below but I do not understand this one. First of all, aren’t we on the main thread there? 
What if this method is called with three flag that says to interrupt rather than processing incoming sync ipc?

[Wenson Hsieh]

Comment on attachment 364105 [details]
Patch

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

>> Source/WebKit/Platform/IPC/Connection.cpp:514
>> +    SyncMessageState::singleton().dispatchMessages(nullptr);
> 
> I am on board with the changes below but I do not understand this one. First of all, aren’t we on the main thread there? 
> What if this method is called with three flag that says to interrupt rather than processing incoming sync ipc?

Right, we're on the main thread here. But that seems like it would be ok? In the case where we call Connection::sendSync(), we do the same thing off of the main thread:

Connection::sendSync
↳ Connection::sendSyncMessage
↳ Connection::waitForSyncReply
↳ Connection::SyncMessageState::dispatchMessages

The idea behind this change was to make Connection::waitForMessage behave in the same way as Connection::waitForSyncReply, by first dispatching incoming sync IPC before waiting. But you raise a good point — maybe when the InterruptWaitingIfSyncMessageArrives flag is set and SyncMessageState::m_messagesToDispatchWhileWaitingForSyncReply is not empty, we ought to just bail instead of trying to dispatch the message before waiting. What do you think about doing something like this?

[Chris Dumez]

Comment on attachment 364105 [details]
Patch

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

>>> Source/WebKit/Platform/IPC/Connection.cpp:514
>>> +    SyncMessageState::singleton().dispatchMessages(nullptr);
>> 
>> I am on board with the changes below but I do not understand this one. First of all, aren’t we on the main thread there? 
>> What if this method is called with three flag that says to interrupt rather than processing incoming sync ipc?
> 
> Right, we're on the main thread here. But that seems like it would be ok? In the case where we call Connection::sendSync(), we do the same thing off of the main thread:
> 
> Connection::sendSync
> ↳ Connection::sendSyncMessage
> ↳ Connection::waitForSyncReply
> ↳ Connection::SyncMessageState::dispatchMessages
> 
> The idea behind this change was to make Connection::waitForMessage behave in the same way as Connection::waitForSyncReply, by first dispatching incoming sync IPC before waiting. But you raise a good point — maybe when the InterruptWaitingIfSyncMessageArrives flag is set and SyncMessageState::m_messagesToDispatchWhileWaitingForSyncReply is not empty, we ought to just bail instead of trying to dispatch the message before waiting. What do you think about doing something like this?

I may be misreading the code but I still do not understand why this is needed. Connection::processIncomingMessage() will already take care of dispatching incoming sync messages if needed, and only if WaitForOption::InterruptWaitingIfSyncMessageArrives is not set.

[Chris Dumez]

Comment on attachment 364105 [details]
Patch

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

>>>> Source/WebKit/Platform/IPC/Connection.cpp:514
>>>> +    SyncMessageState::singleton().dispatchMessages(nullptr);
>>> 
>>> I am on board with the changes below but I do not understand this one. First of all, aren’t we on the main thread there? 
>>> What if this method is called with three flag that says to interrupt rather than processing incoming sync ipc?
>> 
>> Right, we're on the main thread here. But that seems like it would be ok? In the case where we call Connection::sendSync(), we do the same thing off of the main thread:
>> 
>> Connection::sendSync
>> ↳ Connection::sendSyncMessage
>> ↳ Connection::waitForSyncReply
>> ↳ Connection::SyncMessageState::dispatchMessages
>> 
>> The idea behind this change was to make Connection::waitForMessage behave in the same way as Connection::waitForSyncReply, by first dispatching incoming sync IPC before waiting. But you raise a good point — maybe when the InterruptWaitingIfSyncMessageArrives flag is set and SyncMessageState::m_messagesToDispatchWhileWaitingForSyncReply is not empty, we ought to just bail instead of trying to dispatch the message before waiting. What do you think about doing something like this?
> 
> I may be misreading the code but I still do not understand why this is needed. Connection::processIncomingMessage() will already take care of dispatching incoming sync messages if needed, and only if WaitForOption::InterruptWaitingIfSyncMessageArrives is not set.

Ok, looking more into Connection::waitForSyncReply(). I do believe you are right that this is needed. One difference though is that Connection::waitForSyncReply() has the SyncMessageState::dispatchMessages() call inside the loop while you have it outside. Any reason why?

[Wenson Hsieh]

Comment on attachment 364105 [details]
Patch

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

>>>>> Source/WebKit/Platform/IPC/Connection.cpp:514
>>>>> +    SyncMessageState::singleton().dispatchMessages(nullptr);
>>>> 
>>>> I am on board with the changes below but I do not understand this one. First of all, aren’t we on the main thread there? 
>>>> What if this method is called with three flag that says to interrupt rather than processing incoming sync ipc?
>>> 
>>> Right, we're on the main thread here. But that seems like it would be ok? In the case where we call Connection::sendSync(), we do the same thing off of the main thread:
>>> 
>>> Connection::sendSync
>>> ↳ Connection::sendSyncMessage
>>> ↳ Connection::waitForSyncReply
>>> ↳ Connection::SyncMessageState::dispatchMessages
>>> 
>>> The idea behind this change was to make Connection::waitForMessage behave in the same way as Connection::waitForSyncReply, by first dispatching incoming sync IPC before waiting. But you raise a good point — maybe when the InterruptWaitingIfSyncMessageArrives flag is set and SyncMessageState::m_messagesToDispatchWhileWaitingForSyncReply is not empty, we ought to just bail instead of trying to dispatch the message before waiting. What do you think about doing something like this?
>> 
>> I may be misreading the code but I still do not understand why this is needed. Connection::processIncomingMessage() will already take care of dispatching incoming sync messages if needed, and only if WaitForOption::InterruptWaitingIfSyncMessageArrives is not set.
> 
> Ok, looking more into Connection::waitForSyncReply(). I do believe you are right that this is needed. One difference though is that Connection::waitForSyncReply() has the SyncMessageState::dispatchMessages() call inside the loop while you have it outside. Any reason why?

Now that you mention it, I can't think of any good reason :P In fact, this should probably be in the while loop so that if multiple incoming sync messages arrive while we're in Connection::waitForMessage and the InterruptWaitingIfSyncMessageArrives flag isn't set, then we won't end up in deadlock.

[Wenson Hsieh]

Created attachment 364153 [details]
Patch

[Chris Dumez]

Comment on attachment 364153 [details]
Patch

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

r=me

> Source/WebKit/Platform/IPC/Connection.cpp:517
> +        // Handle any messages that are blocked on our response from us.

nit: I think "a response from us" sounds better than "our response from us".

[Wenson Hsieh]

Comment on attachment 364153 [details]
Patch

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

>> Source/WebKit/Platform/IPC/Connection.cpp:517
>> +        // Handle any messages that are blocked on our response from us.
> 
> nit: I think "a response from us" sounds better than "our response from us".

Oops, fixed!

[Wenson Hsieh]

Created attachment 364164 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 364164 [details]
Patch for landing

Clearing flags on attachment: 364164

Committed r242682: <https://trac.webkit.org/changeset/242682>