Changeset 244243 in webkit

Timestamp:

Apr 13, 2019 9:22:34 PM (5 years ago)

Author:

Chris Dumez

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):

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/ProvisionalPageProxy.cpp (modified) (12 diffs)
• UIProcess/ProvisionalPageProxy.h (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-04-13  Chris Dumez  <cdumez@apple.com>
+
+        [ 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  <wenson_hsieh@apple.com>
 

trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp

@@ -184 +184 @@
 }
 
+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)
 {
@@ -213 +222 @@
 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);
 }
@@ -218 +230 @@
 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;
 
@@ -239 +250 @@
 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 = { };
 
@@ -251 +266 @@
 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;
 
@@ -265 +279 @@
 void ProvisionalPageProxy::didNavigateWithNavigationData(const WebNavigationDataStore& store, uint64_t frameID)
 {
+    if (!validateInput(frameID))
+        return;
+
     m_page.didNavigateWithNavigationDataShared(m_process.copyRef(), store, frameID);
 }
@@ -270 +287 @@
 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));
 }
@@ -277 +297 @@
     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);
@@ -286 +307 @@
     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);
 }
@@ -291 +315 @@
 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);
 }
@@ -296 +323 @@
 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);
 }
@@ -314 +344 @@
     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;

trunk/Source/WebKit/UIProcess/ProvisionalPageProxy.h

@@ -119 +119 @@
 
     void initializeWebPage();
+    bool validateInput(uint64_t frameID, const Optional<uint64_t>& navigationID = WTF::nullopt);
 
     WebPageProxy& m_page;