REGRESSION (r242551): Sporadic hangs when tapping to change selection on iOS <>
Sun, 10 Mar 2019 05:43:36 +0000 (05:43 +0000) <>
Sun, 10 Mar 2019 05:43:36 +0000 (05:43 +0000)

Reviewed by Chris Dumez.


r242551 refactored synchronous autocorrection context requests to send an async IPC message and then use
waitForAndDispatchImmediately, instead of calling sendSync. However, this exposes a couple of existing issues in
the implementation of waitForAndDispatchImmediately that causes sporadic IPC deadlocks when changing selection.

First, passing in InterruptWaitingIfSyncMessageArrives when synchronously waiting for an IPC message currently
does not fulfill its intended behavior of interrupting waiting when a sync message arrives. This is because sync
IPC messages, by default, may be dispatched while the receiver is waiting for a sync reply. This means that the
logic in Connection::SyncMessageState::processIncomingMessage to dispatch an incoming sync message on the main
thread will attempt to handle the incoming message by enqueueing it on the main thread, and then waking up the
client runloop (i.e. signaling m_waitForSyncReplySemaphore). This works in the case of sendSync since the sync
reply semaphore is used to block the main thread, but in the case of waitForAndDispatchImmediately, a different
m_waitForMessageCondition is used instead, so SyncMessageState::processIncomingMessage will only enqueue the
incoming sync message on the main thread, and not actually invoke it.

To fix this first issue, observe that there is pre-existing logic to enqueue the incoming message and signal
m_waitForMessageCondition in Connection::processIncomingMessage. This codepath is currently not taken because we
end up bailing early in the call to SyncMessageState::processIncomingMessage. Instead, we can move this early
return further down in the function, such that if there is an incoming sync message and we're waiting with the
InterruptWaitingIfSyncMessageArrives option, we will correctly enqueue the incoming message, wake the runloop,
and proceed to handle the incoming message.

The second issue is more subtle; consider the scenario in which we send a sync message A from the web process to
the UI process, and simultaneously, in the UI process, we schedule some work to be done on the main thread.
Let's additionally suppose that this scheduled work will send an IPC message B to the web process and
synchronously wait for a reply (in the case of this particular bug, this is the sync autocorrection context
request). What happens upon receiving sync message A is that the IPC thread in the UI process will schedule A on
the main thread; however, before the scheduled response to A is invoked, we will first invoke previously
scheduled work that attempts to block synchronously until a message B is received. In summary:

1. (Web process)    sends sync IPC message A to UI process.
2. (UI process)     schedules some main runloop task that will block synchronously on IPC message B.
3. (UI process)     receives sync IPC message A and schedules A on the main runloop.
4. (UI process)     carry out the task scheduled in (2) and block on B.

...and then, the UI process and web process are now deadlocked because the UI process is waiting for B to
arrive, but the web process can't send B because it's waiting for a reply for IPC message A! To fix this second
deadlock, we first make an important observation: when using sendSync, we don't run into this problem because
immediately before sending sync IPC, we will attempt to handle any incoming sync IPC messages that have been
queued up. However, when calling waitForAndDispatchImmediately, we don't have this extra step, so a deadlock may
occur in the manner described above. To fix this, we make waitForAndDispatchImmediately behave more like
sendSync, by handling all incoming sync messages prior to blocking on an IPC response.

Test: editing/selection/ios/change-selection-by-tapping.html

* Platform/IPC/Connection.cpp:


Add a new layout test that taps to change selection 20 times in a contenteditable area and additionally
disables IPC timeout, to ensure that any IPC deadlocks will result in the test failing due to timing out.

* editing/selection/ios/change-selection-by-tapping-expected.txt: Added.
* editing/selection/ios/change-selection-by-tapping.html: Added.

git-svn-id: 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/editing/selection/ios/change-selection-by-tapping-expected.txt [new file with mode: 0644]
LayoutTests/editing/selection/ios/change-selection-by-tapping.html [new file with mode: 0644]

index 57c91dc..ccd57c7 100644 (file)
@@ -1,3 +1,17 @@
+2019-03-09  Wenson Hsieh  <>
+        REGRESSION (r242551): Sporadic hangs when tapping to change selection on iOS
+        <rdar://problem/48721153>
+        Reviewed by Chris Dumez.
+        Add a new layout test that taps to change selection 20 times in a contenteditable area and additionally
+        disables IPC timeout, to ensure that any IPC deadlocks will result in the test failing due to timing out.
+        * editing/selection/ios/change-selection-by-tapping-expected.txt: Added.
+        * editing/selection/ios/change-selection-by-tapping.html: Added.
 2019-03-09  Zalan Bujtas  <>
         [ContentChangeObserver] Click event fires immediately on hover menu at
