<!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>[244493] trunk/Source/WebKit</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/244493">244493</a></dd>
<dt>Author</dt> <dd>cdumez@apple.com</dd>
<dt>Date</dt> <dd>2019-04-21 13:14:04 -0700 (Sun, 21 Apr 2019)</dd>
</dl>

<h3>Log Message</h3>
<pre>Regression(<a href="http://trac.webkit.org/projects/webkit/changeset/243767">r243767</a>) WebFrame::m_navigationIsContinuingInAnotherProcess flag is never reset
https://bugs.webkit.org/show_bug.cgi?id=197144

Reviewed by Darin Adler.

WebFrame::m_navigationIsContinuingInAnotherProcess flag is never reset since it was introduced in
<a href="http://trac.webkit.org/projects/webkit/changeset/243767">r243767</a>. This leads to leaking Navigation objects in the UIProcess when reusing a previously
suspended process because such process will no longer send the DidDestroyNavigation IPC.

It turns out that resetting the flags causes API tests such as ProcessSwap.QuickBackForwardNavigationWithPSON
to ASSERT. This is because when the UIProcess quickly navigate back and forth without waiting for policy
decisions, we may end up getting the policy decision for a particular navigation *after* we've sent the
DidDestroyNavigation.

As a result, this patch reverts <a href="http://trac.webkit.org/projects/webkit/changeset/243767">r243767</a> and fixes in the assertion in http/tests/adClickAttribution/store-ad-click-attribution.html
another way. We initially assumed that the logic in WebPageProxy::didDestroyNavigation() was failing to
ignore the DidDestroyNavigation from the previous process after a swap due to a race, maybe because it was
sometimes received too late and m_provisionalPage was already cleared. However, this would not make sense
since the test is crashing consistently and the page would no longer be able to receive IPC from the
previous process *after* we've committed the provisional process/page.

The real issue was that the DidDestroyNavigation IPC was received *before* we could construct the
provisional page, which is why the logic in WebPageProxy::didDestroyNavigation() was failing to ignore
the bad IPC. In WebPageProxy::receivedNavigationPolicyDecision(), we were calling receivedPolicyDecision()
(which would send the DidReceivePolicyDecision to the previous WebProcess) and then continueNavigationInNewProcess()
in order to construct the provisional page. I personally did not expect we could receive IPC between the
calls to receivedNavigationPolicyDecision() and receivedPolicyDecision(), since we are not yielding and since
the DidReceivePolicyDecision IPC is asynchronous. However, this is exactly what was happening in the context
of this test. The reason is that the DidReceivePolicyDecision IPC was getting wrapped in a synchronous message
and sent as synchronous message due to the Connection::m_inDispatchMessageMarkedToUseFullySynchronousModeForTesting
flag which seems to get set in the test due to some EventSender IPC. I believe this is because the test uses
EventSender to do a click on a link which triggers the navigation.

To address the issue, I now call receivedNavigationPolicyDecision() *after* continueNavigationInNewProcess()
to make sure that we always start the provisional load in the new process before we tell the previous process
to stop loading. This way, there is no way we get IPC from the previous process about the current navigation
before we have a provisional page.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::receivedNavigationPolicyDecision):
(WebKit::WebPageProxy::didDestroyNavigation):
* WebProcess/WebPage/WebFrame.cpp:
(WebKit::WebFrame::didReceivePolicyDecision):
(WebKit::WebFrame::documentLoaderDetached):
* WebProcess/WebPage/WebFrame.h:</pre>

<h3>Modified Paths</h3>
<ul>
<li><a href="#trunkSourceWebKitChangeLog">trunk/Source/WebKit/ChangeLog</a></li>
<li><a href="#trunkSourceWebKitUIProcessWebPageProxycpp">trunk/Source/WebKit/UIProcess/WebPageProxy.cpp</a></li>
<li><a href="#trunkSourceWebKitWebProcessWebPageWebFramecpp">trunk/Source/WebKit/WebProcess/WebPage/WebFrame.cpp</a></li>
<li><a href="#trunkSourceWebKitWebProcessWebPageWebFrameh">trunk/Source/WebKit/WebProcess/WebPage/WebFrame.h</a></li>
</ul>

