[webkit-changes] [244243] trunk/Source/WebKit

Sat, 13 Apr 2019 21:23:45 -0700

Title: [244243] trunk/Source/WebKit
Revision
244243
Author
[email protected]
Date
2019-04-13 21:22:34 -0700 (Sat, 13 Apr 2019)

Log Message

[ Mac Debug ] TestWebKitAPI.ProcessSwap.ReuseSuspendedProcessForRegularNavigationRetainBundlePage is a flaky crash
https://bugs.webkit.org/show_bug.cgi?id=196548
<rdar://problem/49567254>

Reviewed by Darin Adler.

Update ProvisionalPageProxy methods to more consistently ignore unexpected IPC from the process. Previously,
some of the methods were doing this, but some other like didFailProvisionalLoadForFrame() weren't and this
was leading to this flaky crash. The issue is that if we do the load in an existing process that was recently
doing, there may be leftover IPC for the same pageID and this IPC gets received by the ProvisionalPageProxy
even though it is from a previous navigation. For this reason, the ProvisionalPageProxy should ignore all
incoming IPC that is not for its associated navigation.

* UIProcess/ProvisionalPageProxy.cpp:
(WebKit::ProvisionalPageProxy::didPerformClientRedirect):
(WebKit::ProvisionalPageProxy::didStartProvisionalLoadForFrame):
(WebKit::ProvisionalPageProxy::didFailProvisionalLoadForFrame):
(WebKit::ProvisionalPageProxy::didCommitLoadForFrame):
(WebKit::ProvisionalPageProxy::didNavigateWithNavigationData):
(WebKit::ProvisionalPageProxy::didChangeProvisionalURLForFrame):
(WebKit::ProvisionalPageProxy::decidePolicyForNavigationActionAsync):
(WebKit::ProvisionalPageProxy::decidePolicyForResponse):
(WebKit::ProvisionalPageProxy::didPerformServerRedirect):
(WebKit::ProvisionalPageProxy::didReceiveServerRedirectForProvisionalLoadForFrame):
(WebKit::ProvisionalPageProxy::decidePolicyForNavigationActionSync):

Modified Paths

Diff

Modified: trunk/Source/WebKit/ChangeLog (244242 => 244243)


--- trunk/Source/WebKit/ChangeLog	2019-04-13 19:44:47 UTC (rev 244242)
+++ trunk/Source/WebKit/ChangeLog	2019-04-14 04:22:34 UTC (rev 244243)
@@ -1,3 +1,31 @@
+2019-04-13  Chris Dumez  <[email protected]>
+
+        [ Mac Debug ] TestWebKitAPI.ProcessSwap.ReuseSuspendedProcessForRegularNavigationRetainBundlePage is a flaky crash
+        https://bugs.webkit.org/show_bug.cgi?id=196548
+        <rdar://problem/49567254>
+
+        Reviewed by Darin Adler.
+
+        Update ProvisionalPageProxy methods to more consistently ignore unexpected IPC from the process. Previously,
+        some of the methods were doing this, but some other like didFailProvisionalLoadForFrame() weren't and this
+        was leading to this flaky crash. The issue is that if we do the load in an existing process that was recently
+        doing, there may be leftover IPC for the same pageID and this IPC gets received by the ProvisionalPageProxy
+        even though it is from a previous navigation. For this reason, the ProvisionalPageProxy should ignore all
+        incoming IPC that is not for its associated navigation.
+
+        * UIProcess/ProvisionalPageProxy.cpp:
+        (WebKit::ProvisionalPageProxy::didPerformClientRedirect):
+        (WebKit::ProvisionalPageProxy::didStartProvisionalLoadForFrame):
+        (WebKit::ProvisionalPageProxy::didFailProvisionalLoadForFrame):
+        (WebKit::ProvisionalPageProxy::didCommitLoadForFrame):
+        (WebKit::ProvisionalPageProxy::didNavigateWithNavigationData):
+        (WebKit::ProvisionalPageProxy::didChangeProvisionalURLForFrame):
+        (WebKit::ProvisionalPageProxy::decidePolicyForNavigationActionAsync):
+        (WebKit::ProvisionalPageProxy::decidePolicyForResponse):
+        (WebKit::ProvisionalPageProxy::didPerformServerRedirect):
+        (WebKit::ProvisionalPageProxy::didReceiveServerRedirectForProvisionalLoadForFrame):
+        (WebKit::ProvisionalPageProxy::decidePolicyForNavigationActionSync):
+
 2019-04-13  Wenson Hsieh  <[email protected]>
 
         Unreviewed, try to fix the internal build after r244239