diff --git a/LayoutTests/editing/selection/ios/change-selection-by-tapping-expected.txt b/LayoutTests/editing/selection/ios/change-selection-by-tapping-expected.txt
new file mode 100644 (file)
index 0000000..3823f7e
--- /dev/null
@@ -0,0 +1,11 @@
+Here's to the crazy ones, the misfits, the rebels, the trouble makers, the round pegs in the square holes, the ones who see things differently. There not fond of rules, and they have no respect for the status quo, you can quote then, disagree with them, glorify or vilify them, about the only thing you can't do is ignore them. Because they change things. They push the human race forward. And while some may see them as the crazy ones, we see genius. Because the people who are crazy enough to think they can change the world are the ones who do.
+Verifies that rapidly tapping to change selection doesn't hang due to IPC deadlock. To verify manually, focus the editable text and tap repeatedly in different parts of the editable area to change selection; check that this does not result in sporadic 1-second IPC hangs.
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+PASS successfullyParsed is true
diff --git a/LayoutTests/editing/selection/ios/change-selection-by-tapping.html b/LayoutTests/editing/selection/ios/change-selection-by-tapping.html
new file mode 100644 (file)
index 0000000..edd75c3
--- /dev/null
@@ -0,0 +1,56 @@
+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ignoreSynchronousMessagingTimeouts=true ] -->
+<script src="../../../resources/js-test.js"></script>
+<script src="../../../resources/ui-helper.js"></script>
+<meta name=viewport content="width=device-width, initial-scale=1, user-scalable=no">
+body, html {
+    width: 100%;
+    height: 100%;
+    margin: 0;
+#editor {
+    width: 300px;
+    height: 320px;
+    font-size: 18px;
+jsTestIsAsync = true;
+function tapAndWaitForSelectionChange(x, y) {
+    return new Promise(resolve => {
+        const editor = document.getElementById("editor");
+        let doneCount = 0;
+        const checkDone = () => {
+            if (++doneCount != 2)
+                return;
+            document.removeEventListener("selectionchange", checkDone);
+            resolve();
+        }
+        document.addEventListener("selectionchange", checkDone);
+        UIHelper.activateAt(x, y).then(checkDone);
+    });
+addEventListener("load", async () => {
+    description("Verifies that rapidly tapping to change selection doesn't hang due to IPC deadlock. To verify manually, focus the editable text and tap repeatedly in different parts of the editable area to change selection; check that this does not result in sporadic 1-second IPC hangs.");
+    await UIHelper.activateElementAndWaitForInputSession(document.getElementById("editor"));
+    for (let i = 0; i < 5; ++i) {
+        for (const [x, y] of [[40, 40], [220, 40], [40, 240], [220, 240]])
+            await tapAndWaitForSelectionChange(x, y);
+    }
+    finishJSTest();
+<p contenteditable id="editor">Here's to the crazy ones, the misfits, the rebels, the trouble makers, the round pegs in the square holes, the ones who see things differently. There not fond of rules, and they have no respect for the status quo, you can quote then, disagree with them, glorify or vilify them, about the only thing you can't do is ignore them.  Because they change things. They push the human race forward. And while some may see them as the crazy ones, we see genius. Because the people who are crazy enough to think they can change the world are the ones who do.</p>
+    <p id="description"></p>
+    <p id="console"></p>
index bf26966..33525ec 100644 (file)
@@ -1,3 +1,59 @@
+2019-03-09  Wenson Hsieh  <>
+        REGRESSION (r242551): Sporadic hangs when tapping to change selection on iOS
+        <rdar://problem/48721153>
+        Reviewed by Chris Dumez.
+        r242551 refactored synchronous autocorrection context requests to send an async IPC message and then use
+        waitForAndDispatchImmediately, instead of calling sendSync. However, this exposes a couple of existing issues in
+        the implementation of waitForAndDispatchImmediately that causes sporadic IPC deadlocks when changing selection.
+        First, passing in InterruptWaitingIfSyncMessageArrives when synchronously waiting for an IPC message currently
+        does not fulfill its intended behavior of interrupting waiting when a sync message arrives. This is because sync
+        IPC messages, by default, may be dispatched while the receiver is waiting for a sync reply. This means that the
+        logic in Connection::SyncMessageState::processIncomingMessage to dispatch an incoming sync message on the main
+        thread will attempt to handle the incoming message by enqueueing it on the main thread, and then waking up the
+        client runloop (i.e. signaling m_waitForSyncReplySemaphore). This works in the case of sendSync since the sync
+        reply semaphore is used to block the main thread, but in the case of waitForAndDispatchImmediately, a different
+        m_waitForMessageCondition is used instead, so SyncMessageState::processIncomingMessage will only enqueue the
+        incoming sync message on the main thread, and not actually invoke it.
+        To fix this first issue, observe that there is pre-existing logic to enqueue the incoming message and signal
+        m_waitForMessageCondition in Connection::processIncomingMessage. This codepath is currently not taken because we
+        end up bailing early in the call to SyncMessageState::processIncomingMessage. Instead, we can move this early
+        return further down in the function, such that if there is an incoming sync message and we're waiting with the
+        InterruptWaitingIfSyncMessageArrives option, we will correctly enqueue the incoming message, wake the runloop,
+        and proceed to handle the incoming message.
+        The second issue is more subtle; consider the scenario in which we send a sync message A from the web process to
+        the UI process, and simultaneously, in the UI process, we schedule some work to be done on the main thread.
+        Let's additionally suppose that this scheduled work will send an IPC message B to the web process and
+        synchronously wait for a reply (in the case of this particular bug, this is the sync autocorrection context
+        request). What happens upon receiving sync message A is that the IPC thread in the UI process will schedule A on
+        the main thread; however, before the scheduled response to A is invoked, we will first invoke previously
+        scheduled work that attempts to block synchronously until a message B is received. In summary:
+        1. (Web process)    sends sync IPC message A to UI process.
+        2. (UI process)     schedules some main runloop task that will block synchronously on IPC message B.
+        3. (UI process)     receives sync IPC message A and schedules A on the main runloop.
+        4. (UI process)     carry out the task scheduled in (2) and block on B.
+        ...and then, the UI process and web process are now deadlocked because the UI process is waiting for B to
+        arrive, but the web process can't send B because it's waiting for a reply for IPC message A! To fix this second
+        deadlock, we first make an important observation: when using sendSync, we don't run into this problem because
+        immediately before sending sync IPC, we will attempt to handle any incoming sync IPC messages that have been
+        queued up. However, when calling waitForAndDispatchImmediately, we don't have this extra step, so a deadlock may
+        occur in the manner described above. To fix this, we make waitForAndDispatchImmediately behave more like
+        sendSync, by handling all incoming sync messages prior to blocking on an IPC response.
+        Test: editing/selection/ios/change-selection-by-tapping.html
+        * Platform/IPC/Connection.cpp:
+        (IPC::Connection::waitForMessage):
+        (IPC::Connection::processIncomingMessage):
 2019-03-09  Andy Estes  <>
         [Apple Pay] CanMakePaymentsWithActiveCard and OpenPaymentSetup should be async messages
index 174b04c..592e772 100644 (file)
@@ -514,6 +514,9 @@ std::unique_ptr<Decoder> Connection::waitForMessage(StringReference messageRecei
     // Now wait for it to be set.
     while (true) {
+        // Handle any messages that are blocked on a response from us.
+        SyncMessageState::singleton().dispatchMessages(nullptr);
         std::unique_lock<Lock> lock(m_waitForMessageMutex);
         if (m_waitingForMessage->decoder) {
@@ -722,13 +725,7 @@ void Connection::processIncomingMessage(std::unique_ptr<Decoder> message)
-    // Check if this is a sync message or if it's a message that should be dispatched even when waiting for
-    // a sync reply. If it is, and we're waiting for a sync reply this message needs to be dispatched.
-    // If we don't we'll end up with a deadlock where both sync message senders are stuck waiting for a reply.
-    if (SyncMessageState::singleton().processIncomingMessage(*this, message))
-        return;
-    // Check if we're waiting for this message.
+    // Check if we're waiting for this message, or if we need to interrupt waiting due to an incoming sync message.
         std::lock_guard<Lock> lock(m_waitForMessageMutex);
@@ -743,10 +740,18 @@ void Connection::processIncomingMessage(std::unique_ptr<Decoder> message)
             if (m_waitingForMessage->waitForOptions.contains(WaitForOption::InterruptWaitingIfSyncMessageArrives) && message->isSyncMessage()) {
                 m_waitingForMessage->messageWaitingInterrupted = true;
+                enqueueIncomingMessage(WTFMove(message));
+                return;
+    // Check if this is a sync message or if it's a message that should be dispatched even when waiting for
+    // a sync reply. If it is, and we're waiting for a sync reply this message needs to be dispatched.
+    // If we don't we'll end up with a deadlock where both sync message senders are stuck waiting for a reply.
+    if (SyncMessageState::singleton().processIncomingMessage(*this, message))
+        return;