REGRESSION (r229831): CMD-clicking an iCloud web app link unexpectedly opens that...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Apr 2018 18:13:49 +0000 (18:13 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 17 Apr 2018 18:13:49 +0000 (18:13 +0000)
https://bugs.webkit.org/show_bug.cgi?id=184678
<rdar://problem/39422122>

Reviewed by Alex Christensen.

Source/WebCore:

Frament navigations need to happen synchronously for Web-compatibility. Because of this,
r225657 added code to make sure that if the client does not make the navigation policy
decision synchronously for frament navigations, then we'll stop waiting for the client
and proceed with the navigation. However, r229831 make the navigation policy decision
IPC decision, meaning that even if the client responds synchronously, it would be
asynchronously from WebCore's point of view. As a result, we would always ignore the
client's policy decision when doing a fragment navigation.

This is an issue on iclould.com because the web-app links are fragment URLs. When you
CMD+click one of these link, we do the navigation policy check. As a result of this
check, Safari responds IGNORE to the policy decision and instead decides to load the
link in a new tab (because CMD key is pressed). Due to the bug mentioned above, we
would not obey the IGNORE policy decision from Safari and load the link in the current
tab, even though Safari would already be loading it in a new tab.

To address the issue, I reintroduced a synchronous code path for navigation policy
decision making, backed by synchronous IPC. This synchronous code path is now used for
fragment navigations to restore pre-r229831 behavior. If the client does not answer
synchronously, we'll proceed with the navigation anyway, as was happening pre-r229831.

Test: http/tests/navigation/fragment-navigation-policy-ignore.html

* loader/EmptyClients.cpp:
(WebCore::EmptyFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
* loader/EmptyFrameLoaderClient.h:
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::loadURL):
(WebCore::FrameLoader::loadWithDocumentLoader):
* loader/FrameLoaderClient.h:
* loader/PolicyChecker.cpp:
(WebCore::PolicyChecker::checkNavigationPolicy):
* loader/PolicyChecker.h:

Source/WebKit:

Re-introduce synchronous code path which existed pre-r229831 and use it for fragment navigations.

* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::receivedPolicyDecision):
(WebKit::WebPageProxy::decidePolicyForNavigationActionSync):
* UIProcess/WebPageProxy.h:
* UIProcess/WebPageProxy.messages.in:
* WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
* WebProcess/WebCoreSupport/WebFrameLoaderClient.h:

Source/WebKitLegacy/mac:

Add new parameter to dispatchDecidePolicyForNavigationAction.

* WebCoreSupport/WebFrameLoaderClient.h:
* WebCoreSupport/WebFrameLoaderClient.mm:
(WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):

Source/WebKitLegacy/win:

Add new parameter to dispatchDecidePolicyForNavigationAction.

