REGRESSION (async policy delegate): Revoking an object URL immediately after triggeri...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 11 May 2018 19:22:34 +0000 (19:22 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 11 May 2018 19:22:34 +0000 (19:22 +0000)
https://bugs.webkit.org/show_bug.cgi?id=185531
<rdar://problem/39909589>

Reviewed by Geoffrey Garen.

Source/WebCore:

Whenever we start an asynchronous navigation policy decision for a blob URL, create a temporary
blob URL pointing to the same data, and update the request's URL. This way, if the page's JS revokes
the URL during the policy decision, the load will still succeed.

Test: fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html

* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::willSendRequest):
* loader/FrameLoader.cpp:
(WebCore::FrameLoader::loadURL):
(WebCore::FrameLoader::load):
(WebCore::FrameLoader::loadPostRequest):
* loader/PolicyChecker.cpp:
(WebCore::PolicyChecker::extendBlobURLLifetimeIfNecessary const):
(WebCore::PolicyChecker::checkNavigationPolicy):
(WebCore::PolicyChecker::checkNewWindowPolicy):
* loader/PolicyChecker.h:

Source/WTF:

Add a default constructor for CompletionHandlerCallingScope, for convenience.

* wtf/CompletionHandler.h:

LayoutTests:

* fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke-expected.txt: Added.
* fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html: Added.
Add layout test coverage.

* platform/ios-wk1/TestExpectations:
* platform/ios-wk2/TestExpectations:
* platform/mac-wk1/TestExpectations:
* platform/win/TestExpectations:
* platform/wincairo/TestExpectations:
Skip new test on platforms that do not support the download attribute.

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

15 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html [new file with mode: 0644]
LayoutTests/platform/ios-wk1/TestExpectations
LayoutTests/platform/ios-wk2/TestExpectations
LayoutTests/platform/mac-wk1/TestExpectations
LayoutTests/platform/win/TestExpectations
LayoutTests/platform/wincairo/TestExpectations
Source/WTF/ChangeLog
Source/WTF/wtf/CompletionHandler.h
Source/WebCore/ChangeLog
Source/WebCore/loader/DocumentLoader.cpp
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/loader/PolicyChecker.cpp
Source/WebCore/loader/PolicyChecker.h

index 2a97810..1a94747 100644 (file)
@@ -1,3 +1,22 @@
+2018-05-11  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION (async policy delegate): Revoking an object URL immediately after triggering download breaks file download
+        https://bugs.webkit.org/show_bug.cgi?id=185531
+        <rdar://problem/39909589>
+
+        Reviewed by Geoffrey Garen.
+
+        * fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke-expected.txt: Added.
+        * fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html: Added.
+        Add layout test coverage.
+
+        * platform/ios-wk1/TestExpectations:
+        * platform/ios-wk2/TestExpectations:
+        * platform/mac-wk1/TestExpectations:
+        * platform/win/TestExpectations:
+        * platform/wincairo/TestExpectations:
+        Skip new test on platforms that do not support the download attribute.
+
 2018-05-11  Antti Koivisto  <antti@apple.com>
 
         LinkLoader fails to remove CachedResourceClient in some cases
