Clicks on Link with download attribute causes all (other) links to tr…

…igger download when clicked https://bugs.webkit.org/show_bug.cgi?id=178267 <rdar://problem/34985016> Reviewed by Darin Adler. LayoutTests/imported/w3c: Rebaseline test which behave differently now in WebKit2 due to WKTR's injected bundle using PassThrough policy for new windows. The new result is identical to what you would get when you open the test in Safari so I think this is a good thing. * web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt: Source/WebKit: When clicking on an anchor with the download attribute, the m_syncNavigationActionHasDownloadAttribute flag on WebPageProxy would get set. This flag would not get reset right away and instead, it would get updated during the next call to WebPageProxy::decidePolicyForNavigationAction(). The issue is that if you later click on a link with target="_blank", WebPageProxy::decidePolicyForNewWindowAction() gets called instead of WebPageProxy::decidePolicyForNavigationAction() and we do not reset the m_syncNavigationActionHasDownloadAttribute flag and we force a download. To address the problem, I got rid of this flag on WebPageProxy and it is error-prone and should really not be at page-level. Instead, I added a shouldForceDownload flag on the navigation object. It makes more sense to associate the flag with the navigation and makes it less error-prone. * UIProcess/API/APINavigation.h: (API::Navigation::setShouldForceDownload): (API::Navigation::shouldForceDownload const): * UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::receivedPolicyDecision): (WebKit::WebPageProxy::decidePolicyForNavigationAction): * UIProcess/WebPageProxy.h: Tools: Use PassThrough policy in WKTR's InjectedBundle's decidePolicyForNewWindowAction so that the request is sent to the UIProcess. This gets WKTR's closer to Safari behavior and helps reproduce the bug. Without this change, I would not be able to write a regression test for this bug that is very easily reproducible in Safari. * WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp: (WTR::InjectedBundlePage::decidePolicyForNewWindowAction): LayoutTests: Add layout test coverage. * http/tests/download/anchor-load-after-download-expected.txt: Added. * http/tests/download/anchor-load-after-download.html: Added. * platform/ios-wk2/TestExpectations: * platform/mac-wk1/TestExpectations: * platform/mac-wk1/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt: Copied from LayoutTests/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt. Canonical link: https://commits.webkit.org/194558@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@223413 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Oct 16, 2017 · 65be2ef07501940edeffe08837253ede9cc86568 · 65be2ef
1 parent 33d7007
commit 65be2ef07501940edeffe08837253ede9cc86568
diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
@@ -1,3 +1,19 @@
+2017-10-16  Chris Dumez  <cdumez@apple.com>
+
+        Clicks on Link with download attribute causes all (other) links to trigger download when clicked
+        https://bugs.webkit.org/show_bug.cgi?id=178267
+        <rdar://problem/34985016>
+
+        Reviewed by Darin Adler.
+
+        Add layout test coverage.
+
+        * http/tests/download/anchor-load-after-download-expected.txt: Added.
+        * http/tests/download/anchor-load-after-download.html: Added.
+        * platform/ios-wk2/TestExpectations:
+        * platform/mac-wk1/TestExpectations:
+        * platform/mac-wk1/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt: Copied from LayoutTests/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt.
+
 2017-10-16  Ryan Haddad  <ryanhaddad@apple.com>
 
         Rebaseline imported/w3c/web-platform-tests/beacon/headers/header-content-type.html for macOS.

