- Revision
- 223413
- Author
- [email protected]
- Date
- 2017-10-16 09:59:47 -0700 (Mon, 16 Oct 2017)
Log Message
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.
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.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (223412 => 223413)
--- trunk/LayoutTests/ChangeLog 2017-10-16 16:40:48 UTC (rev 223412)
+++ trunk/LayoutTests/ChangeLog 2017-10-16 16:59:47 UTC (rev 223413)
@@ -1,3 +1,19 @@
+2017-10-16 Chris Dumez <[email protected]>
+
+ 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 <[email protected]>
Rebaseline imported/w3c/web-platform-tests/beacon/headers/header-content-type.html for macOS.
Added: trunk/LayoutTests/http/tests/download/anchor-load-after-download-expected.txt (0 => 223413)
--- trunk/LayoutTests/http/tests/download/anchor-load-after-download-expected.txt (rev 0)
+++ trunk/LayoutTests/http/tests/download/anchor-load-after-download-expected.txt 2017-10-16 16:59:47 UTC (rev 223413)
@@ -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.
Added: trunk/LayoutTests/http/tests/download/anchor-load-after-download.html (0 => 223413)
--- trunk/LayoutTests/http/tests/download/anchor-load-after-download.html (rev 0)
+++ trunk/LayoutTests/http/tests/download/anchor-load-after-download.html 2017-10-16 16:59:47 UTC (rev 223413)
@@ -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='' download="test.html">Link with download attribute.</a>
+<a id="non-download-url" href="" 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>
Modified: trunk/LayoutTests/imported/w3c/ChangeLog (223412 => 223413)
--- trunk/LayoutTests/imported/w3c/ChangeLog 2017-10-16 16:40:48 UTC (rev 223412)
+++ trunk/LayoutTests/imported/w3c/ChangeLog 2017-10-16 16:59:47 UTC (rev 223413)
@@ -1,3 +1,17 @@
+2017-10-16 Chris Dumez <[email protected]>
+
+ 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 <[email protected]>
Resync tests up to c1716b039411090428e7073158b1aea081dafe71
Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt (223412 => 223413)
--- trunk/LayoutTests/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt 2017-10-16 16:40:48 UTC (rev 223412)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt 2017-10-16 16:59:47 UTC (rev 223413)
@@ -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
Modified: trunk/LayoutTests/platform/ios-wk2/TestExpectations (223412 => 223413)
--- trunk/LayoutTests/platform/ios-wk2/TestExpectations 2017-10-16 16:40:48 UTC (rev 223412)
+++ trunk/LayoutTests/platform/ios-wk2/TestExpectations 2017-10-16 16:59:47 UTC (rev 223413)
@@ -1816,6 +1816,7 @@
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 ]
Modified: trunk/LayoutTests/platform/mac-wk1/TestExpectations (223412 => 223413)
--- trunk/LayoutTests/platform/mac-wk1/TestExpectations 2017-10-16 16:40:48 UTC (rev 223412)
+++ trunk/LayoutTests/platform/mac-wk1/TestExpectations 2017-10-16 16:59:47 UTC (rev 223413)
@@ -251,6 +251,7 @@
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 ]
Copied: trunk/LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt (from rev 223412, trunk/LayoutTests/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt) (0 => 223413)
--- trunk/LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt (rev 0)
+++ trunk/LayoutTests/platform/mac-wk1/imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-window-name-expected.txt 2017-10-16 16:59:47 UTC (rev 223413)
@@ -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
+
Modified: trunk/Source/WebKit/ChangeLog (223412 => 223413)
--- trunk/Source/WebKit/ChangeLog 2017-10-16 16:40:48 UTC (rev 223412)
+++ trunk/Source/WebKit/ChangeLog 2017-10-16 16:59:47 UTC (rev 223413)
@@ -1,3 +1,30 @@
+2017-10-16 Chris Dumez <[email protected]>
+
+ 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 <[email protected]>
Unreviewed, rolling out r223271.
Modified: trunk/Source/WebKit/UIProcess/API/APINavigation.h (223412 => 223413)
--- trunk/Source/WebKit/UIProcess/API/APINavigation.h 2017-10-16 16:40:48 UTC (rev 223412)
+++ trunk/Source/WebKit/UIProcess/API/APINavigation.h 2017-10-16 16:59:47 UTC (rev 223413)
@@ -59,6 +59,9 @@
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 @@
WebCore::ResourceRequest m_request;
Vector<WebCore::URL> m_redirectChain;
bool m_wasUserInitiated { true };
+ bool m_shouldForceDownload { false };
};
} // namespace API
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.cpp (223412 => 223413)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2017-10-16 16:40:48 UTC (rev 223412)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.cpp 2017-10-16 16:59:47 UTC (rev 223413)
@@ -2285,10 +2285,8 @@
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 = ""
-#endif
DownloadID downloadID = { };
if (action == PolicyAction::Download) {
@@ -3695,10 +3693,12 @@
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 @@
m_inDecidePolicyForNavigationAction = true;
m_syncNavigationActionPolicyActionIsValid = false;
-#if ENABLE(DOWNLOAD_ATTRIBUTE)
- m_syncNavigationActionHasDownloadAttribute = !navigationActionData.downloadAttribute.isNull();
-#endif
WebFrameProxy* originatingFrame = m_process->webFrame(originatingFrameInfoData.frameID);
Modified: trunk/Source/WebKit/UIProcess/WebPageProxy.h (223412 => 223413)
--- trunk/Source/WebKit/UIProcess/WebPageProxy.h 2017-10-16 16:40:48 UTC (rev 223412)
+++ trunk/Source/WebKit/UIProcess/WebPageProxy.h 2017-10-16 16:59:47 UTC (rev 223413)
@@ -2003,10 +2003,6 @@
bool m_isPointerLocked { false };
#endif
-#if ENABLE(DOWNLOAD_ATTRIBUTE)
- bool m_syncNavigationActionHasDownloadAttribute { false };
-#endif
-
bool m_isUsingHighPerformanceWebGL { false };
WeakPtrFactory<WebPageProxy> m_weakPtrFactory;
Modified: trunk/Tools/ChangeLog (223412 => 223413)
--- trunk/Tools/ChangeLog 2017-10-16 16:40:48 UTC (rev 223412)
+++ trunk/Tools/ChangeLog 2017-10-16 16:59:47 UTC (rev 223413)
@@ -1,3 +1,19 @@
+2017-10-16 Chris Dumez <[email protected]>
+
+ 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 <[email protected]>
Add Emilio Cobos Álvarez to the contributors list.
Modified: trunk/Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp (223412 => 223413)
--- trunk/Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp 2017-10-16 16:40:48 UTC (rev 223412)
+++ trunk/Tools/WebKitTestRunner/InjectedBundle/InjectedBundlePage.cpp 2017-10-16 16:59:47 UTC (rev 223413)
@@ -1358,7 +1358,7 @@
WKBundlePagePolicyAction InjectedBundlePage::decidePolicyForNewWindowAction(WKBundlePageRef, WKBundleFrameRef, WKBundleNavigationActionRef, WKURLRequestRef, WKStringRef, WKTypeRef*)
{
- return WKBundlePagePolicyActionUse;
+ return WKBundlePagePolicyActionPassThrough;
}
WKBundlePagePolicyAction InjectedBundlePage::decidePolicyForResponse(WKBundlePageRef page, WKBundleFrameRef, WKURLResponseRef response, WKURLRequestRef, WKTypeRef*)
