<rdar://problem/48624675>
Created attachment 363731 [details] Patch
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 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 ☠️🔒
Created attachment 363740 [details] Patch for EWS
Created attachment 363741 [details] Patch for EWS
Comment on attachment 363741 [details] Patch for EWS Clearing flags on attachment: 363741 Committed r242551: <https://trac.webkit.org/changeset/242551>
All reviewed patches have been landed. Closing bug.
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>.
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.
(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>.