* WebCoreSupport/WebFrameLoaderClient.cpp:
(WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
* WebCoreSupport/WebFrameLoaderClient.h:

LayoutTests:

Add layout test coverage.

* http/tests/navigation/fragment-navigation-policy-ignore-expected.txt: Added.
* http/tests/navigation/fragment-navigation-policy-ignore.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@230721 268f45cc-cd09-0410-ab3c-d52691b4dbfc

22 files changed:
LayoutTests/ChangeLog
LayoutTests/http/tests/navigation/fragment-navigation-policy-ignore-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/navigation/fragment-navigation-policy-ignore.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/EmptyClients.cpp
Source/WebCore/loader/EmptyFrameLoaderClient.h
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/FrameLoaderClient.h
Source/WebCore/loader/PolicyChecker.cpp
Source/WebCore/loader/PolicyChecker.h
Source/WebKit/ChangeLog
Source/WebKit/UIProcess/WebPageProxy.cpp
Source/WebKit/UIProcess/WebPageProxy.h
Source/WebKit/UIProcess/WebPageProxy.messages.in
Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.h
Source/WebKitLegacy/mac/ChangeLog
Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.h
Source/WebKitLegacy/mac/WebCoreSupport/WebFrameLoaderClient.mm
Source/WebKitLegacy/win/ChangeLog
Source/WebKitLegacy/win/WebCoreSupport/WebFrameLoaderClient.cpp
Source/WebKitLegacy/win/WebCoreSupport/WebFrameLoaderClient.h

index ce3154adb16140f964da0dd5b9c1f387b256377b..9cc1ef19090b462323c8cb624ac5ece526c29622 100644 (file)
@@ -1,3 +1,16 @@
+2018-04-17  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION (r229831): CMD-clicking an iCloud web app link unexpectedly opens that link in a new tab and the current tab
+        https://bugs.webkit.org/show_bug.cgi?id=184678
+        <rdar://problem/39422122>
+
+        Reviewed by Alex Christensen.
+
+        Add layout test coverage.
+
+        * http/tests/navigation/fragment-navigation-policy-ignore-expected.txt: Added.
+        * http/tests/navigation/fragment-navigation-policy-ignore.html: Added.
+
 2018-04-17  Said Abou-Hallawa  <sabouhallawa@apple.com>
 
         Animated GIF imagery with finite looping are falling one loop short
diff --git a/LayoutTests/http/tests/navigation/fragment-navigation-policy-ignore-expected.txt b/LayoutTests/http/tests/navigation/fragment-navigation-policy-ignore-expected.txt
new file mode 100644 (file)
index 0000000..75fa25c
--- /dev/null
@@ -0,0 +1,11 @@
+Policy delegate: attempt to load http://127.0.0.1:8000/navigation/fragment-navigation-policy-ignore.html#test with navigation type 'other'
+Checks that the client can prevent a fragment navigation via the decidePolicyForNavigationAction delegate.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS window.location.href is "http://127.0.0.1:8000/navigation/fragment-navigation-policy-ignore.html"
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/navigation/fragment-navigation-policy-ignore.html b/LayoutTests/http/tests/navigation/fragment-navigation-policy-ignore.html
new file mode 100644 (file)
index 0000000..9a2b17b
--- /dev/null
@@ -0,0 +1,23 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script src="/js-test-resources/js-test.js"></script>
+</head>
+<body>
+<script>
+description("Checks that the client can prevent a fragment navigation via the decidePolicyForNavigationAction delegate.");
+jsTestIsAsync = true;
+
+if (window.testRunner)
+    testRunner.setCustomPolicyDelegate(true, false);
+
+onload = function() {
+    location = "#test";
+    setTimeout(function() {
+        shouldBeEqualToString("window.location.href", "http://127.0.0.1:8000/navigation/fragment-navigation-policy-ignore.html");
+        finishJSTest();
+    }, 0);
+}
+</script>
+</body>
+</html>
index edf92299d459af1a944d7020102c5b6b5f355ac9..4d9512e32652d3cc0878ba36a73bf7897f4ff307 100644 (file)
@@ -1,3 +1,44 @@
+2018-04-17  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION (r229831): CMD-clicking an iCloud web app link unexpectedly opens that link in a new tab and the current tab
+        https://bugs.webkit.org/show_bug.cgi?id=184678
+        <rdar://problem/39422122>
+
+        Reviewed by Alex Christensen.
+
+        Frament navigations need to happen synchronously for Web-compatibility. Because of this,
+        r225657 added code to make sure that if the client does not make the navigation policy
+        decision synchronously for frament navigations, then we'll stop waiting for the client
+        and proceed with the navigation. However, r229831 make the navigation policy decision
+        IPC decision, meaning that even if the client responds synchronously, it would be
+        asynchronously from WebCore's point of view. As a result, we would always ignore the
+        client's policy decision when doing a fragment navigation.
+
+        This is an issue on iclould.com because the web-app links are fragment URLs. When you
+        CMD+click one of these link, we do the navigation policy check. As a result of this
+        check, Safari responds IGNORE to the policy decision and instead decides to load the
+        link in a new tab (because CMD key is pressed). Due to the bug mentioned above, we
+        would not obey the IGNORE policy decision from Safari and load the link in the current
+        tab, even though Safari would already be loading it in a new tab.
+
+        To address the issue, I reintroduced a synchronous code path for navigation policy
+        decision making, backed by synchronous IPC. This synchronous code path is now used for
+        fragment navigations to restore pre-r229831 behavior. If the client does not answer
+        synchronously, we'll proceed with the navigation anyway, as was happening pre-r229831.
+
+        Test: http/tests/navigation/fragment-navigation-policy-ignore.html
+
+        * loader/EmptyClients.cpp:
+        (WebCore::EmptyFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
+        * loader/EmptyFrameLoaderClient.h:
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::loadURL):
+        (WebCore::FrameLoader::loadWithDocumentLoader):
+        * loader/FrameLoaderClient.h:
+        * loader/PolicyChecker.cpp:
+        (WebCore::PolicyChecker::checkNavigationPolicy):
+        * loader/PolicyChecker.h:
+
 2018-04-17  Matt Lewis  <jlewis3@apple.com>
 
         Unreviewed, rolling out r230713.
index 1ed3cb141ab3741d30a08a36eef524e5d0aef0f8..b61b26fe115a43b4bd96280aca7bf6add7139fdf 100644 (file)
@@ -436,7 +436,7 @@ void EmptyFrameLoaderClient::dispatchDecidePolicyForNewWindowAction(const Naviga
 {
 }
 
-void EmptyFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const NavigationAction&, const ResourceRequest&, bool, FormState*, FramePolicyFunction&&)
+void EmptyFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const NavigationAction&, const ResourceRequest&, bool, FormState*, PolicyDecisionMode, FramePolicyFunction&&)
 {
 }
 