diff --git a/LayoutTests/http/tests/download/anchor-load-after-download-expected.txt b/LayoutTests/http/tests/download/anchor-load-after-download-expected.txt
@@ -0,0 +1,5 @@
+Clicking the download link link and then the non-download link should not cause a second download.
+
+This test passes if it does not time out.
+
+Link with download attribute. Link without download attribute.
diff --git a/LayoutTests/http/tests/download/anchor-load-after-download.html b/LayoutTests/http/tests/download/anchor-load-after-download.html
@@ -0,0 +1,29 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.testRunner) {
+  testRunner.dumpAsText();
+  testRunner.waitUntilDone();
+}
+</script>
+</head>
+<body>
+<p>Clicking the download link link and then the non-download link should not cause a second download.</p>
+<p>This test passes if it does not time out.</p>
+<a id="download-url" href='data:text/html,HelloWorld!' download="test.html">Link with download attribute.</a>
+<a id="non-download-url" href="/misc/resources/success-notify-done.html" target="_blank">Link without download attribute.</a>
+<script>
+function runTest()
+{
+    var downloadLink = document.getElementById("download-url");
+    var nonDownloadLink = document.getElementById("non-download-url");
+    downloadLink.click();
+    setTimeout(function() {
+        nonDownloadLink.click();
+    }, 0);
+}
+runTest();
+</script>
+</body>
+</html>
diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
@@ -1,3 +1,17 @@
+2017-10-16  Chris Dumez  <cdumez@apple.com>
+
+        Clicks on Link with download attribute causes all (other) links to trigger download when clicked
+        https://bugs.webkit.org/show_bug.cgi?id=178267
+        <rdar://problem/34985016>
+
+        Reviewed by Darin Adler.
+
+        Rebaseline test which behave differently now in WebKit2 due to WKTR's injected bundle
+        using PassThrough policy for new windows. The new result is identical to what you
+        would get when you open the test in Safari so I think this is a good thing.
+
+        * web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt:
+
 2017-10-14  Youenn Fablet  <youenn@apple.com>
 
         Resync tests up to c1716b039411090428e7073158b1aea081dafe71

diff --git a/...imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt b/...imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt
@@ -1,13 +1,7 @@
-CONSOLE MESSAGE: line 37: Unsafe JavaScript attempt to initiate navigation for frame with URL '' from frame with URL 'http://localhost:8800/html/browsers/windows/noreferrer-window-name.html'. The frame attempting navigation is neither same-origin with the target, nor is it the target's parent or opener.
-
-CONSOLE MESSAGE: line 38: Unsafe JavaScript attempt to initiate navigation for frame with URL '' from frame with URL 'http://localhost:8800/html/browsers/windows/noreferrer-window-name.html'. The frame attempting navigation is neither same-origin with the target, nor is it the target's parent or opener.
-
-CONSOLE MESSAGE: line 38: Unsafe JavaScript attempt to initiate navigation for frame with URL '' from frame with URL 'http://localhost:8800/html/browsers/windows/noreferrer-window-name.html'. The frame attempting navigation is neither same-origin with the target, nor is it the target's parent or opener.
-
 
 Harness Error (TIMEOUT), message = null
 
-PASS Following a noreferrer link with a named target should not cause creation of a window that can be targeted by another noreferrer link with the same named target 
+TIMEOUT Following a noreferrer link with a named target should not cause creation of a window that can be targeted by another noreferrer link with the same named target Test timed out
 PASS Targeting a rel=noreferrer link at an existing named subframe should work 
 TIMEOUT Targeting a rel=noreferrer link at an existing named window should work Test timed out
 
diff --git a/LayoutTests/platform/ios-wk2/TestExpectations b/LayoutTests/platform/ios-wk2/TestExpectations
@@ -1816,6 +1816,7 @@ webkit.org/b/156067 fast/dom/HTMLAnchorElement/anchor-download-user-triggered-sy
 webkit.org/b/156067 http/tests/download/anchor-download-attribute-content-disposition.html [ Skip ]
 webkit.org/b/156067 http/tests/download/anchor-download-no-extension.html [ Skip ]
 webkit.org/b/156067 http/tests/download/anchor-download-redirect.html [ Skip ]
+webkit.org/b/156067 http/tests/download/anchor-load-after-download.html [ Skip ]
 webkit.org/b/156067 http/tests/download/area-download.html [ Skip ]
 webkit.org/b/156067 http/tests/security/anchor-download-allow-blob.html [ Skip ]
 webkit.org/b/156067 http/tests/security/anchor-download-allow-data.html [ Skip ]

diff --git a/LayoutTests/platform/mac-wk1/TestExpectations b/LayoutTests/platform/mac-wk1/TestExpectations
@@ -251,6 +251,7 @@ webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-file-blob-download-no-exte
 webkit.org/b/156069 http/tests/download/anchor-download-attribute-content-disposition.html [ Skip ]
 webkit.org/b/156069 http/tests/download/anchor-download-no-extension.html [ Skip ]
 webkit.org/b/156069 http/tests/download/anchor-download-redirect.html [ Skip ]
+webkit.org/b/156069 http/tests/download/anchor-load-after-download.html [ Skip ]
 webkit.org/b/156069 http/tests/download/area-download.html [ Skip ]
 webkit.org/b/156069 http/tests/security/anchor-download-allow-blob.html [ Skip ]
 webkit.org/b/156069 http/tests/security/anchor-download-allow-data.html [ Skip ]