Modified: trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp (244242 => 244243)


--- trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp	2019-04-13 19:44:47 UTC (rev 244242)
+++ trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp	2019-04-14 04:22:34 UTC (rev 244243)
@@ -183,6 +183,15 @@
     m_process->responsivenessTimer().start();
 }
 
+inline bool ProvisionalPageProxy::validateInput(uint64_t frameID, const Optional<uint64_t>& navigationID)
+{
+    // If the previous provisional load used an existing process, we may receive leftover IPC for a previous navigation, which we need to ignore.
+    if (!m_mainFrame || m_mainFrame->frameID() != frameID)
+        return false;
+
+    return !navigationID || *navigationID == m_navigationID;
+}
+
 void ProvisionalPageProxy::didCreateMainFrame(uint64_t frameID)
 {
     RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "didCreateMainFrame: pageID = %" PRIu64 ", frameID = %" PRIu64, m_page.pageID(), frameID);
@@ -212,13 +221,15 @@
 
 void ProvisionalPageProxy::didPerformClientRedirect(const String& sourceURLString, const String& destinationURLString, uint64_t frameID)
 {
+    if (!validateInput(frameID))
+        return;
+
     m_page.didPerformClientRedirectShared(m_process.copyRef(), sourceURLString, destinationURLString, frameID);
 }
 
 void ProvisionalPageProxy::didStartProvisionalLoadForFrame(uint64_t frameID, uint64_t navigationID, URL&& url, URL&& unreachableURL, const UserData& userData)
 {
-    // If the previous provisional load used the same process, we may receive IPC for this previous provisional's main frame that we need to ignore.
-    if (!m_mainFrame || m_mainFrame->frameID() != frameID)
+    if (!validateInput(frameID, navigationID))
         return;
 
     RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "didStartProvisionalLoadForFrame: pageID = %" PRIu64 ", frameID = %" PRIu64 ", navigationID = %" PRIu64, m_page.pageID(), frameID, navigationID);