index 07c32b8c3ee769170b2d5ea5115c339cbce8a490..34a508ab58c6d67ade235779603d47d9d423a928 100644 (file)
@@ -95,7 +95,7 @@ class WEBCORE_EXPORT EmptyFrameLoaderClient : public FrameLoaderClient {
 
     void dispatchDecidePolicyForResponse(const ResourceResponse&, const ResourceRequest&, FramePolicyFunction&&) final { }
     void dispatchDecidePolicyForNewWindowAction(const NavigationAction&, const ResourceRequest&, FormState*, const String&, FramePolicyFunction&&) final;
-    void dispatchDecidePolicyForNavigationAction(const NavigationAction&, const ResourceRequest&, bool didReceiveRedirectResponse, FormState*, FramePolicyFunction&&) final;
+    void dispatchDecidePolicyForNavigationAction(const NavigationAction&, const ResourceRequest&, bool didReceiveRedirectResponse, FormState*, PolicyDecisionMode, FramePolicyFunction&&) final;
     void cancelPolicyCheck() final { }
 
     void dispatchUnableToImplementPolicy(const ResourceError&) final { }
index 759a4ce7e073916bfd0d541dbf1a54f6058eaf83..a5c6e3d0bebbdeb6456a2ca601d6abdb98302214 100644 (file)
@@ -1260,10 +1260,6 @@ bool FrameLoader::isStopLoadingAllowed() const
     return m_pageDismissalEventBeingDispatched == PageDismissalType::None;
 }
 
-struct SharedBool : public RefCounted<SharedBool> {
-    bool value { false };
-};
-
 void FrameLoader::loadURL(FrameLoadRequest&& frameLoadRequest, const String& referrer, FrameLoadType newLoadType, Event* event, FormState* formState, CompletionHandler<void()>&& completionHandler)
 {
     CompletionHandlerCallingScope completionHandlerCaller(WTFMove(completionHandler));
@@ -1336,17 +1332,9 @@ void FrameLoader::loadURL(FrameLoadRequest&& frameLoadRequest, const String& ref
         oldDocumentLoader->setLastCheckedRequest(ResourceRequest());
         policyChecker().stopCheck();
         policyChecker().setLoadType(newLoadType);
-        auto completionHandlerCalled = adoptRef(*new SharedBool);
-        policyChecker().checkNavigationPolicy(ResourceRequest(request), false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState, [this, protectedFrame = makeRef(m_frame), completionHandlerCalled = completionHandlerCalled.copyRef()] (const ResourceRequest& request, FormState*, ShouldContinue shouldContinue) {
-            if (!completionHandlerCalled->value) {
-                completionHandlerCalled->value = true;
-                continueFragmentScrollAfterNavigationPolicy(request, shouldContinue == ShouldContinue::Yes);
-            }
-        });
-            if (!completionHandlerCalled->value) {
-            completionHandlerCalled->value = true;
-            continueFragmentScrollAfterNavigationPolicy(WTFMove(request), true);
-        }
+        policyChecker().checkNavigationPolicy(ResourceRequest(request), false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState, [this, protectedFrame = makeRef(m_frame)] (const ResourceRequest& request, FormState*, ShouldContinue shouldContinue) {
+            continueFragmentScrollAfterNavigationPolicy(request, shouldContinue == ShouldContinue::Yes);
+        }, PolicyDecisionMode::Synchronous);
         return;
     }
 
@@ -1512,17 +1500,9 @@ void FrameLoader::loadWithDocumentLoader(DocumentLoader* loader, FrameLoadType t
         oldDocumentLoader->setTriggeringAction(action);
         oldDocumentLoader->setLastCheckedRequest(ResourceRequest());
         policyChecker().stopCheck();
-        auto completionHandlerCalled = adoptRef(*new SharedBool);
-        policyChecker().checkNavigationPolicy(ResourceRequest(loader->request()), false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState, [this, protectedFrame = makeRef(m_frame), completionHandlerCalled = completionHandlerCalled.copyRef()] (const ResourceRequest& request, FormState*, ShouldContinue shouldContinue) {
-            if (!completionHandlerCalled->value) {
-                completionHandlerCalled->value = true;
-                continueFragmentScrollAfterNavigationPolicy(request, shouldContinue == ShouldContinue::Yes);
-            }
-        });
-        if (!completionHandlerCalled->value) {
-            completionHandlerCalled->value = true;
-            continueFragmentScrollAfterNavigationPolicy(loader->request(), true);
-        }
+        policyChecker().checkNavigationPolicy(ResourceRequest(loader->request()), false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState, [this, protectedFrame = makeRef(m_frame)] (const ResourceRequest& request, FormState*, ShouldContinue shouldContinue) {
+            continueFragmentScrollAfterNavigationPolicy(request, shouldContinue == ShouldContinue::Yes);
+        }, PolicyDecisionMode::Synchronous);
         return;
     }
 
index d941d26582df293b1f9fc8c3bfdeaa69c8dd59a1..f8c4b4e8965fcba74c17baf78dd0365e5797f809 100644 (file)
@@ -102,6 +102,7 @@ class SubstituteData;
 class URL;
 class Widget;
 
+enum class PolicyDecisionMode;
 struct StringWithDirection;
 
 typedef WTF::Function<void (PolicyAction)> FramePolicyFunction;
@@ -189,7 +190,7 @@ public:
 
     virtual void dispatchDecidePolicyForResponse(const ResourceResponse&, const ResourceRequest&, FramePolicyFunction&&) = 0;
     virtual void dispatchDecidePolicyForNewWindowAction(const NavigationAction&, const ResourceRequest&, FormState*, const String& frameName, FramePolicyFunction&&) = 0;
-    virtual void dispatchDecidePolicyForNavigationAction(const NavigationAction&, const ResourceRequest&, bool didReceiveRedirectResponse, FormState*, FramePolicyFunction&&) = 0;
+    virtual void dispatchDecidePolicyForNavigationAction(const NavigationAction&, const ResourceRequest&, bool didReceiveRedirectResponse, FormState*, PolicyDecisionMode, FramePolicyFunction&&) = 0;
     virtual void didDecidePolicyForNavigationAction() { }
     virtual void cancelPolicyCheck() = 0;
 
index 3702580c3d2b97891bda0a026411c3710a0e7880..82d6f697861e4bf396c23f0589d4083f4b72e853 100644 (file)
@@ -81,7 +81,7 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& newRequest, bool did
     checkNavigationPolicy(WTFMove(newRequest), didReceiveRedirectResponse, m_frame.loader().activeDocumentLoader(), nullptr, WTFMove(function));
 }
 