</div>
<div id="patch">
<h3>Diff</h3>
<a id="trunkSourceWebKitChangeLog"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/ChangeLog (244492 => 244493)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit/ChangeLog    2019-04-21 19:48:46 UTC (rev 244492)
+++ trunk/Source/WebKit/ChangeLog       2019-04-21 20:14:04 UTC (rev 244493)
</span><span class="lines">@@ -1,3 +1,51 @@
</span><ins>+2019-04-21  Chris Dumez  <cdumez@apple.com>
+
+        Regression(r243767) WebFrame::m_navigationIsContinuingInAnotherProcess flag is never reset
+        https://bugs.webkit.org/show_bug.cgi?id=197144
+
+        Reviewed by Darin Adler.
+
+        WebFrame::m_navigationIsContinuingInAnotherProcess flag is never reset since it was introduced in
+        r243767. This leads to leaking Navigation objects in the UIProcess when reusing a previously
+        suspended process because such process will no longer send the DidDestroyNavigation IPC.
+
+        It turns out that resetting the flags causes API tests such as ProcessSwap.QuickBackForwardNavigationWithPSON
+        to ASSERT. This is because when the UIProcess quickly navigate back and forth without waiting for policy
+        decisions, we may end up getting the policy decision for a particular navigation *after* we've sent the
+        DidDestroyNavigation.
+
+        As a result, this patch reverts r243767 and fixes in the assertion in http/tests/adClickAttribution/store-ad-click-attribution.html
+        another way. We initially assumed that the logic in WebPageProxy::didDestroyNavigation() was failing to
+        ignore the DidDestroyNavigation from the previous process after a swap due to a race, maybe because it was
+        sometimes received too late and m_provisionalPage was already cleared. However, this would not make sense
+        since the test is crashing consistently and the page would no longer be able to receive IPC from the
+        previous process *after* we've committed the provisional process/page.
+
+        The real issue was that the DidDestroyNavigation IPC was received *before* we could construct the
+        provisional page, which is why the logic in WebPageProxy::didDestroyNavigation() was failing to ignore
+        the bad IPC. In WebPageProxy::receivedNavigationPolicyDecision(), we were calling receivedPolicyDecision()
+        (which would send the DidReceivePolicyDecision to the previous WebProcess) and then continueNavigationInNewProcess()
+        in order to construct the provisional page. I personally did not expect we could receive IPC between the
+        calls to receivedNavigationPolicyDecision() and receivedPolicyDecision(), since we are not yielding and since
+        the DidReceivePolicyDecision IPC is asynchronous. However, this is exactly what was happening in the context
+        of this test. The reason is that the DidReceivePolicyDecision IPC was getting wrapped in a synchronous message
+        and sent as synchronous message due to the Connection::m_inDispatchMessageMarkedToUseFullySynchronousModeForTesting
+        flag which seems to get set in the test due to some EventSender IPC. I believe this is because the test uses
+        EventSender to do a click on a link which triggers the navigation.
+
+        To address the issue, I now call receivedNavigationPolicyDecision() *after* continueNavigationInNewProcess()
+        to make sure that we always start the provisional load in the new process before we tell the previous process
+        to stop loading. This way, there is no way we get IPC from the previous process about the current navigation
+        before we have a provisional page.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::receivedNavigationPolicyDecision):
+        (WebKit::WebPageProxy::didDestroyNavigation):
+        * WebProcess/WebPage/WebFrame.cpp:
+        (WebKit::WebFrame::didReceivePolicyDecision):
+        (WebKit::WebFrame::documentLoaderDetached):
+        * WebProcess/WebPage/WebFrame.h:
+
</ins><span class="cx"> 2019-04-20  Chris Dumez  <cdumez@apple.com>
</span><span class="cx"> 
</span><span class="cx">         Unreviewed, fix iOS build with recent SDKs.
</span></span></pre></div>
<a id="trunkSourceWebKitUIProcessWebPageProxycpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (244492 => 244493)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp   2019-04-21 19:48:46 UTC (rev 244492)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp      2019-04-21 20:14:04 UTC (rev 244493)
</span><span class="lines">@@ -2789,21 +2789,20 @@
</span><span class="cx">         } else
</span><span class="cx">             RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "decidePolicyForNavigationAction: keep using process %i for navigation, reason: %{public}s", processIdentifier(), reason.utf8().data());
</span><span class="cx"> 
</span><del>-        receivedPolicyDecision(policyAction, navigation.ptr(), shouldProcessSwap ? WTF::nullopt : WTFMove(data), WTFMove(sender), shouldProcessSwap ? WillContinueLoadInNewProcess::Yes : WillContinueLoadInNewProcess::No);
</del><ins>+        if (shouldProcessSwap) {
+            // FIXME: Architecturally we do not currently support multiple WebPage's with the same ID in a given WebProcess.
+            // In the case where the destination WebProcess has a SuspendedPageProxy for this WebPage, we should have thrown
+            // it away to support WebProcess re-use.
+            ASSERT(destinationSuspendedPage || !process().processPool().hasSuspendedPageFor(processForNavigation, *this));
</ins><span class="cx"> 
</span><del>-        if (!shouldProcessSwap)
-            return;
</del><ins>+            auto suspendedPage = destinationSuspendedPage ? process().processPool().takeSuspendedPage(*destinationSuspendedPage) : nullptr;
+            if (suspendedPage && suspendedPage->failedToSuspend())
+                suspendedPage = nullptr;
</ins><span class="cx"> 
</span><del>-        // FIXME: Architecturally we do not currently support multiple WebPage's with the same ID in a given WebProcess.
-        // In the case where the destination WebProcess has a SuspendedPageProxy for this WebPage, we should have thrown
-        // it away to support WebProcess re-use.
-        ASSERT(destinationSuspendedPage || !process().processPool().hasSuspendedPageFor(processForNavigation, *this));
</del><ins>+            continueNavigationInNewProcess(navigation, WTFMove(suspendedPage), WTFMove(processForNavigation), processSwapRequestedByClient, WTFMove(data));
+        }
</ins><span class="cx"> 
</span><del>-        auto suspendedPage = destinationSuspendedPage ? process().processPool().takeSuspendedPage(*destinationSuspendedPage) : nullptr;
-        if (suspendedPage && suspendedPage->failedToSuspend())
-            suspendedPage = nullptr;
-
-        continueNavigationInNewProcess(navigation, WTFMove(suspendedPage), WTFMove(processForNavigation), processSwapRequestedByClient, WTFMove(data));
</del><ins>+        receivedPolicyDecision(policyAction, navigation.ptr(), shouldProcessSwap ? WTF::nullopt : WTFMove(data), WTFMove(sender), shouldProcessSwap ? WillContinueLoadInNewProcess::Yes : WillContinueLoadInNewProcess::No);
</ins><span class="cx">     });
</span><span class="cx"> }
</span><span class="cx"> 
</span><span class="lines">@@ -3868,6 +3867,10 @@
</span><span class="cx"> {
</span><span class="cx">     PageClientProtector protector(pageClient());
</span><span class="cx"> 
</span><ins>+    // On process-swap, the previous process tries to destroy the navigation but the provisional process is actually taking over the navigation.
+    if (m_provisionalPage && m_provisionalPage->navigationID() == navigationID)
+        return;
+
</ins><span class="cx">     // FIXME: Message check the navigationID.
</span><span class="cx">     m_navigationState->didDestroyNavigation(navigationID);
</span><span class="cx"> }
</span></span></pre></div>
<a id="trunkSourceWebKitWebProcessWebPageWebFramecpp"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/WebProcess/WebPage/WebFrame.cpp (244492 => 244493)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit/WebProcess/WebPage/WebFrame.cpp      2019-04-21 19:48:46 UTC (rev 244492)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebFrame.cpp 2019-04-21 20:14:04 UTC (rev 244493)
</span><span class="lines">@@ -278,9 +278,6 @@
</span><span class="cx">             documentLoader->setNavigationID(navigationID);
</span><span class="cx">     }
</span><span class="cx"> 
</span><del>-    if (action == PolicyAction::StopAllLoads)
-        m_navigationIsContinuingInAnotherProcess = true;
-
</del><span class="cx">     function(action, identifier);
</span><span class="cx"> }
</span><span class="cx"> 
</span><span class="lines">@@ -815,8 +812,6 @@
</span><span class="cx"> 
</span><span class="cx"> void WebFrame::documentLoaderDetached(uint64_t navigationID)
</span><span class="cx"> {
</span><del>-    if (m_navigationIsContinuingInAnotherProcess)
-        return;
</del><span class="cx">     if (auto* page = this->page())
</span><span class="cx">         page->send(Messages::WebPageProxy::DidDestroyNavigation(navigationID));
</span><span class="cx"> }
</span></span></pre></div>
<a id="trunkSourceWebKitWebProcessWebPageWebFrameh"></a>
<div class="modfile"><h4>Modified: trunk/Source/WebKit/WebProcess/WebPage/WebFrame.h (244492 => 244493)</h4>
<pre class="diff"><span>
<span class="info">--- trunk/Source/WebKit/WebProcess/WebPage/WebFrame.h        2019-04-21 19:48:46 UTC (rev 244492)
+++ trunk/Source/WebKit/WebProcess/WebPage/WebFrame.h   2019-04-21 20:14:04 UTC (rev 244493)
</span><span class="lines">@@ -188,7 +188,6 @@
</span><span class="cx">     LoadListener* m_loadListener { nullptr };
</span><span class="cx">     
</span><span class="cx">     uint64_t m_frameID { 0 };
</span><del>-    bool m_navigationIsContinuingInAnotherProcess { false };
</del><span class="cx"> 
</span><span class="cx"> #if PLATFORM(IOS_FAMILY)
</span><span class="cx">     uint64_t m_firstLayerTreeTransactionIDAfterDidCommitLoad { 0 };
</span></span></pre>
</div>
</div>

</body>
</html>