diff --git a/...imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt b/...imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt
@@ -0,0 +1,13 @@
+CONSOLE MESSAGE: line 37: Unsafe JavaScript attempt to initiate navigation for frame with URL '' from frame with URL 'http://localhost:8800/html/browsers/windows/noreferrer-window-name.html'. The frame attempting navigation is neither same-origin with the target, nor is it the target's parent or opener.
+
+CONSOLE MESSAGE: line 38: Unsafe JavaScript attempt to initiate navigation for frame with URL '' from frame with URL 'http://localhost:8800/html/browsers/windows/noreferrer-window-name.html'. The frame attempting navigation is neither same-origin with the target, nor is it the target's parent or opener.
+
+CONSOLE MESSAGE: line 38: Unsafe JavaScript attempt to initiate navigation for frame with URL '' from frame with URL 'http://localhost:8800/html/browsers/windows/noreferrer-window-name.html'. The frame attempting navigation is neither same-origin with the target, nor is it the target's parent or opener.
+
+
+Harness Error (TIMEOUT), message = null
+
+PASS Following a noreferrer link with a named target should not cause creation of a window that can be targeted by another noreferrer link with the same named target 
+PASS Targeting a rel=noreferrer link at an existing named subframe should work 
+TIMEOUT Targeting a rel=noreferrer link at an existing named window should work Test timed out
+
diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
@@ -1,3 +1,30 @@
+2017-10-16  Chris Dumez  <cdumez@apple.com>
+
+        Clicks on Link with download attribute causes all (other) links to trigger download when clicked
+        https://bugs.webkit.org/show_bug.cgi?id=178267
+        <rdar://problem/34985016>
+
+        Reviewed by Darin Adler.
+
+        When clicking on an anchor with the download attribute, the m_syncNavigationActionHasDownloadAttribute
+        flag on WebPageProxy would get set. This flag would not get reset right away and instead, it would get
+        updated during the next call to WebPageProxy::decidePolicyForNavigationAction(). The issue is that if
+        you later click on a link with target="_blank", WebPageProxy::decidePolicyForNewWindowAction() gets
+        called instead of WebPageProxy::decidePolicyForNavigationAction() and we do not reset the
+        m_syncNavigationActionHasDownloadAttribute flag and we force a download.
+
+        To address the problem, I got rid of this flag on WebPageProxy and it is error-prone and should really
+        not be at page-level. Instead, I added a shouldForceDownload flag on the navigation object. It makes
+        more sense to associate the flag with the navigation and makes it less error-prone.
+
+        * UIProcess/API/APINavigation.h:
+        (API::Navigation::setShouldForceDownload):
+        (API::Navigation::shouldForceDownload const):
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::receivedPolicyDecision):
+        (WebKit::WebPageProxy::decidePolicyForNavigationAction):
+        * UIProcess/WebPageProxy.h:
+
 2017-10-16  Ryan Haddad  <ryanhaddad@apple.com>
 
         Unreviewed, rolling out r223271.

diff --git a/Source/WebKit/UIProcess/API/APINavigation.h b/Source/WebKit/UIProcess/API/APINavigation.h
@@ -59,6 +59,9 @@ class Navigation : public ObjectImpl<Object::Type::Navigation> {
     void setWasUserInitiated(bool value) { m_wasUserInitiated = value; }
     bool wasUserInitiated() const { return m_wasUserInitiated; }
 
+    void setShouldForceDownload(bool value) { m_shouldForceDownload = value; }
+    bool shouldForceDownload() const { return m_shouldForceDownload; }
+
 private:
     explicit Navigation(WebKit::WebNavigationState&);
     explicit Navigation(WebKit::WebNavigationState&, WebCore::ResourceRequest&&);
@@ -67,6 +70,7 @@ class Navigation : public ObjectImpl<Object::Type::Navigation> {
     WebCore::ResourceRequest m_request;
     Vector<WebCore::URL> m_redirectChain;
     bool m_wasUserInitiated { true };
+    bool m_shouldForceDownload { false };
 };
 
 } // namespace API