diff --git a/LayoutTests/fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke-expected.txt b/LayoutTests/fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke-expected.txt
new file mode 100644 (file)
index 0000000..d2b6224
--- /dev/null
@@ -0,0 +1,7 @@
+CONSOLE MESSAGE: line 37: PASS: URL was revoked
+Download started.
+Downloading URL with suggested filename "foo.txt"
+Download completed.
+The suggested filename above should be "foo.txt" and the download should succeed.
+
+Memory backed blob URL
diff --git a/LayoutTests/fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html b/LayoutTests/fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html
new file mode 100644 (file)
index 0000000..70723c4
--- /dev/null
@@ -0,0 +1,42 @@
+<!DOCTYPE html>
+<html>
+<head>
+<script>
+if (window.testRunner) {
+  testRunner.dumpAsText();
+  testRunner.setShouldLogDownloadCallbacks(true);
+  testRunner.waitUntilDownloadFinished();
+}
+</script>
+</head>
+<body>
+<p>The suggested filename above should be "foo.txt" and the download should succeed.</p>
+<a id="blob-url" download="foo">Memory backed blob URL</a>
+<script>
+function runTest()
+{
+    file = new File(["foo"], "foo.txt", {
+        type: "text/plain"
+    });
+    var link = document.getElementById("blob-url");
+    link.href = window.URL.createObjectURL(file);
+    link.click();
+    // Revoke the URL right away.
+    window.URL.revokeObjectURL(link.href);
+
+    // Make sure the URL was revoked.
+    xhr = new XMLHttpRequest();
+    xhr.open("GET", link.href, false);
+    try {
+        xhr.send(null);
+    } catch (e) {
+    }
+    if (xhr.status == 200)
+        console.log("FAIL: URL was not revoked");
+    else
+        console.log("PASS: URL was revoked");
+}
+runTest();
+</script>
+</body>
+</html>
index ea68a10..f4aeed2 100644 (file)
@@ -1367,6 +1367,7 @@ webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-file-blob-download-include
 webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-doublequote.html [ Skip ]
 webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes.html [ Skip ]
 webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-unicode.html [ Skip ]
+webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.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 ]
index 5c72faf..b29577e 100644 (file)
@@ -1157,6 +1157,7 @@ webkit.org/b/156067 fast/dom/HTMLAnchorElement/anchor-file-blob-download-include
 webkit.org/b/156067 fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes.html [ Skip ]
 webkit.org/b/156067 fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-unicode.html [ Skip ]
 webkit.org/b/156067 fast/dom/HTMLAnchorElement/anchor-file-blob-download-no-extension.html [ Skip ]
+webkit.org/b/156067 fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html [ Skip ]
 webkit.org/b/156067 fast/dom/HTMLAnchorElement/anchor-nodownload-set.html [ Skip ]
 webkit.org/b/156067 fast/dom/HTMLAnchorElement/anchor-nodownload.html [ Skip ]
 webkit.org/b/156067 fast/dom/HTMLAnchorElement/anchor-download-synthetic-click.html [ Skip ]
index 275d910..b9d6df1 100644 (file)
@@ -301,6 +301,7 @@ webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-file-blob-download-include
 webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes.html [ Skip ]
 webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-unicode.html [ Skip ]
 webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-file-blob-download-no-extension.html [ Skip ]
+webkit.org/b/156069 fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html [ Skip ]
 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 ]
