Bug 195354 - [iOS] Frequent 1 second IPC deadlocks when showing a paste callout
Summary: [iOS] Frequent 1 second IPC deadlocks when showing a paste callout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-05 21:09 PST by Wenson Hsieh
Modified: 2019-03-06 09:38 PST (History)
6 users (show)

See Also:


Attachments
Patch (22.90 KB, patch)
2019-03-05 21:57 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch for EWS (24.02 KB, patch)
2019-03-06 08:15 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch for EWS (24.01 KB, patch)
2019-03-06 08:18 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-05 21:09:47 PST
<rdar://problem/48624675>
Comment 1 Wenson Hsieh 2019-03-05 21:57:49 PST
Created attachment 363731 [details]
Patch
Comment 2 Tim Horton 2019-03-05 23:07:13 PST
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
Comment 3 Wenson Hsieh 2019-03-06 07:27:43 PST
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

☠️🔒
Comment 4 Wenson Hsieh 2019-03-06 08:15:43 PST Comment hidden (obsolete)
Comment 5 Wenson Hsieh 2019-03-06 08:18:15 PST
Created attachment 363741 [details]
Patch for EWS
Comment 6 WebKit Commit Bot 2019-03-06 09:38:43 PST
Comment on attachment 363741 [details]
Patch for EWS

Clearing flags on attachment: 363741

Committed r242551: <https://trac.webkit.org/changeset/242551>
Comment 7 WebKit Commit Bot 2019-03-06 09:38:45 PST
All reviewed patches have been landed.  Closing bug.