Attachment 362508 Details for Bug 194857 – Patch

[patch] Patch

bug-194857-20190220100654.patch (text/plain), 13.32 KB, created by Chris Dumez on 2019-02-20 10:06:55 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-02-20 10:06:55 PST

Size: 13.32 KB

patch

obsolete

>Subversion Revision: 241816
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 242cea16984340e3b616856d7e566a3c8fa4eb53..a3153146320b11f8d3240361810161d9d0cadcd4 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,26 @@
>+2019-02-20  Chris Dumez  <cdumez@apple.com>
>+
>+        Regression(PSON) Crash under WebKit::WebPageProxy::decidePolicyForNavigationActionSync
>+        https://bugs.webkit.org/show_bug.cgi?id=194857
>+        <rdar://problem/47759323>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The ProvisionalPageProxy was blindly forwarding the DecidePolicyForNavigationActionSync
>+        synchronous IPC to the WebPageProxy, without passing it the process the IPC came from.
>+        As a result, WebPageProxy::decidePolicyForNavigationActionSync() would try to look up
>+        a WebFrameProxy using the provided frameID from the wrong process and we would end up
>+        hitting a RELEASE_ASSERT().
>+
>+        * UIProcess/ProvisionalPageProxy.cpp:
>+        (WebKit::ProvisionalPageProxy::decidePolicyForNavigationActionSync):
>+        (WebKit::ProvisionalPageProxy::didReceiveSyncMessage):
>+        * UIProcess/ProvisionalPageProxy.h:
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::WebPageProxy::decidePolicyForNavigationActionSync):
>+        (WebKit::WebPageProxy::decidePolicyForNavigationActionSyncShared):
>+        * UIProcess/WebPageProxy.h:
>+
> 2019-02-20  Carlos Garcia Campos  <cgarcia@igalia.com>
> 
>         [WPE] Send client host fd and library name as web process creation parameters
>diff --git a/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp b/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp
>index 1f8d4a35fca0d74e6bb86625b2b082aa26a217af..eb299a87b7555f03cc8e25dd1992c95a0f4739f2 100644
>--- a/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/ProvisionalPageProxy.cpp
>@@ -304,6 +304,29 @@ void ProvisionalPageProxy::backForwardGoToItem(const WebCore::BackForwardItemIde
>     m_page.backForwardGoToItemShared(m_process.copyRef(), identifier, handle);
> }
> 
>+void ProvisionalPageProxy::decidePolicyForNavigationActionSync(uint64_t frameID, bool isMainFrame, WebCore::SecurityOriginData&& frameSecurityOrigin, WebCore::PolicyCheckIdentifier identifier,
>+    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, Messages::WebPageProxy::DecidePolicyForNavigationActionSync::DelayedReply&& reply)
>+{
>+    ASSERT(isMainFrame);
>+    ASSERT(!m_mainFrame || m_mainFrame->frameID() == frameID);
>+
>+    if (!isMainFrame || (m_mainFrame && m_mainFrame->frameID() != frameID)) {
>+        reply(identifier, WebCore::PolicyAction::Ignore, navigationID, DownloadID(), WTF::nullopt);
>+        return;
>+    }
>+
>+    if (!m_mainFrame) {
>+        // This synchronous IPC message was processed before the asynchronous DidCreateMainFrame one so we do not know about this frameID yet.
>+        didCreateMainFrame(frameID);
>+    }
>+    ASSERT(m_mainFrame);
>+
>+    m_page.decidePolicyForNavigationActionSyncShared(m_process.copyRef(), frameID, isMainFrame, WTFMove(frameSecurityOrigin), identifier, navigationID, WTFMove(navigationActionData),
>+        WTFMove(frameInfoData), originatingPageID, originalRequest, WTFMove(request), WTFMove(requestBody), WTFMove(redirectResponse), userData, WTFMove(reply));
>+}
>+
> #if PLATFORM(COCOA)
> void ProvisionalPageProxy::registerWebProcessAccessibilityToken(const IPC::DataReference& data)
> {
>@@ -404,6 +427,11 @@ void ProvisionalPageProxy::didReceiveSyncMessage(IPC::Connection& connection, IP
>         return;
>     }
> 
>+    if (decoder.messageName() == Messages::WebPageProxy::DecidePolicyForNavigationActionSync::name()) {
>+        IPC::handleMessageDelayed<Messages::WebPageProxy::DecidePolicyForNavigationActionSync>(connection, decoder, replyEncoder, this, &ProvisionalPageProxy::decidePolicyForNavigationActionSync);
>+        return;
>+    }
>+
>     m_page.didReceiveSyncMessage(connection, decoder, replyEncoder);
> }
> 
>diff --git a/Source/WebKit/UIProcess/ProvisionalPageProxy.h b/Source/WebKit/UIProcess/ProvisionalPageProxy.h
>index 60d391083bffc678bf9945d2bab9443e8de4aa8f..c42fe6df4bad88c09bd484ee63b63f044edc4590 100644
>--- a/Source/WebKit/UIProcess/ProvisionalPageProxy.h
>+++ b/Source/WebKit/UIProcess/ProvisionalPageProxy.h
>@@ -102,6 +102,9 @@ private:
>     void didFailProvisionalLoadForFrame(uint64_t frameID, const WebCore::SecurityOriginData& frameSecurityOrigin, uint64_t navigationID, const String& provisionalURL, const WebCore::ResourceError&, const UserData&);
>     void startURLSchemeTask(URLSchemeTaskParameters&&);
>     void backForwardGoToItem(const WebCore::BackForwardItemIdentifier&, SandboxExtension::Handle&);
>+    void decidePolicyForNavigationActionSync(uint64_t frameID, bool isMainFrame, WebCore::SecurityOriginData&&, WebCore::PolicyCheckIdentifier, uint64_t navigationID, NavigationActionData&&,
>+        FrameInfoData&&, uint64_t originatingPageID, const WebCore::ResourceRequest& originalRequest, WebCore::ResourceRequest&&, IPC::FormDataReference&& requestBody,
>+        WebCore::ResourceResponse&& redirectResponse, const UserData&, Messages::WebPageProxy::DecidePolicyForNavigationActionSync::DelayedReply&&);
> #if PLATFORM(COCOA)
>     void registerWebProcessAccessibilityToken(const IPC::DataReference&);
> #endif
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index 52b4c8e227a3bd3d7e457bf6c5dcd0e36b187bda..fc19ce0c03b0d41bd9be8b53c4f03c24678813a6 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -4621,8 +4621,6 @@ void WebPageProxy::decidePolicyForNavigationActionSync(uint64_t frameID, bool is
>     const WebCore::ResourceRequest& originalRequest, WebCore::ResourceRequest&& request, IPC::FormDataReference&& requestBody, WebCore::ResourceResponse&& redirectResponse,
>     const UserData& userData, Messages::WebPageProxy::DecidePolicyForNavigationActionSync::DelayedReply&& reply)
> {
>-    auto sender = PolicyDecisionSender::create(identifier, WTFMove(reply));
>-
>     auto* frame = m_process->webFrame(frameID);
>     if (!frame) {
>         // This synchronous IPC message was processed before the asynchronous DidCreateMainFrame / DidCreateSubframe one so we do not know about this frameID yet.
>@@ -4630,12 +4628,23 @@ void WebPageProxy::decidePolicyForNavigationActionSync(uint64_t frameID, bool is
>             didCreateMainFrame(frameID);
>         else
>             didCreateSubframe(frameID);
>-
>-        frame = m_process->webFrame(frameID);
>-        RELEASE_ASSERT(frame);
>     }
> 
>-    decidePolicyForNavigationAction(m_process.copyRef(), *frame, WTFMove(frameSecurityOrigin), navigationID, WTFMove(navigationActionData), WTFMove(frameInfoData),
>+    decidePolicyForNavigationActionSyncShared(m_process.copyRef(), frameID, isMainFrame, WTFMove(frameSecurityOrigin), identifier, navigationID, WTFMove(navigationActionData),
>+        WTFMove(frameInfoData), originatingPageID, originalRequest, WTFMove(request), WTFMove(requestBody), WTFMove(redirectResponse), userData, WTFMove(reply));
>+}
>+
>+void WebPageProxy::decidePolicyForNavigationActionSyncShared(Ref<WebProcessProxy>&& process, uint64_t frameID, bool isMainFrame, WebCore::SecurityOriginData&& frameSecurityOrigin, PolicyCheckIdentifier identifier,
>+    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, Messages::WebPageProxy::DecidePolicyForNavigationActionSync::DelayedReply&& reply)
>+{
>+    auto sender = PolicyDecisionSender::create(identifier, WTFMove(reply));
>+
>+    auto* frame = process->webFrame(frameID);
>+    MESSAGE_CHECK(process, frame);
>+
>+    decidePolicyForNavigationAction(WTFMove(process), *frame, WTFMove(frameSecurityOrigin), navigationID, WTFMove(navigationActionData), WTFMove(frameInfoData),
>         originatingPageID, originalRequest, WTFMove(request), WTFMove(requestBody), WTFMove(redirectResponse), userData, sender.copyRef());
> 
>     // If the client did not respond synchronously, proceed with the load.
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.h b/Source/WebKit/UIProcess/WebPageProxy.h
>index b2b0e85d2e0d159b08d09b33817c8fc126c0d84f..df67eec428592422e0bf02b2d84a2f1d7d168a16 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.h
>+++ b/Source/WebKit/UIProcess/WebPageProxy.h
>@@ -1455,6 +1455,9 @@ public:
>     void loadDataWithNavigationShared(Ref<WebProcessProxy>&&, API::Navigation&, const IPC::DataReference&, const String& MIMEType, const String& encoding, const String& baseURL, API::Object* userData, WebCore::ShouldTreatAsContinuingLoad, Optional<WebsitePoliciesData>&& = WTF::nullopt);
>     void loadRequestWithNavigationShared(Ref<WebProcessProxy>&&, API::Navigation&, WebCore::ResourceRequest&&, WebCore::ShouldOpenExternalURLsPolicy, API::Object* userData, WebCore::ShouldTreatAsContinuingLoad, Optional<WebsitePoliciesData>&& = WTF::nullopt);
>     void backForwardGoToItemShared(Ref<WebProcessProxy>&&, const WebCore::BackForwardItemIdentifier&, SandboxExtension::Handle&);
>+    void decidePolicyForNavigationActionSyncShared(Ref<WebProcessProxy>&&, uint64_t frameID, bool isMainFrame, WebCore::SecurityOriginData&&, WebCore::PolicyCheckIdentifier, uint64_t navigationID, NavigationActionData&&,
>+        FrameInfoData&&, uint64_t originatingPageID, const WebCore::ResourceRequest& originalRequest, WebCore::ResourceRequest&&, IPC::FormDataReference&& requestBody,
>+        WebCore::ResourceResponse&& redirectResponse, const UserData&, Messages::WebPageProxy::DecidePolicyForNavigationActionSync::DelayedReply&&);
> 
>     void dumpAdClickAttribution(CompletionHandler<void(const String&)>&&);
>     void clearAdClickAttribution(CompletionHandler<void()>&&);
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 96708e79bad6a035a8ef720dba9a8e63d295b9b1..947864c012bb746c27b5fccf0df4b1bd7e381582 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,15 @@
>+2019-02-20  Chris Dumez  <cdumez@apple.com>
>+
>+        Regression(PSON) Crash under WebKit::WebPageProxy::decidePolicyForNavigationActionSync
>+        https://bugs.webkit.org/show_bug.cgi?id=194857
>+        <rdar://problem/47759323>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Add API test coverage.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
>+
> 2019-02-20  Adrian Perez de Castro  <aperez@igalia.com>
> 
>         [WPE][GTK] Enable support for CONTENT_EXTENSIONS
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>index 8c96bec03b3e70ffddaa349a562d72c55a41d5ef..1f210ac175fc3a825880c188af42ee8c48b19403 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>@@ -1483,6 +1483,55 @@ TEST(ProcessSwap, ServerRedirect2)
>     EXPECT_WK_STREQ(@"pson://www.webkit.org/main1.html", [[webView URL] absoluteString]);
> }
> 
>+TEST(ProcessSwap, ServerRedirectToAboutBlank)
>+{
>+    auto processPoolConfiguration = psonProcessPoolConfiguration();
>+    auto processPool = adoptNS([[WKProcessPool alloc] _initWithConfiguration:processPoolConfiguration.get()]);
>+
>+    auto webViewConfiguration = adoptNS([[WKWebViewConfiguration alloc] init]);
>+    [webViewConfiguration setProcessPool:processPool.get()];
>+    auto handler = adoptNS([[PSONScheme alloc] init]);
>+    [handler addRedirectFromURLString:@"pson://www.webkit.org/main.html" toURLString:@"about:blank"];
>+    [webViewConfiguration setURLSchemeHandler:handler.get() forURLScheme:@"pson"];
>+
>+    auto webView = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:webViewConfiguration.get()]);
>+    auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
>+    [webView setNavigationDelegate:delegate.get()];
>+
>+    NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.google.com/main.html"]];
>+    [webView loadRequest:request];
>+
>+    TestWebKitAPI::Util::run(&done);
>+    done = false;
>+
>+    auto pidAfterFirstLoad = [webView _webProcessIdentifier];
>+
>+    EXPECT_EQ(1, numberOfDecidePolicyCalls);
>+    EXPECT_EQ(1u, seenPIDs.size());
>+    EXPECT_TRUE(*seenPIDs.begin() == pidAfterFirstLoad);
>+
>+    request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
>+    [webView loadRequest:request];
>+
>+    TestWebKitAPI::Util::run(&serverRedirected);
>+    serverRedirected = false;
>+
>+    seenPIDs.add([webView _webProcessIdentifier]);
>+    if (auto provisionalPID = [webView _provisionalWebProcessIdentifier])
>+        seenPIDs.add(provisionalPID);
>+
>+    TestWebKitAPI::Util::run(&done);
>+    done = false;
>+
>+    seenPIDs.add([webView _webProcessIdentifier]);
>+    if (auto provisionalPID = [webView _provisionalWebProcessIdentifier])
>+        seenPIDs.add(provisionalPID);
>+
>+    EXPECT_FALSE(serverRedirected);
>+    EXPECT_EQ(3, numberOfDecidePolicyCalls);
>+    EXPECT_EQ(2u, seenPIDs.size());
>+}
>+
> enum class ShouldCacheProcessFirst { No, Yes };
> static void runSameOriginServerRedirectTest(ShouldCacheProcessFirst shouldCacheProcessFirst)
> {

Actions: View | Formatted Diff | Diff

Attachments on bug 194857: 362508