index b3615b2..8159fee 100644 (file)
@@ -446,6 +446,7 @@ fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-doublequote.html [
 fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes.html [ Skip ]
 fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-unicode.html [ Skip ]
 fast/dom/HTMLAnchorElement/anchor-file-blob-download-no-extension.html [ Skip ]
+fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html [ Skip ]
 http/tests/download/anchor-download-attribute-content-disposition.html [ Skip ]
 http/tests/download/anchor-download-no-extension.html [ Skip ]
 http/tests/download/area-download.html [ Skip ]
index dfbb498..93b69bb 100644 (file)
@@ -66,6 +66,7 @@ fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-doublequote.html [
 fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-slashes.html [ Skip ]
 fast/dom/HTMLAnchorElement/anchor-file-blob-download-includes-unicode.html [ Skip ]
 fast/dom/HTMLAnchorElement/anchor-file-blob-download-no-extension.html [ Skip ]
+fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html [ Skip ]
 fast/dom/HTMLAnchorElement/anchor-nodownload-set.html [ Skip ]
 
 # ENABLE_INPUT_TYPE_* are disabled.
index 508bec6..c8274c8 100644 (file)
@@ -1,3 +1,15 @@
+2018-05-11  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION (async policy delegate): Revoking an object URL immediately after triggering download breaks file download
+        https://bugs.webkit.org/show_bug.cgi?id=185531
+        <rdar://problem/39909589>
+
+        Reviewed by Geoffrey Garen.
+
+        Add a default constructor for CompletionHandlerCallingScope, for convenience.
+
+        * wtf/CompletionHandler.h:
+
 2018-05-11  Saam Barati  <sbarati@apple.com>
 
         Don't allocate value profiles when the JIT is disabled
index e02faf1..7a25a5e 100644 (file)
@@ -66,6 +66,8 @@ private:
 
 class CompletionHandlerCallingScope {
 public:
+    CompletionHandlerCallingScope() = default;
+
     CompletionHandlerCallingScope(CompletionHandler<void()>&& completionHandler)
         : m_completionHandler(WTFMove(completionHandler))
     { }
index b6bc4a4..20d7b23 100644 (file)
@@ -1,3 +1,29 @@
+2018-05-11  Chris Dumez  <cdumez@apple.com>
+
+        REGRESSION (async policy delegate): Revoking an object URL immediately after triggering download breaks file download
+        https://bugs.webkit.org/show_bug.cgi?id=185531
+        <rdar://problem/39909589>
+
+        Reviewed by Geoffrey Garen.
+
+        Whenever we start an asynchronous navigation policy decision for a blob URL, create a temporary
+        blob URL pointing to the same data, and update the request's URL. This way, if the page's JS revokes
+        the URL during the policy decision, the load will still succeed.
+
+        Test: fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html
+
+        * loader/DocumentLoader.cpp:
+        (WebCore::DocumentLoader::willSendRequest):
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::loadURL):
+        (WebCore::FrameLoader::load):
+        (WebCore::FrameLoader::loadPostRequest):
+        * loader/PolicyChecker.cpp:
+        (WebCore::PolicyChecker::extendBlobURLLifetimeIfNecessary const):
+        (WebCore::PolicyChecker::checkNavigationPolicy):
+        (WebCore::PolicyChecker::checkNewWindowPolicy):
+        * loader/PolicyChecker.h:
+
 2018-05-11  Antti Koivisto  <antti@apple.com>
 
         LinkLoader fails to remove CachedResourceClient in some cases
index 8352e95..2ef6f49 100644 (file)
@@ -666,7 +666,7 @@ void DocumentLoader::willSendRequest(ResourceRequest&& newRequest, const Resourc
         return;
     }
 
-    frameLoader()->policyChecker().checkNavigationPolicy(ResourceRequest(newRequest), didReceiveRedirectResponse, WTFMove(navigationPolicyCompletionHandler));
+    frameLoader()->policyChecker().checkNavigationPolicy(WTFMove(newRequest), didReceiveRedirectResponse, WTFMove(navigationPolicyCompletionHandler));
 }
 
 bool DocumentLoader::tryLoadingRequestFromApplicationCache()
index 0a5aff2..d7711c3 100644 (file)
@@ -1336,7 +1336,7 @@ void FrameLoader::loadURL(FrameLoadRequest&& frameLoadRequest, const String& ref
 
     if (!targetFrame && !frameName.isEmpty()) {
         action = action.copyWithShouldOpenExternalURLsPolicy(shouldOpenExternalURLsPolicyToApply(m_frame, frameLoadRequest));
-        policyChecker().checkNewWindowPolicy(WTFMove(action), request, formState, frameName, [this, allowNavigationToInvalidURL, openerPolicy, completionHandler = completionHandlerCaller.release()] (const ResourceRequest& request, FormState* formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) {
+        policyChecker().checkNewWindowPolicy(WTFMove(action), WTFMove(request), formState, frameName, [this, allowNavigationToInvalidURL, openerPolicy, completionHandler = completionHandlerCaller.release()] (const ResourceRequest& request, FormState* formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) {
             continueLoadAfterNewWindowPolicy(request, formState, frameName, action, shouldContinue, allowNavigationToInvalidURL, openerPolicy);
             completionHandler();
         });
@@ -1356,7 +1356,7 @@ void FrameLoader::loadURL(FrameLoadRequest&& frameLoadRequest, const String& ref
         oldDocumentLoader->setLastCheckedRequest(ResourceRequest());
         policyChecker().stopCheck();
         policyChecker().setLoadType(newLoadType);
-        policyChecker().checkNavigationPolicy(ResourceRequest(request), false /* didReceiveRedirectResponse */, oldDocumentLoader.get(), formState, [this, protectedFrame = makeRef(m_frame)] (const ResourceRequest& request, FormState*, ShouldContinue shouldContinue) {
+        policyChecker().checkNavigationPolicy(WTFMove(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;
@@ -1412,7 +1412,7 @@ void FrameLoader::load(FrameLoadRequest&& request)
 
     if (request.shouldCheckNewWindowPolicy()) {
         NavigationAction action { request.requester(), request.resourceRequest(), InitiatedByMainFrame::Unknown, NavigationType::Other, request.shouldOpenExternalURLsPolicy() };
-        policyChecker().checkNewWindowPolicy(WTFMove(action), request.resourceRequest(), nullptr, request.frameName(), [this] (const ResourceRequest& request, FormState* formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) {
+        policyChecker().checkNewWindowPolicy(WTFMove(action), WTFMove(request.resourceRequest()), nullptr, request.frameName(), [this] (const ResourceRequest& request, FormState* formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) {
             continueLoadAfterNewWindowPolicy(request, formState, frameName, action, shouldContinue, AllowNavigationToInvalidURL::Yes, NewFrameOpenerPolicy::Suppress);
         });
 
@@ -2858,7 +2858,7 @@ void FrameLoader::loadPostRequest(FrameLoadRequest&& request, const String& refe
             return;
         }
 
-        policyChecker().checkNewWindowPolicy(WTFMove(action), workingResourceRequest, WTFMove(formState), frameName, [this, allowNavigationToInvalidURL, openerPolicy, completionHandler = WTFMove(completionHandler)] (const ResourceRequest& request, FormState* formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) {
+        policyChecker().checkNewWindowPolicy(WTFMove(action), WTFMove(workingResourceRequest), WTFMove(formState), frameName, [this, allowNavigationToInvalidURL, openerPolicy, completionHandler = WTFMove(completionHandler)] (const ResourceRequest& request, FormState* formState, const String& frameName, const NavigationAction& action, ShouldContinue shouldContinue) {
             continueLoadAfterNewWindowPolicy(request, formState, frameName, action, shouldContinue, allowNavigationToInvalidURL, openerPolicy);
             completionHandler();
         });
index fc24194..53d19a4 100644 (file)
@@ -31,6 +31,8 @@
 #include "config.h"
 #include "PolicyChecker.h"
 
+#include "BlobRegistry.h"
+#include "BlobURL.h"
 #include "ContentFilter.h"
 #include "ContentSecurityPolicy.h"
 #include "DOMWindow.h"
@@ -81,6 +83,20 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& newRequest, bool did
     checkNavigationPolicy(WTFMove(newRequest), didReceiveRedirectResponse, m_frame.loader().activeDocumentLoader(), nullptr, WTFMove(function));
 }
 
+CompletionHandlerCallingScope PolicyChecker::extendBlobURLLifetimeIfNecessary(ResourceRequest& request) const
+{
+    if (!request.url().protocolIsBlob())
+        return { };
+
+    // Create a new temporary blobURL in case this one gets revoked during the asynchronous navigation policy decision.
+    URL temporaryBlobURL = BlobURL::createPublicURL(&m_frame.document()->securityOrigin());
+    blobRegistry().registerBlobURL(temporaryBlobURL, request.url());
+    request.setURL(temporaryBlobURL);
+    return CompletionHandler<void()>([temporaryBlobURL = WTFMove(temporaryBlobURL)] {
+        blobRegistry().unregisterBlobURL(temporaryBlobURL);
+    });
+}
+
 void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, bool didReceiveRedirectResponse, DocumentLoader* loader, FormState* formState, NavigationPolicyDecisionFunction&& function, PolicyDecisionMode policyDecisionMode)
 {
     NavigationAction action = loader->triggeringAction();
@@ -147,10 +163,11 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, bool didRec
 
     m_frame.loader().clearProvisionalLoadForPolicyCheck();
 
+    auto blobURLLifetimeExtension = policyDecisionMode == PolicyDecisionMode::Asynchronous ? extendBlobURLLifetimeIfNecessary(request) : CompletionHandlerCallingScope { };
+
     m_delegateIsDecidingNavigationPolicy = true;
     String suggestedFilename = action.downloadAttribute().isEmpty() ? nullAtom() : action.downloadAttribute();
-    ResourceRequest requestCopy = request;
-    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().dispatchDecidePolicyForNavigationAction(action, request, didReceiveRedirectResponse, formState, policyDecisionMode, [this, function = WTFMove(function), request = ResourceRequest(request), formState = makeRefPtr(formState), suggestedFilename = WTFMove(suggestedFilename), blobURLLifetimeExtension = WTFMove(blobURLLifetimeExtension)](PolicyAction policyAction) mutable {
         m_delegateIsDecidingNavigationPolicy = false;
 
         switch (policyAction) {
@@ -173,7 +190,7 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, bool didRec
     });
 }
 
-void PolicyChecker::checkNewWindowPolicy(NavigationAction&& navigationAction, const ResourceRequest& request, FormState* formState, const String& frameName, NewWindowPolicyDecisionFunction&& function)
+void PolicyChecker::checkNewWindowPolicy(NavigationAction&& navigationAction, ResourceRequest&& request, FormState* formState, const String& frameName, NewWindowPolicyDecisionFunction&& function)
 {
     if (m_frame.document() && m_frame.document()->isSandboxed(SandboxPopups))
         return function({ }, nullptr, { }, { }, ShouldContinue::No);
@@ -181,7 +198,9 @@ void PolicyChecker::checkNewWindowPolicy(NavigationAction&& navigationAction, co
     if (!DOMWindow::allowPopUp(m_frame))
         return function({ }, nullptr, { }, { }, ShouldContinue::No);
 
-    m_frame.loader().client().dispatchDecidePolicyForNewWindowAction(navigationAction, request, formState, frameName, [frame = makeRef(m_frame), request, formState = makeRefPtr(formState), frameName, navigationAction, function = WTFMove(function)](PolicyAction policyAction) mutable {
+    auto blobURLLifetimeExtension = extendBlobURLLifetimeIfNecessary(request);
+
+    m_frame.loader().client().dispatchDecidePolicyForNewWindowAction(navigationAction, request, formState, frameName, [frame = makeRef(m_frame), request, formState = makeRefPtr(formState), frameName, navigationAction, function = WTFMove(function), blobURLLifetimeExtension = WTFMove(blobURLLifetimeExtension)](PolicyAction policyAction) mutable {
         switch (policyAction) {
         case PolicyAction::Download:
             frame->loader().client().startDownload(request);
index b16c1ee..c329ec4 100644 (file)
@@ -39,6 +39,7 @@
 
 namespace WTF {
 template<typename> class CompletionHandler;
+class CompletionHandlerCallingScope;
 }
 
 namespace WebCore {
@@ -69,7 +70,7 @@ public:
 
     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&&);
+    void checkNewWindowPolicy(NavigationAction&&, ResourceRequest&&, FormState*, const String& frameName, NewWindowPolicyDecisionFunction&&);
 
     void stopCheck();
 
@@ -87,6 +88,7 @@ public:
 
 private:
     void handleUnimplementablePolicy(const ResourceError&);
+    WTF::CompletionHandlerCallingScope extendBlobURLLifetimeIfNecessary(ResourceRequest&) const;
 
     Frame& m_frame;