Attachment 357321 Details for Bug 192704 – Patch

[patch] Patch

bug-192704-20181214100422.patch (text/plain), 12.03 KB, created by Chris Dumez on 2018-12-14 10:04:22 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-12-14 10:04:22 PST

Size: 12.03 KB

patch

obsolete

>Subversion Revision: 239210
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 0109f1a48b0f999666f8286f4f3ba065862ec3c1..860be01f2545839ab62733dbd37f8c1aa501de4a 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,25 @@
>+2018-12-14  Chris Dumez  <cdumez@apple.com>
>+
>+        [PSON] Process-swapping on a loadHTMLString causes duplicate decidePolicyForNavigationAction delegate calls
>+        https://bugs.webkit.org/show_bug.cgi?id=192704
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Process-swapping on a loadHTMLString causes duplicate decidePolicyForNavigationAction delegate calls. This
>+        is because we were failing to pass the ShouldTreatAsContinuingLoad flag to the WebContent process when
>+        doing a LoadData.
>+
>+        * UIProcess/WebPageProxy.cpp:
>+        (WebKit::WebPageProxy::loadData):
>+        (WebKit::WebPageProxy::loadDataWithNavigation):
>+        (WebKit::WebPageProxy::continueNavigationInNewProcess):
>+        * UIProcess/WebPageProxy.h:
>+        * WebProcess/WebPage/WebPage.cpp:
>+        (WebKit::WebPage::loadDataImpl):
>+        (WebKit::WebPage::loadData):
>+        (WebKit::WebPage::loadAlternateHTML):
>+        * WebProcess/WebPage/WebPage.h:
>+
> 2018-12-14  Chris Dumez  <cdumez@apple.com>
> 
>         [PSON] WebsitePolicies are lost on process-swap
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
>index cb225109767ec3f9cd49b8efc826993f42a29a97..9fd9fb8210673e4b2a78bcc4f6ae5a372505fa28 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebPageProxy.cpp
>@@ -1130,11 +1130,11 @@ RefPtr<API::Navigation> WebPageProxy::loadData(const IPC::DataReference& data, c
>         return nullptr;
> 
>     auto navigation = m_navigationState->createLoadDataNavigation(std::make_unique<API::SubstituteData>(data.vector(), MIMEType, encoding, baseURL, userData));
>-    loadDataWithNavigation(navigation, data, MIMEType, encoding, baseURL, userData);
>+    loadDataWithNavigation(navigation, data, MIMEType, encoding, baseURL, userData, ShouldTreatAsContinuingLoad::No);
>     return WTFMove(navigation);
> }
> 
>-void WebPageProxy::loadDataWithNavigation(API::Navigation& navigation, const IPC::DataReference& data, const String& MIMEType, const String& encoding, const String& baseURL, API::Object* userData, std::optional<WebsitePoliciesData>&& websitePolicies)
>+void WebPageProxy::loadDataWithNavigation(API::Navigation& navigation, const IPC::DataReference& data, const String& MIMEType, const String& encoding, const String& baseURL, API::Object* userData, ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad, std::optional<WebsitePoliciesData>&& websitePolicies)
> {
>     ASSERT(!m_isClosed);
> 
>@@ -1151,6 +1151,7 @@ void WebPageProxy::loadDataWithNavigation(API::Navigation& navigation, const IPC
>     loadParameters.MIMEType = MIMEType;
>     loadParameters.encodingName = encoding;
>     loadParameters.baseURLString = baseURL;
>+    loadParameters.shouldTreatAsContinuingLoad = shouldTreatAsContinuingLoad == ShouldTreatAsContinuingLoad::Yes;
>     loadParameters.userData = UserData(process().transformObjectsToHandles(userData).get());
>     loadParameters.websitePolicies = WTFMove(websitePolicies);
>     addPlatformLoadParameters(loadParameters);
>@@ -2727,7 +2728,7 @@ void WebPageProxy::continueNavigationInNewProcess(API::Navigation& navigation, s
>     // FIXME: Work out timing of responding with the last policy delegate, etc
>     ASSERT(!navigation.currentRequest().isEmpty());
>     if (auto& substituteData = navigation.substituteData())
>-        loadDataWithNavigation(navigation, { substituteData->content.data(), substituteData->content.size() }, substituteData->MIMEType, substituteData->encoding, substituteData->baseURL, substituteData->userData.get(), WTFMove(websitePolicies));
>+        loadDataWithNavigation(navigation, { substituteData->content.data(), substituteData->content.size() }, substituteData->MIMEType, substituteData->encoding, substituteData->baseURL, substituteData->userData.get(), ShouldTreatAsContinuingLoad::Yes, WTFMove(websitePolicies));
>     else
>         loadRequestWithNavigation(navigation, ResourceRequest { navigation.currentRequest() }, WebCore::ShouldOpenExternalURLsPolicy::ShouldAllowExternalSchemes, nullptr, ShouldTreatAsContinuingLoad::Yes, WTFMove(websitePolicies));
> 
>diff --git a/Source/WebKit/UIProcess/WebPageProxy.h b/Source/WebKit/UIProcess/WebPageProxy.h
>index ec4763c41b92f1b8c9f3bc8b2e82b5ccc2cca405..3ea01dc7c2e699675b4a1c1339e0bb571d2b24bf 100644
>--- a/Source/WebKit/UIProcess/WebPageProxy.h
>+++ b/Source/WebKit/UIProcess/WebPageProxy.h
>@@ -1576,7 +1576,7 @@ private:
>     RefPtr<API::Navigation> reattachToWebProcessForReload();
>     RefPtr<API::Navigation> reattachToWebProcessWithItem(WebBackForwardListItem&);
> 
>-    void loadDataWithNavigation(API::Navigation&, const IPC::DataReference&, const String& MIMEType, const String& encoding, const String& baseURL, API::Object* userData = nullptr, std::optional<WebsitePoliciesData>&& = std::nullopt);
>+    void loadDataWithNavigation(API::Navigation&, const IPC::DataReference&, const String& MIMEType, const String& encoding, const String& baseURL, API::Object* userData, WebCore::ShouldTreatAsContinuingLoad, std::optional<WebsitePoliciesData>&& = std::nullopt);
>     void loadRequestWithNavigation(API::Navigation&, WebCore::ResourceRequest&&, WebCore::ShouldOpenExternalURLsPolicy, API::Object* userData, WebCore::ShouldTreatAsContinuingLoad, std::optional<WebsitePoliciesData>&& = std::nullopt);
> 
>     void requestNotificationPermission(uint64_t notificationID, const String& originString);
>diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>index 1762a7b8f8d7eadf153122cc8075d957f6012012..5d1cf685ccb088e3f838a87469a6d7e0bf57fa55 100644
>--- a/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>+++ b/Source/WebKit/WebProcess/WebPage/WebPage.cpp
>@@ -1378,7 +1378,7 @@ void WebPage::loadRequest(LoadParameters&& loadParameters)
>     ASSERT(!m_pendingWebsitePolicies);
> }
> 
>-void WebPage::loadDataImpl(uint64_t navigationID, std::optional<WebsitePoliciesData>&& websitePolicies, Ref<SharedBuffer>&& sharedBuffer, const String& MIMEType, const String& encodingName, const URL& baseURL, const URL& unreachableURL, const UserData& userData)
>+void WebPage::loadDataImpl(uint64_t navigationID, bool shouldTreatAsContinuingLoad, std::optional<WebsitePoliciesData>&& websitePolicies, Ref<SharedBuffer>&& sharedBuffer, const String& MIMEType, const String& encodingName, const URL& baseURL, const URL& unreachableURL, const UserData& userData)
> {
>     SendStopResponsivenessTimer stopper;
> 
>@@ -1394,7 +1394,9 @@ void WebPage::loadDataImpl(uint64_t navigationID, std::optional<WebsitePoliciesD
>     m_loaderClient->willLoadDataRequest(*this, request, const_cast<SharedBuffer*>(substituteData.content()), substituteData.mimeType(), substituteData.textEncoding(), substituteData.failingURL(), WebProcess::singleton().transformHandlesToObjects(userData.object()).get());
> 
>     // Initate the load in WebCore.
>-    m_mainFrame->coreFrame()->loader().load(FrameLoadRequest(*m_mainFrame->coreFrame(), request, ShouldOpenExternalURLsPolicy::ShouldNotAllow, substituteData));
>+    FrameLoadRequest frameLoadRequest(*m_mainFrame->coreFrame(), request, ShouldOpenExternalURLsPolicy::ShouldNotAllow, substituteData);
>+    frameLoadRequest.setShouldTreatAsContinuingLoad(shouldTreatAsContinuingLoad);
>+    m_mainFrame->coreFrame()->loader().load(WTFMove(frameLoadRequest));
> }
> 
> void WebPage::loadData(LoadParameters&& loadParameters)
>@@ -1403,7 +1405,7 @@ void WebPage::loadData(LoadParameters&& loadParameters)
> 
>     auto sharedBuffer = SharedBuffer::create(reinterpret_cast<const char*>(loadParameters.data.data()), loadParameters.data.size());
>     URL baseURL = loadParameters.baseURLString.isEmpty() ? WTF::blankURL() : URL(URL(), loadParameters.baseURLString);
>-    loadDataImpl(loadParameters.navigationID, WTFMove(loadParameters.websitePolicies), WTFMove(sharedBuffer), loadParameters.MIMEType, loadParameters.encodingName, baseURL, URL(), loadParameters.userData);
>+    loadDataImpl(loadParameters.navigationID, loadParameters.shouldTreatAsContinuingLoad, WTFMove(loadParameters.websitePolicies), WTFMove(sharedBuffer), loadParameters.MIMEType, loadParameters.encodingName, baseURL, URL(), loadParameters.userData);
> }
> 
> void WebPage::loadAlternateHTML(LoadParameters&& loadParameters)
>@@ -1415,7 +1417,7 @@ void WebPage::loadAlternateHTML(LoadParameters&& loadParameters)
>     URL provisionalLoadErrorURL = loadParameters.provisionalLoadErrorURLString.isEmpty() ? URL() : URL(URL(), loadParameters.provisionalLoadErrorURLString);
>     auto sharedBuffer = SharedBuffer::create(reinterpret_cast<const char*>(loadParameters.data.data()), loadParameters.data.size());
>     m_mainFrame->coreFrame()->loader().setProvisionalLoadErrorBeingHandledURL(provisionalLoadErrorURL);    
>-    loadDataImpl(loadParameters.navigationID, WTFMove(loadParameters.websitePolicies), WTFMove(sharedBuffer), loadParameters.MIMEType, loadParameters.encodingName, baseURL, unreachableURL, loadParameters.userData);
>+    loadDataImpl(loadParameters.navigationID, loadParameters.shouldTreatAsContinuingLoad, WTFMove(loadParameters.websitePolicies), WTFMove(sharedBuffer), loadParameters.MIMEType, loadParameters.encodingName, baseURL, unreachableURL, loadParameters.userData);
>     m_mainFrame->coreFrame()->loader().setProvisionalLoadErrorBeingHandledURL({ });
> }
> 
>diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.h b/Source/WebKit/WebProcess/WebPage/WebPage.h
>index e873c87646b37769be030a99cdcd0868506491d3..7908e90d2ade17e261078d3f219d78797ef7085d 100644
>--- a/Source/WebKit/WebProcess/WebPage/WebPage.h
>+++ b/Source/WebKit/WebProcess/WebPage/WebPage.h
>@@ -1195,7 +1195,7 @@ private:
> 
>     String sourceForFrame(WebFrame*);
> 
>-    void loadDataImpl(uint64_t navigationID, std::optional<WebsitePoliciesData>&&, Ref<WebCore::SharedBuffer>&&, const String& MIMEType, const String& encodingName, const URL& baseURL, const URL& failingURL, const UserData&);
>+    void loadDataImpl(uint64_t navigationID, bool shouldTreatAsContinuingLoad, std::optional<WebsitePoliciesData>&&, Ref<WebCore::SharedBuffer>&&, const String& MIMEType, const String& encodingName, const URL& baseURL, const URL& failingURL, const UserData&);
> 
>     // Actions
>     void tryClose();
>diff --git a/Tools/ChangeLog b/Tools/ChangeLog
>index 9e01480c36598f8405b0ddae4f43fb552c0f5423..03a4be0b0ce54d9fec8e33330cc4a128c49b8100 100644
>--- a/Tools/ChangeLog
>+++ b/Tools/ChangeLog
>@@ -1,3 +1,14 @@
>+2018-12-14  Chris Dumez  <cdumez@apple.com>
>+
>+        [PSON] Process-swapping on a loadHTMLString causes duplicate decidePolicyForNavigationAction delegate calls
>+        https://bugs.webkit.org/show_bug.cgi?id=192704
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Extend existing API test to reproduce the problem.
>+
>+        * TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:
>+
> 2018-12-14  Chris Dumez  <cdumez@apple.com>
> 
>         [PSON] WebsitePolicies are lost on process-swap
>diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>index 149ec1a2552f6aacab061b17de3fee82561e26bc..0701dc4be64909b96c2db8f5316221002c236619 100644
>--- a/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>+++ b/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm
>@@ -2997,6 +2997,8 @@ TEST(ProcessSwap, SwapOnLoadHTMLString)
>     auto delegate = adoptNS([[PSONNavigationDelegate alloc] init]);
>     [webView setNavigationDelegate:delegate.get()];
> 
>+    numberOfDecidePolicyCalls = 0;
>+
>     NSURLRequest *request = [NSURLRequest requestWithURL:[NSURL URLWithString:@"pson://www.webkit.org/main.html"]];
>     [webView loadRequest:request];
> 
>@@ -3014,6 +3016,8 @@ TEST(ProcessSwap, SwapOnLoadHTMLString)
>     auto pid2 = [webView _webProcessIdentifier];
>     EXPECT_NE(pid1, pid2);
> 
>+    EXPECT_EQ(2, numberOfDecidePolicyCalls);
>+
>     [webView evaluateJavaScript:@"document.body.innerText" completionHandler:^(id innerText, NSError *error) {
>         EXPECT_WK_STREQ(@"substituteData", (NSString *)innerText);
>         done = true;

Actions: View | Formatted Diff | Diff

Attachments on bug 192704: 357321