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 :(
<rdar://problem/48721153>
Created attachment 364047 [details] WIP
Created attachment 364060 [details] Alternate approach (WIP)
Created attachment 364105 [details] Patch
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
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
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 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 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 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 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.
Created attachment 364153 [details] Patch
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 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!
Created attachment 364164 [details] Patch for landing
Comment on attachment 364164 [details] Patch for landing Clearing flags on attachment: 364164 Committed r242682: <https://trac.webkit.org/changeset/242682>