diff --git a/Source/WebKit/UIProcess/WebPageProxy.cpp b/Source/WebKit/UIProcess/WebPageProxy.cpp
@@ -2285,10 +2285,8 @@ void WebPageProxy::receivedPolicyDecision(PolicyAction action, WebFrameProxy& fr
     if (action == PolicyAction::Ignore)
         m_pageLoadState.clearPendingAPIRequestURL(transaction);
 
-#if ENABLE(DOWNLOAD_ATTRIBUTE)
-    if (m_syncNavigationActionHasDownloadAttribute && action == PolicyAction::Use)
+    if (navigation && navigation->shouldForceDownload() && action == PolicyAction::Use)
         action = PolicyAction::Download;
-#endif
 
     DownloadID downloadID = { };
     if (action == PolicyAction::Download) {
@@ -3695,10 +3693,12 @@ void WebPageProxy::decidePolicyForNavigationAction(uint64_t frameID, const Secur
         auto navigation = m_navigationState->createLoadRequestNavigation(ResourceRequest(request));
         newNavigationID = navigation->navigationID();
         navigation->setWasUserInitiated(!!navigationActionData.userGestureTokenIdentifier);
+        navigation->setShouldForceDownload(!navigationActionData.downloadAttribute.isNull());
         listener->setNavigation(WTFMove(navigation));
     } else {
         auto& navigation = m_navigationState->navigation(navigationID);
         navigation.setWasUserInitiated(!!navigationActionData.userGestureTokenIdentifier);
+        navigation.setShouldForceDownload(!navigationActionData.downloadAttribute.isNull());
         listener->setNavigation(navigation);
     }
 
@@ -3714,9 +3714,6 @@ void WebPageProxy::decidePolicyForNavigationAction(uint64_t frameID, const Secur
 
     m_inDecidePolicyForNavigationAction = true;
     m_syncNavigationActionPolicyActionIsValid = false;
-#if ENABLE(DOWNLOAD_ATTRIBUTE)
-    m_syncNavigationActionHasDownloadAttribute = !navigationActionData.downloadAttribute.isNull();
-#endif
 
     WebFrameProxy* originatingFrame = m_process->webFrame(originatingFrameInfoData.frameID);
 

diff --git a/Source/WebKit/UIProcess/WebPageProxy.h b/Source/WebKit/UIProcess/WebPageProxy.h
@@ -2003,10 +2003,6 @@ class WebPageProxy : public API::ObjectImpl<API::Object::Type::Page>
     bool m_isPointerLocked { false };
 #endif
 
-#if ENABLE(DOWNLOAD_ATTRIBUTE)
-    bool m_syncNavigationActionHasDownloadAttribute { false };
-#endif
-
     bool m_isUsingHighPerformanceWebGL { false };
 
     WeakPtrFactory<WebPageProxy> m_weakPtrFactory;

diff --git a/Tools/ChangeLog b/Tools/ChangeLog
@@ -1,3 +1,19 @@
+2017-10-16  Chris Dumez  <cdumez@apple.com>
+
+        Clicks on Link with download attribute causes all (other) links to trigger download when clicked
+        https://bugs.webkit.org/show_bug.cgi?id=178267
+        <rdar://problem/34985016>
+
+        Reviewed by Darin Adler.
+
+        Use PassThrough policy in WKTR's InjectedBundle's decidePolicyForNewWindowAction so that the
+        request is sent to the UIProcess. This gets WKTR's closer to Safari behavior and helps
+        reproduce the bug. Without this change, I would not be able to write a regression test for
+        this bug that is very easily reproducible in Safari.
+
+        * WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp:
+        (WTR::InjectedBundlePage::decidePolicyForNewWindowAction):
+
 2017-10-16  Emilio Cobos Álvarez  <emilio@crisal.io>
 
         Add Emilio Cobos Álvarez to the contributors list.

diff --git a/Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp b/Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp
@@ -1358,7 +1358,7 @@ WKBundlePagePolicyAction InjectedBundlePage::decidePolicyForNavigationAction(WKB
 
 WKBundlePagePolicyAction InjectedBundlePage::decidePolicyForNewWindowAction(WKBundlePageRef, WKBundleFrameRef, WKBundleNavigationActionRef, WKURLRequestRef, WKStringRef, WKTypeRef*)
 {
-    return WKBundlePagePolicyActionUse;
+    return WKBundlePagePolicyActionPassThrough;
 }
 
 WKBundlePagePolicyAction InjectedBundlePage::decidePolicyForResponse(WKBundlePageRef page, WKBundleFrameRef, WKURLResponseRef response, WKURLRequestRef, WKTypeRef*)