-void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, bool didReceiveRedirectResponse, DocumentLoader* loader, FormState* formState, NavigationPolicyDecisionFunction&& function)
+void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, bool didReceiveRedirectResponse, DocumentLoader* loader, FormState* formState, NavigationPolicyDecisionFunction&& function, PolicyDecisionMode policyDecisionMode)
 {
     NavigationAction action = loader->triggeringAction();
     if (action.isEmpty()) {
@@ -149,7 +149,7 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, bool didRec
     m_delegateIsDecidingNavigationPolicy = true;
     String suggestedFilename = action.downloadAttribute().isEmpty() ? nullAtom() : action.downloadAttribute();
     ResourceRequest requestCopy = request;
-    m_frame.loader().client().dispatchDecidePolicyForNavigationAction(action, request, didReceiveRedirectResponse, formState, [this, function = WTFMove(function), request = WTFMove(requestCopy), formState = makeRefPtr(formState), suggestedFilename = WTFMove(suggestedFilename)](PolicyAction policyAction) mutable {
+    m_frame.loader().client().dispatchDecidePolicyForNavigationAction(action, request, didReceiveRedirectResponse, formState, policyDecisionMode, [this, function = WTFMove(function), request = WTFMove(requestCopy), formState = makeRefPtr(formState), suggestedFilename = WTFMove(suggestedFilename)](PolicyAction policyAction) mutable {
         m_frame.loader().client().didDecidePolicyForNavigationAction();
 
         m_delegateIsDecidingNavigationPolicy = false;
index 2fdb25021956a55019c67273def2f8122958d499..b16c1ee2594dd7db157e43ee4d8396f6d75a7381 100644 (file)
@@ -56,6 +56,8 @@ enum class ShouldContinue {
     ForSuspension
 };
 
+enum class PolicyDecisionMode { Synchronous, Asynchronous };
+
 using NewWindowPolicyDecisionFunction = CompletionHandler<void(const ResourceRequest&, FormState*, const String& frameName, const NavigationAction&, ShouldContinue)>;
 using NavigationPolicyDecisionFunction = CompletionHandler<void(ResourceRequest&&, FormState*, ShouldContinue)>;
 
@@ -65,7 +67,7 @@ class PolicyChecker {
 public:
     explicit PolicyChecker(Frame&);
 
-    void checkNavigationPolicy(ResourceRequest&&, bool didReceiveRedirectResponse, DocumentLoader*, FormState*, NavigationPolicyDecisionFunction&&);
+    void checkNavigationPolicy(ResourceRequest&&, bool didReceiveRedirectResponse, DocumentLoader*, FormState*, NavigationPolicyDecisionFunction&&, PolicyDecisionMode = PolicyDecisionMode::Asynchronous);
     void checkNavigationPolicy(ResourceRequest&&, bool didReceiveRedirectResponse, NavigationPolicyDecisionFunction&&);
     void checkNewWindowPolicy(NavigationAction&&, const ResourceRequest&, FormState*, const String& frameName, NewWindowPolicyDecisionFunction&&);
 
index a0ee9c68c611a32461319f54d66da7cf8b536be6..eb70da486fa209f72e723ca7c2ddb91ccf056ade 100644 (file)
@@ -1,3 +1,22 @@
+2018-04-17  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION (r229831): CMD-clicking an iCloud web app link unexpectedly opens that link in a new tab and the current tab
+        https://bugs.webkit.org/show_bug.cgi?id=184678
+        <rdar://problem/39422122>
+
+        Reviewed by Alex Christensen.
+
+        Re-introduce synchronous code path which existed pre-r229831 and use it for fragment navigations.
+
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::receivedPolicyDecision):
+        (WebKit::WebPageProxy::decidePolicyForNavigationActionSync):
+        * UIProcess/WebPageProxy.h:
+        * UIProcess/WebPageProxy.messages.in:
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+        (WebKit::WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.h:
+
 2018-04-17  Michael Catanzaro  <mcatanzaro@igalia.com>
 
         [WPE][GTK] GObject introspection annotation fixes: BackForwardList, NetworkProxySettings
index 8f3740e961f3cb5719df0cc243232e1c0d6be215..84ce249467c59b8a98d9a29a8945822099336d9a 100644 (file)
@@ -2390,6 +2390,11 @@ void WebPageProxy::receivedPolicyDecision(PolicyAction action, WebFrameProxy& fr
         }
     }
 
+    if (auto syncNavigationActionPolicyReply = WTFMove(m_syncNavigationActionPolicyReply)) {
+        syncNavigationActionPolicyReply->send(navigation ? navigation->navigationID() : 0, action, downloadID, WTFMove(websitePolicies));
+        return;
+    }
+
     m_process->send(Messages::WebPage::DidReceivePolicyDecision(frame.frameID(), listenerID, action, navigation ? navigation->navigationID() : 0, downloadID, websitePolicies), m_pageID);
 }
 
@@ -3911,6 +3916,18 @@ void WebPageProxy::decidePolicyForNavigationAction(uint64_t frameID, const Secur
     m_shouldSuppressAppLinksInNextNavigationPolicyDecision = false;
 }
 
+void WebPageProxy::decidePolicyForNavigationActionSync(uint64_t frameID, const WebCore::SecurityOriginData& frameSecurityOrigin, uint64_t navigationID, NavigationActionData&& navigationActionData, const FrameInfoData& originatingFrameInfoData, uint64_t originatingPageID, const WebCore::ResourceRequest& originalRequest, WebCore::ResourceRequest&& request, uint64_t listenerID, const UserData& userData, Ref<Messages::WebPageProxy::DecidePolicyForNavigationActionSync::DelayedReply>&& reply)
+{
+    ASSERT(!m_syncNavigationActionPolicyReply);
+    m_syncNavigationActionPolicyReply = WTFMove(reply);
+
+    decidePolicyForNavigationAction(frameID, frameSecurityOrigin, navigationID, WTFMove(navigationActionData), originatingFrameInfoData, originatingPageID, originalRequest, WTFMove(request), listenerID, userData);
+
+    // If the client did not respond synchronously, proceed with the load.
+    if (auto syncNavigationActionPolicyReply = WTFMove(m_syncNavigationActionPolicyReply))
+        syncNavigationActionPolicyReply->send(navigationID, PolicyAction::Use, { }, { });
+}
+
 void WebPageProxy::decidePolicyForNewWindowAction(uint64_t frameID, const SecurityOriginData& frameSecurityOrigin, NavigationActionData&& navigationActionData, ResourceRequest&& request, const String& frameName, uint64_t listenerID, const UserData& userData)
 {
     PageClientProtector protector(m_pageClient);
index 151c5769c104ad1da0cfaf1e6fed776f2342f7e8..a1d231ad639313ea2ea6b97eaea1218d313496c8 100644 (file)
@@ -1367,6 +1367,7 @@ private:
     void didDestroyNavigation(uint64_t navigationID);
 
     void decidePolicyForNavigationAction(uint64_t frameID, const WebCore::SecurityOriginData& frameSecurityOrigin, uint64_t navigationID, NavigationActionData&&, const FrameInfoData&, uint64_t originatingPageID, const WebCore::ResourceRequest& originalRequest, WebCore::ResourceRequest&&, uint64_t listenerID, const UserData&);
+    void decidePolicyForNavigationActionSync(uint64_t frameID, const WebCore::SecurityOriginData& frameSecurityOrigin, uint64_t navigationID, NavigationActionData&&, const FrameInfoData&, uint64_t originatingPageID, const WebCore::ResourceRequest& originalRequest, WebCore::ResourceRequest&&, uint64_t listenerID, const UserData&, Ref<Messages::WebPageProxy::DecidePolicyForNavigationActionSync::DelayedReply>&&);
     void decidePolicyForNewWindowAction(uint64_t frameID, const WebCore::SecurityOriginData& frameSecurityOrigin, NavigationActionData&&, WebCore::ResourceRequest&&, const String& frameName, uint64_t listenerID, const UserData&);
     void decidePolicyForResponse(uint64_t frameID, const WebCore::SecurityOriginData& frameSecurityOrigin, uint64_t navigationID, const WebCore::ResourceResponse&, const WebCore::ResourceRequest&, bool canShowMIMEType, uint64_t listenerID, const UserData&);
     void unableToImplementPolicy(uint64_t frameID, const WebCore::ResourceError&, const UserData&);
@@ -1925,6 +1926,9 @@ private:
     bool m_isInPrintingMode { false };
     bool m_isPerformingDOMPrintOperation { false };
 
+    // Synchronous navigation policy decision.
+    RefPtr<Messages::WebPageProxy::DecidePolicyForNavigationActionSync::DelayedReply> m_syncNavigationActionPolicyReply;
+
     WebCore::ResourceRequest m_decidePolicyForResponseRequest;
     bool m_shouldSuppressAppLinksInNextNavigationPolicyDecision { false };
 
index 0f37b63d37e6a81ad105fed771b6941b05fffc00..2304a70a4d01f78b95dd9b35713ad15d81b82053 100644 (file)
@@ -100,6 +100,7 @@ messages -> WebPageProxy {
     # Policy messages
     DecidePolicyForResponse(uint64_t frameID, struct WebCore::SecurityOriginData frameSecurityOrigin, uint64_t navigationID, WebCore::ResourceResponse response, WebCore::ResourceRequest request, bool canShowMIMEType, uint64_t listenerID, WebKit::UserData userData)
     DecidePolicyForNavigationAction(uint64_t frameID, struct WebCore::SecurityOriginData frameSecurityOrigin, uint64_t navigationID, struct WebKit::NavigationActionData navigationActionData, struct WebKit::FrameInfoData originatingFrameInfoData, uint64_t originatingPageID, WebCore::ResourceRequest originalRequest, WebCore::ResourceRequest request, uint64_t listenerID, WebKit::UserData userData)
+    DecidePolicyForNavigationActionSync(uint64_t frameID, struct WebCore::SecurityOriginData frameSecurityOrigin, uint64_t navigationID, struct WebKit::NavigationActionData navigationActionData, struct WebKit::FrameInfoData originatingFrameInfoData, uint64_t originatingPageID, WebCore::ResourceRequest originalRequest, WebCore::ResourceRequest request, uint64_t listenerID, WebKit::UserData userData) -> (uint64_t newNavigationID, enum WebCore::PolicyAction policyAction, WebKit::DownloadID downloadID, std::optional<WebKit::WebsitePoliciesData> websitePolicies) Delayed
     DecidePolicyForNewWindowAction(uint64_t frameID, struct WebCore::SecurityOriginData frameSecurityOrigin, struct WebKit::NavigationActionData navigationActionData, WebCore::ResourceRequest request, String frameName, uint64_t listenerID, WebKit::UserData userData)
     UnableToImplementPolicy(uint64_t frameID, WebCore::ResourceError error, WebKit::UserData userData)
 
index 94294b0038456728ea86de3e8a2e4b664343db17..0ab11311a04b9dcf96c49ec7a206b0e21a2929e3 100644 (file)
@@ -815,7 +815,7 @@ void WebFrameLoaderClient::applyToDocumentLoader(WebsitePoliciesData&& websitePo
     WebsitePoliciesData::applyToDocumentLoader(WTFMove(websitePolicies), *documentLoader);
 }
 
-void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const NavigationAction& navigationAction, const ResourceRequest& request, bool didReceiveRedirectResponse, FormState* formState, FramePolicyFunction&& function)
+void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const NavigationAction& navigationAction, const ResourceRequest& request, bool didReceiveRedirectResponse, FormState* formState, PolicyDecisionMode policyDecisionMode, FramePolicyFunction&& function)
 {
     WebPage* webPage = m_frame ? m_frame->page() : nullptr;
     if (!webPage) {
@@ -887,6 +887,23 @@ void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const Navigat
 
     // Notify the UIProcess.
     Ref<WebFrame> protect(*m_frame);
+
+    if (policyDecisionMode == PolicyDecisionMode::Synchronous) {
+        uint64_t newNavigationID;
+        PolicyAction policyAction;
+        DownloadID downloadID;
+        std::optional<WebsitePoliciesData> websitePolicies;
+
+        if (!webPage->sendSync(Messages::WebPageProxy::DecidePolicyForNavigationActionSync(m_frame->frameID(), SecurityOriginData::fromFrame(coreFrame), documentLoader->navigationID(), navigationActionData, originatingFrameInfoData, originatingFrame && originatingFrame->page() ? originatingFrame->page()->pageID() : 0, navigationAction.resourceRequest(), request, listenerID, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get())), Messages::WebPageProxy::DecidePolicyForNavigationActionSync::Reply(newNavigationID, policyAction, downloadID, websitePolicies))) {
+            m_frame->didReceivePolicyDecision(listenerID, PolicyAction::Ignore, 0, { }, { });
+            return;
+        }
+
+        m_frame->didReceivePolicyDecision(listenerID, policyAction, 0, downloadID, { });
+        return;
+    }
+
+    ASSERT(policyDecisionMode == PolicyDecisionMode::Asynchronous);
     if (!webPage->send(Messages::WebPageProxy::DecidePolicyForNavigationAction(m_frame->frameID(), SecurityOriginData::fromFrame(coreFrame), documentLoader->navigationID(), navigationActionData, originatingFrameInfoData, originatingFrame && originatingFrame->page() ? originatingFrame->page()->pageID() : 0, navigationAction.resourceRequest(), request, listenerID, UserData(WebProcess::singleton().transformObjectsToHandles(userData.get()).get()))))
         m_frame->didReceivePolicyDecision(listenerID, PolicyAction::Ignore, 0, { }, { });
 }
index d38b814f750b19929aa7b089484101de93a18157..768bf7cdca39f4f6e79b5ee36818f2dce32ca926 100644 (file)
@@ -125,7 +125,7 @@ private:
     
     void dispatchDecidePolicyForResponse(const WebCore::ResourceResponse&, const WebCore::ResourceRequest&, WebCore::FramePolicyFunction&&) final;
     void dispatchDecidePolicyForNewWindowAction(const WebCore::NavigationAction&, const WebCore::ResourceRequest&, WebCore::FormState*, const String& frameName, WebCore::FramePolicyFunction&&) final;
-    void dispatchDecidePolicyForNavigationAction(const WebCore::NavigationAction&, const WebCore::ResourceRequest&, bool didReceiveRedirectResponse, WebCore::FormState*, WebCore::FramePolicyFunction&&) final;
+    void dispatchDecidePolicyForNavigationAction(const WebCore::NavigationAction&, const WebCore::ResourceRequest&, bool didReceiveRedirectResponse, WebCore::FormState*, WebCore::PolicyDecisionMode, WebCore::FramePolicyFunction&&) final;
     void didDecidePolicyForNavigationAction() final;
     void cancelPolicyCheck() final;
     
index f6bcffab9c2f4620fc2ab0ce697dee18a32cf01f..64a5907639412753726abf9e35969ef65721fb01 100644 (file)
@@ -1,3 +1,17 @@
+2018-04-17  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION (r229831): CMD-clicking an iCloud web app link unexpectedly opens that link in a new tab and the current tab
+        https://bugs.webkit.org/show_bug.cgi?id=184678
+        <rdar://problem/39422122>
+
+        Reviewed by Alex Christensen.
+
+        Add new parameter to dispatchDecidePolicyForNavigationAction.
+
+        * WebCoreSupport/WebFrameLoaderClient.h:
+        * WebCoreSupport/WebFrameLoaderClient.mm:
+        (WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
+
 2018-04-13  Chris Dumez  <cdumez@apple.com>
 
         Split WindowProxy handling out of ScriptController and into a new class owned by AbstractFrame
index c3b499a8c58a5d435ece72cf1490fdbe5bba989e..a2c7932f68269976c2933694c3eedc6029652dd1 100644 (file)
@@ -127,7 +127,7 @@ private:
 
     void dispatchDecidePolicyForResponse(const WebCore::ResourceResponse&, const WebCore::ResourceRequest&, WebCore::FramePolicyFunction&&) final;
     void dispatchDecidePolicyForNewWindowAction(const WebCore::NavigationAction&, const WebCore::ResourceRequest&, WebCore::FormState*, const WTF::String& frameName, WebCore::FramePolicyFunction&&) final;
-    void dispatchDecidePolicyForNavigationAction(const WebCore::NavigationAction&, const WebCore::ResourceRequest&, bool didReceiveRedirectResponse, WebCore::FormState*, WebCore::FramePolicyFunction&&) final;
+    void dispatchDecidePolicyForNavigationAction(const WebCore::NavigationAction&, const WebCore::ResourceRequest&, bool didReceiveRedirectResponse, WebCore::FormState*, WebCore::PolicyDecisionMode, WebCore::FramePolicyFunction&&) final;
     void cancelPolicyCheck() final;
 
     void dispatchUnableToImplementPolicy(const WebCore::ResourceError&) final;
index bbcda784ba1d8c503b83b0033eef9cb5ab216226..1a411348a8eb6c77d87edafcdc49ed1859b5f616 100644 (file)
@@ -900,7 +900,7 @@ void WebFrameLoaderClient::dispatchDecidePolicyForNewWindowAction(const Navigati
                           decisionListener:setUpPolicyListener(WTFMove(function), tryAppLink ? (NSURL *)request.url() : nil).get()];
 }
 
-void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const NavigationAction& action, const ResourceRequest& request, bool, FormState* formState, FramePolicyFunction&& function)
+void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const NavigationAction& action, const ResourceRequest& request, bool, FormState* formState, PolicyDecisionMode, FramePolicyFunction&& function)
 {
     WebView *webView = getWebView(m_webFrame.get());
     BOOL tryAppLink = shouldTryAppLink(webView, action, core(m_webFrame.get()));
index 3018ff6915596b4d3d58aa0d6f959782310a1cc9..933d040d6c3955a3c1d35931b3eef1476b238cb1 100644 (file)
@@ -1,3 +1,17 @@
+2018-04-17  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION (r229831): CMD-clicking an iCloud web app link unexpectedly opens that link in a new tab and the current tab
+        https://bugs.webkit.org/show_bug.cgi?id=184678
+        <rdar://problem/39422122>
+
+        Reviewed by Alex Christensen.
+
+        Add new parameter to dispatchDecidePolicyForNavigationAction.
+
+        * WebCoreSupport/WebFrameLoaderClient.cpp:
+        (WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction):
+        * WebCoreSupport/WebFrameLoaderClient.h:
+
 2018-04-12  Antoine Quint  <graouts@apple.com>
 
         [Web Animations] Turn CSS Animations and CSS Transitions as Web Animations on by default
index 27bedfc8ddd97cfb5c2e0a4b5f13dce3aad56bc9..c9736b1c8cd3c4f9f3e10d50e5cbb2cbdd2c4b10 100644 (file)
@@ -565,7 +565,7 @@ void WebFrameLoaderClient::dispatchDecidePolicyForNewWindowAction(const Navigati
     m_policyListenerPrivate->m_policyFunction(PolicyAction::Use);
 }
 
-void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const NavigationAction& action, const ResourceRequest& request, bool, FormState* formState, FramePolicyFunction&& function)
+void WebFrameLoaderClient::dispatchDecidePolicyForNavigationAction(const NavigationAction& action, const ResourceRequest& request, bool, FormState* formState, WebCore::PolicyDecisionMode, WebCore::PolicyDecisionMode, FramePolicyFunction&& function)
 {
     WebView* webView = m_webFrame->webView();
     Frame* coreFrame = core(m_webFrame);
index 9b8333bc1ab9967b6f7e39a25b754ff34980862f..1a03106f3974fb03c05dbecd290fd76ee1875806 100644 (file)
@@ -102,7 +102,7 @@ public:
 
     void dispatchDecidePolicyForResponse(const WebCore::ResourceResponse&, const WebCore::ResourceRequest&, WebCore::FramePolicyFunction&&) override;
     void dispatchDecidePolicyForNewWindowAction(const WebCore::NavigationAction&, const WebCore::ResourceRequest&, WebCore::FormState*, const WTF::String& frameName, WebCore::FramePolicyFunction&&) override;
-    void dispatchDecidePolicyForNavigationAction(const WebCore::NavigationAction&, const WebCore::ResourceRequest&, bool didReceiveRedirectResponse, WebCore::FormState*, WebCore::FramePolicyFunction&&) override;
+    void dispatchDecidePolicyForNavigationAction(const WebCore::NavigationAction&, const WebCore::ResourceRequest&, bool didReceiveRedirectResponse, WebCore::FormState*, WebCore::PolicyDecisionMode, WebCore::FramePolicyFunction&&) override;
     void cancelPolicyCheck() override;
 
     void dispatchUnableToImplementPolicy(const WebCore::ResourceError&) override;