<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
"http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">
<html xmlns="http://www.w3.org/1999/xhtml">
<head><meta http-equiv="content-type" content="text/html; charset=utf-8" />
<title>[242682] trunk</title>
</head>
<body>

<style type="text/css"><!--
#msg dl.meta { border: 1px #006 solid; background: #369; padding: 6px; color: #fff; }
#msg dl.meta dt { float: left; width: 6em; font-weight: bold; }
#msg dt:after { content:':';}
#msg dl, #msg dt, #msg ul, #msg li, #header, #footer, #logmsg { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt;  }
#msg dl a { font-weight: bold}
#msg dl a:link    { color:#fc3; }
#msg dl a:active  { color:#ff0; }
#msg dl a:visited { color:#cc6; }
h3 { font-family: verdana,arial,helvetica,sans-serif; font-size: 10pt; font-weight: bold; }
#msg pre { overflow: auto; background: #ffc; border: 1px #fa0 solid; padding: 6px; }
#logmsg { background: #ffc; border: 1px #fa0 solid; padding: 1em 1em 0 1em; }
#logmsg p, #logmsg pre, #logmsg blockquote { margin: 0 0 1em 0; }
#logmsg p, #logmsg li, #logmsg dt, #logmsg dd { line-height: 14pt; }
#logmsg h1, #logmsg h2, #logmsg h3, #logmsg h4, #logmsg h5, #logmsg h6 { margin: .5em 0; }
#logmsg h1:first-child, #logmsg h2:first-child, #logmsg h3:first-child, #logmsg h4:first-child, #logmsg h5:first-child, #logmsg h6:first-child { margin-top: 0; }
#logmsg ul, #logmsg ol { padding: 0; list-style-position: inside; margin: 0 0 0 1em; }
#logmsg ul { text-indent: -1em; padding-left: 1em; }#logmsg ol { text-indent: -1.5em; padding-left: 1.5em; }
#logmsg > ul, #logmsg > ol { margin: 0 0 1em 0; }
#logmsg pre { background: #eee; padding: 1em; }
#logmsg blockquote { border: 1px solid #fa0; border-left-width: 10px; padding: 1em 1em 0 1em; background: white;}
#logmsg dl { margin: 0; }
#logmsg dt { font-weight: bold; }
#logmsg dd { margin: 0; padding: 0 0 0.5em 0; }
#logmsg dd:before { content:'\00bb';}
#logmsg table { border-spacing: 0px; border-collapse: collapse; border-top: 4px solid #fa0; border-bottom: 1px solid #fa0; background: #fff; }
#logmsg table th { text-align: left; font-weight: normal; padding: 0.2em 0.5em; border-top: 1px dotted #fa0; }
#logmsg table td { text-align: right; border-top: 1px dotted #fa0; padding: 0.2em 0.5em; }
#logmsg table thead th { text-align: center; border-bottom: 1px solid #fa0; }
#logmsg table th.Corner { text-align: left; }
#logmsg hr { border: none 0; border-top: 2px dashed #fa0; height: 1px; }
#header, #footer { color: #fff; background: #636; border: 1px #300 solid; padding: 6px; }
#patch { width: 100%; }
#patch h4 {font-family: verdana,arial,helvetica,sans-serif;font-size:10pt;padding:8px;background:#369;color:#fff;margin:0;}
#patch .propset h4, #patch .binary h4 {margin:0;}
#patch pre {padding:0;line-height:1.2em;margin:0;}
#patch .diff {width:100%;background:#eee;padding: 0 0 10px 0;overflow:auto;}
#patch .propset .diff, #patch .binary .diff  {padding:10px 0;}
#patch span {display:block;padding:0 10px;}
#patch .modfile, #patch .addfile, #patch .delfile, #patch .propset, #patch .binary, #patch .copfile {border:1px solid #ccc;margin:10px 0;}
#patch ins {background:#dfd;text-decoration:none;display:block;padding:0 10px;}
#patch del {background:#fdd;text-decoration:none;display:block;padding:0 10px;}
#patch .lines, .info {color:#888;background:#fff;}
--></style>
<div id="msg">
<dl class="meta">
<dt>Revision</dt> <dd><a href="http://trac.webkit.org/projects/webkit/changeset/242682">242682</a></dd>
<dt>Author</dt> <dd>wenson_hsieh@apple.com</dd>
<dt>Date</dt> <dd>2019-03-09 21:43:36 -0800 (Sat, 09 Mar 2019)</dd>
</dl>

<h3>Log Message</h3>
<pre>REGRESSION (<a href="http://trac.webkit.org/projects/webkit/changeset/242551">r242551</a>): Sporadic hangs when tapping to change selection on iOS
https://bugs.webkit.org/show_bug.cgi?id=195475
<rdar://problem/48721153>

Reviewed by Chris Dumez.

Source/WebKit:

<a href="http://trac.webkit.org/projects/webkit/changeset/242551">r242551</a> 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):

LayoutTests:

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.</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkLayoutTestsChangeLog">trunk/LayoutTests/ChangeLog</a></li>
<li><a href="#trunkSourceWebKitChangeLog">trunk/Source/WebKit/ChangeLog</a></li>
<li><a href="#trunkSourceWebKitPlatformIPCConnectioncpp">trunk/Source/WebKit/Platform/IPC/Connection.cpp</a></li>
</ul>