@@ -238,7 +249,11 @@
 
 void ProvisionalPageProxy::didFailProvisionalLoadForFrame(uint64_t frameID, const WebCore::SecurityOriginData& frameSecurityOrigin, uint64_t navigationID, const String& provisionalURL, const WebCore::ResourceError& error, const UserData& userData)
 {
+    if (!validateInput(frameID, navigationID))
+        return;
+
     RELEASE_LOG_ERROR_IF_ALLOWED(ProcessSwapping, "didFailProvisionalLoadForFrame: pageID = %" PRIu64 ", frameID = %" PRIu64 ", navigationID = %" PRIu64, m_page.pageID(), frameID, navigationID);
+    ASSERT(!m_provisionalLoadURL.isNull());
     m_provisionalLoadURL = { };
 
     // Make sure the Page's main frame's expectedURL gets cleared since we updated it in didStartProvisionalLoad.
@@ -250,8 +265,7 @@
 
 void ProvisionalPageProxy::didCommitLoadForFrame(uint64_t frameID, uint64_t navigationID, const String& mimeType, bool frameHasCustomContentProvider, uint32_t frameLoadType, const WebCore::CertificateInfo& certificateInfo, bool containsPluginDocument, Optional<WebCore::HasInsecureContent> forcedHasInsecureContent, const UserData& userData)
 {
-    // If the previous provisional load used the same process, we may receive IPC for this previous provisional's main frame that we need to ignore.
-    if (!m_mainFrame || m_mainFrame->frameID() != frameID)
+    if (!validateInput(frameID, navigationID))
         return;
 
     RELEASE_LOG_IF_ALLOWED(ProcessSwapping, "didCommitLoadForFrame: pageID = %" PRIu64 ", frameID = %" PRIu64 ", navigationID = %" PRIu64, m_page.pageID(), frameID, navigationID);
@@ -264,11 +278,17 @@
 
 void ProvisionalPageProxy::didNavigateWithNavigationData(const WebNavigationDataStore& store, uint64_t frameID)
 {
+    if (!validateInput(frameID))
+        return;
+
     m_page.didNavigateWithNavigationDataShared(m_process.copyRef(), store, frameID);
 }
 
 void ProvisionalPageProxy::didChangeProvisionalURLForFrame(uint64_t frameID, uint64_t navigationID, URL&& url)
 {
+    if (!validateInput(frameID, navigationID))
+        return;
+
     m_page.didChangeProvisionalURLForFrameShared(m_process.copyRef(), frameID, navigationID, WTFMove(url));
 }
 
@@ -276,8 +296,9 @@
     uint64_t navigationID, NavigationActionData&& navigationActionData, FrameInfoData&& frameInfoData, uint64_t originatingPageID, const WebCore::ResourceRequest& originalRequest,
     WebCore::ResourceRequest&& request, IPC::FormDataReference&& requestBody, WebCore::ResourceResponse&& redirectResponse, const UserData& userData, uint64_t listenerID)
 {
-    ASSERT(m_mainFrame);
-    ASSERT(m_mainFrame->frameID() == frameID);
+    if (!validateInput(frameID, navigationID))
+        return;
+
     m_page.decidePolicyForNavigationActionAsyncShared(m_process.copyRef(), frameID, WTFMove(frameSecurityOrigin), identifier, navigationID, WTFMove(navigationActionData),
         WTFMove(frameInfoData), originatingPageID, originalRequest, WTFMove(request), WTFMove(requestBody), WTFMove(redirectResponse), userData, listenerID);
 }
@@ -285,16 +306,25 @@
 void ProvisionalPageProxy::decidePolicyForResponse(uint64_t frameID, const WebCore::SecurityOriginData& frameSecurityOrigin, WebCore::PolicyCheckIdentifier identifier,
     uint64_t navigationID, const WebCore::ResourceResponse& response, const WebCore::ResourceRequest& request, bool canShowMIMEType, const String& downloadAttribute, uint64_t listenerID, const UserData& userData)
 {
+    if (!validateInput(frameID, navigationID))
+        return;
+
     m_page.decidePolicyForResponseShared(m_process.copyRef(), frameID, frameSecurityOrigin, identifier, navigationID, response, request, canShowMIMEType, downloadAttribute, listenerID, userData);
 }
 
 void ProvisionalPageProxy::didPerformServerRedirect(const String& sourceURLString, const String& destinationURLString, uint64_t frameID)
 {
+    if (!validateInput(frameID))
+        return;
+
     m_page.didPerformServerRedirectShared(m_process.copyRef(), sourceURLString, destinationURLString, frameID);
 }
 
 void ProvisionalPageProxy::didReceiveServerRedirectForProvisionalLoadForFrame(uint64_t frameID, uint64_t navigationID, WebCore::ResourceRequest&& request, const UserData& userData)
 {
+    if (!validateInput(frameID, navigationID))
+        return;
+
     m_page.didReceiveServerRedirectForProvisionalLoadForFrameShared(m_process.copyRef(), frameID, navigationID, WTFMove(request), userData);
 }
 
@@ -313,10 +343,7 @@
     const WebCore::ResourceRequest& originalRequest, WebCore::ResourceRequest&& request, IPC::FormDataReference&& requestBody, WebCore::ResourceResponse&& redirectResponse,
     const UserData& userData, Messages::WebPageProxy::DecidePolicyForNavigationActionSync::DelayedReply&& reply)
 {
-    ASSERT(isMainFrame);
-    ASSERT(!m_mainFrame || m_mainFrame->frameID() == frameID);
-
-    if (!isMainFrame || (m_mainFrame && m_mainFrame->frameID() != frameID)) {
+    if (!isMainFrame || (m_mainFrame && m_mainFrame->frameID() != frameID) || navigationID != m_navigationID) {
         reply(identifier, WebCore::PolicyAction::Ignore, navigationID, DownloadID(), WTF::nullopt);
         return;
     }

Modified: trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.h (244242 => 244243)


--- trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.h	2019-04-13 19:44:47 UTC (rev 244242)
+++ trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.h	2019-04-14 04:22:34 UTC (rev 244243)
@@ -118,6 +118,7 @@
 #endif
 
     void initializeWebPage();
+    bool validateInput(uint64_t frameID, const Optional<uint64_t>& navigationID = WTF::nullopt);
 
     WebPageProxy& m_page;
     Ref<WebProcessProxy> m_process;

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to