Bug 195354 – [iOS] Frequent 1 second IPC deadlocks when showing a paste callout

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Wenson Hsieh]

<rdar://problem/48624675>

[Wenson Hsieh]

Created attachment 363731 [details]
Patch

[Tim Horton]

Comment on attachment 363731 [details]
Patch

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

> Source/WebKit/UIProcess/PageClient.h:399
> +    virtual void handleAutocorrectionContextSync(const WebAutocorrectionContext&) = 0;

This name seems ... odd. Mostly because I think I expect the -Sync suffix on things that block on sync IPC, but this isn't that!

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:579
> +    m_process->send(Messages::WebPage::RequestAutocorrectionContextSync(), m_pageID);

Oh... Even the request one is named wrong! Get rid of all the -Sync; there's nothing inherently synchronous about this message or functions related to it anymore.

This is just request, the other one is just the reply, and we just *happen* to block on the reply in the UI process.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6375
> +    // FIXME: Computing and sending an autocorrection context is a workaround for the fact that autocorrection context

You know how I feel about this already but it is a clever solution to a problem with no clear alternatives.

> LayoutTests/ChangeLog:10
> +        Most of these tests currently encounter and rely on the 1 second IPC timeout to finish. To test this fix, force
> +        `ignoreSynchronousMessagingTimeouts=true` to make them fail if the processes encounter a deadlock.

Hilarious

[Wenson Hsieh]

Comment on attachment 363731 [details]
Patch

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

>> Source/WebKit/UIProcess/PageClient.h:399
>> +    virtual void handleAutocorrectionContextSync(const WebAutocorrectionContext&) = 0;
> 
> This name seems ... odd. Mostly because I think I expect the -Sync suffix on things that block on sync IPC, but this isn't that!

Indeed! (see below)

>> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:579
>> +    m_process->send(Messages::WebPage::RequestAutocorrectionContextSync(), m_pageID);
> 
> Oh... Even the request one is named wrong! Get rid of all the -Sync; there's nothing inherently synchronous about this message or functions related to it anymore.
> 
> This is just request, the other one is just the reply, and we just *happen* to block on the reply in the UI process.

That's a good point! We just happen to waitForAndDispatchImmediately when using these two.

I had named this RequestAutocorrectionContextSync() to avoid conflicting with an existing RequestAutocorrectionContext() message, but now that I think about it some more, having RequestAutocorrectionContext() is redundant. I'll remove the existing RequestAutocorrectionContext() and then rename what I call RequestAutocorrectionContextSync() here to just RequestAutocorrectionContext().

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:6375
>> +    // FIXME: Computing and sending an autocorrection context is a workaround for the fact that autocorrection context
> 
> You know how I feel about this already but it is a clever solution to a problem with no clear alternatives.

Agreed, this is just a stop gap :(

We really ought to push for <rdar://problem/16207002> (in reality, <rdar://problem/48383001>).

>> LayoutTests/ChangeLog:10
>> +        `ignoreSynchronousMessagingTimeouts=true` to make them fail if the processes encounter a deadlock.
> 
> Hilarious

☠️🔒

[Wenson Hsieh]

Created attachment 363740 [details]
Patch for EWS

[Wenson Hsieh]

Created attachment 363741 [details]
Patch for EWS

[WebKit Commit Bot]

Comment on attachment 363741 [details]
Patch for EWS

Clearing flags on attachment: 363741

Committed r242551: <https://trac.webkit.org/changeset/242551>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

Comment on attachment 363741 [details]
Patch for EWS

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

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3613
> +        _page->process().connection()->waitForAndDispatchImmediately<Messages::WebPageProxy::HandleAutocorrectionContext>(_page->pageID(), 1_s, IPC::WaitForOption::InterruptWaitingIfSyncMessageArrives);
> +        [self _invokePendingAutocorrectionContextHandler:WKAutocorrectionContext.emptyAutocorrectionContext];

This makes autocorrection context updating non-deterministic: sometimes UIKit gets valid context data and sometimes it get an empty context, which is indistinguishable from editing an empty element. See <rdar://problem/62605526>.

[Wenson Hsieh]

Comment on attachment 363741 [details]
Patch for EWS

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

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3613
>> +        [self _invokePendingAutocorrectionContextHandler:WKAutocorrectionContext.emptyAutocorrectionContext];
> 
> This makes autocorrection context updating non-deterministic: sometimes UIKit gets valid context data and sometimes it get an empty context, which is indistinguishable from editing an empty element. See <rdar://problem/62605526>.

I assume that the 1 second IPC timeout wasn’t hit while reproducing this bug. If it is, then it seems the real regression is whatever’s causing the web process to deadlock or become otherwise unresponsive while this IPC message is being sent.

In the case where the web process is not hosed, it isn’t clear to me how this change result in non-deterministic behavior, unless `waitForAndDispatchImmediately` sometimes returns *before* invoking the IPC receiver. I’ll first try reverting out this change, to verify that it is the cause.

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #9)
> Comment on attachment 363741 [details]
> Patch for EWS
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=363741&action=review
> 
> >> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:3613
> >> +        [self _invokePendingAutocorrectionContextHandler:WKAutocorrectionContext.emptyAutocorrectionContext];
> > 
> > This makes autocorrection context updating non-deterministic: sometimes UIKit gets valid context data and sometimes it get an empty context, which is indistinguishable from editing an empty element. See <rdar://problem/62605526>.
> 
> I assume that the 1 second IPC timeout wasn’t hit while reproducing this
> bug. If it is, then it seems the real regression is whatever’s causing the
> web process to deadlock or become otherwise unresponsive while this IPC
> message is being sent.
> 
> In the case where the web process is not hosed, it isn’t clear to me how
> this change result in non-deterministic behavior, unless
> `waitForAndDispatchImmediately` sometimes returns *before* invoking the IPC
> receiver. I’ll first try reverting out this change, to verify that it is the
> cause.

To follow up — this change does not appear to be the cause of <rdar://problem/62605526>.