<h3>Added Paths</h3>
<ul>
<li><a href="#trunkLayoutTestseditingselectionioschangeselectionbytappingexpectedtxt">trunk/LayoutTests/editing/selection/ios/change-selection-by-tapping-expected.txt</a></li>
<li><a href="#trunkLayoutTestseditingselectionioschangeselectionbytappinghtml">trunk/LayoutTests/editing/selection/ios/change-selection-by-tapping.html</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkLayoutTestsChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/LayoutTests/ChangeLog (242681 => 242682)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/ChangeLog      2019-03-10 05:29:42 UTC (rev 242681)
+++ trunk/LayoutTests/ChangeLog 2019-03-10 05:43:36 UTC (rev 242682)
</span><span class="lines">@@ -1,3 +1,17 @@
</span><ins>+2019-03-09  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION (r242551): Sporadic hangs when tapping to change selection on iOS
+        https://bugs.webkit.org/show_bug.cgi?id=195475
+        <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.
+
</ins><span class="cx"> 2019-03-09  Zalan Bujtas  <zalan@apple.com>
</span><span class="cx"> 
</span><span class="cx">         [ContentChangeObserver] Click event fires immediately on hover menu at seriouseats.com
</span></span></pre></div>
<a id="trunkLayoutTestseditingselectionioschangeselectionbytappingexpectedtxt"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/editing/selection/ios/change-selection-by-tapping-expected.txt (0 => 242682)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/editing/selection/ios/change-selection-by-tapping-expected.txt                         (rev 0)
+++ trunk/LayoutTests/editing/selection/ios/change-selection-by-tapping-expected.txt    2019-03-10 05:43:36 UTC (rev 242682)
</span><span class="lines">@@ -0,0 +1,11 @@
</span><ins>+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
+
+TEST COMPLETE
+
</ins></span></pre></div>
<a id="trunkLayoutTestseditingselectionioschangeselectionbytappinghtml"></a>
<div class="addfile"><h4>Added: trunk/LayoutTests/editing/selection/ios/change-selection-by-tapping.html (0 => 242682)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/LayoutTests/editing/selection/ios/change-selection-by-tapping.html                         (rev 0)
+++ trunk/LayoutTests/editing/selection/ios/change-selection-by-tapping.html    2019-03-10 05:43:36 UTC (rev 242682)
</span><span class="lines">@@ -0,0 +1,56 @@
</span><ins>+<!DOCTYPE html> <!-- webkit-test-runner [ useFlexibleViewport=true ignoreSynchronousMessagingTimeouts=true ] -->
+<html>
+<head>
+<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">
+<style>
+body, html {
+    width: 100%;
+    height: 100%;
+    margin: 0;
+}
+
+#editor {
+    width: 300px;
+    height: 320px;
+    font-size: 18px;
+}
+</style>
+<script>
+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();
+});
+</script>
+</head>
+<body>
+<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>
+</body>
+</html>
</ins></span></pre></div>
<a id="trunkSourceWebKitChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/ChangeLog (242681 => 242682)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit/ChangeLog    2019-03-10 05:29:42 UTC (rev 242681)
+++ trunk/Source/WebKit/ChangeLog       2019-03-10 05:43:36 UTC (rev 242682)
</span><span class="lines">@@ -1,3 +1,59 @@
</span><ins>+2019-03-09  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        REGRESSION (r242551): Sporadic hangs when tapping to change selection on iOS
+        https://bugs.webkit.org/show_bug.cgi?id=195475
+        <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):
+
</ins><span class="cx"> 2019-03-09  Andy Estes  <aestes@apple.com>
</span><span class="cx"> 
</span><span class="cx">         [Apple Pay] CanMakePaymentsWithActiveCard and OpenPaymentSetup should be async messages
</span></span></pre></div>
<a id="trunkSourceWebKitPlatformIPCConnectioncpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/Platform/IPC/Connection.cpp (242681 => 242682)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit/Platform/IPC/Connection.cpp  2019-03-10 05:29:42 UTC (rev 242681)
+++ trunk/Source/WebKit/Platform/IPC/Connection.cpp     2019-03-10 05:43:36 UTC (rev 242682)
</span><span class="lines">@@ -514,6 +514,9 @@
</span><span class="cx"> 
</span><span class="cx">     // Now wait for it to be set.
</span><span class="cx">     while (true) {
</span><ins>+        // Handle any messages that are blocked on a response from us.
+        SyncMessageState::singleton().dispatchMessages(nullptr);
+
</ins><span class="cx">         std::unique_lock<Lock> lock(m_waitForMessageMutex);
</span><span class="cx"> 
</span><span class="cx">         if (m_waitingForMessage->decoder) {
</span><span class="lines">@@ -722,13 +725,7 @@
</span><span class="cx">         m_incomingSyncMessageCallbacks.clear();
</span><span class="cx">     }
</span><span class="cx"> 
</span><del>-    // 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.
</del><ins>+    // Check if we're waiting for this message, or if we need to interrupt waiting due to an incoming sync message.
</ins><span class="cx">     {
</span><span class="cx">         std::lock_guard<Lock> lock(m_waitForMessageMutex);
</span><span class="cx"> 
</span><span class="lines">@@ -743,10 +740,18 @@
</span><span class="cx">             if (m_waitingForMessage->waitForOptions.contains(WaitForOption::InterruptWaitingIfSyncMessageArrives) && message->isSyncMessage()) {
</span><span class="cx">                 m_waitingForMessage->messageWaitingInterrupted = true;
</span><span class="cx">                 m_waitForMessageCondition.notifyOne();
</span><ins>+                enqueueIncomingMessage(WTFMove(message));
+                return;
</ins><span class="cx">             }
</span><span class="cx">         }
</span><span class="cx">     }
</span><span class="cx"> 
</span><ins>+    // 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;
+
</ins><span class="cx">     enqueueIncomingMessage(WTFMove(message));
</span><span class="cx"> }
</span><span class="cx"> 
</span></span></pre>
</div>
</div>

</body>
</html>