Bug 195475 - REGRESSION (r242551): Sporadic hangs when tapping to change selection on iOS
Summary: REGRESSION (r242551): Sporadic hangs when tapping to change selection on iOS
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-08 12:12 PST by Wenson Hsieh
Modified: 2019-03-10 12:56 PDT (History)
7 users (show)

See Also:


Attachments
WIP (2.57 KB, patch)
2019-03-08 12:20 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Alternate approach (WIP) (3.25 KB, patch)
2019-03-08 13:58 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (12.10 KB, patch)
2019-03-08 18:32 PST, Wenson Hsieh
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (2.87 MB, application/zip)
2019-03-09 00:36 PST, Build Bot
no flags Details
Patch (12.10 KB, patch)
2019-03-09 18:31 PST, Wenson Hsieh
cdumez: review+
Details | Formatted Diff | Diff
Patch for landing (12.10 KB, patch)
2019-03-09 21:07 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 2019-03-08 12:12:07 PST
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 :(
Comment 1 Radar WebKit Bug Importer 2019-03-08 12:12:48 PST
<rdar://problem/48721153>
Comment 2 Wenson Hsieh 2019-03-08 12:20:07 PST
Created attachment 364047 [details]
WIP
Comment 3 Wenson Hsieh 2019-03-08 13:58:20 PST
Created attachment 364060 [details]
Alternate approach (WIP)
Comment 4 Wenson Hsieh 2019-03-08 18:32:11 PST
Created attachment 364105 [details]
Patch
Comment 5 Build Bot 2019-03-09 00:36:18 PST Comment hidden (obsolete)
Comment 6 Build Bot 2019-03-09 00:36:20 PST Comment hidden (obsolete)
Comment 7 Chris Dumez 2019-03-09 07:24:32 PST
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?
Comment 8 Wenson Hsieh 2019-03-09 11:43:00 PST
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?
Comment 9 Chris Dumez 2019-03-09 12:40:21 PST
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.
Comment 10 Chris Dumez 2019-03-09 12:49:09 PST
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?
Comment 11 Wenson Hsieh 2019-03-09 17:48:20 PST
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.
Comment 12 Wenson Hsieh 2019-03-09 18:31:38 PST
Created attachment 364153 [details]
Patch
Comment 13 Chris Dumez 2019-03-09 18:37:25 PST
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".
Comment 14 Wenson Hsieh 2019-03-09 19:21:34 PST
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!
Comment 15 Wenson Hsieh 2019-03-09 21:07:49 PST
Created attachment 364164 [details]
Patch for landing
Comment 16 WebKit Commit Bot 2019-03-09 21:43:42 PST
Comment on attachment 364164 [details]
Patch for landing

Clearing flags on attachment: 364164

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