WebKit Bugzilla
Attachment 340141 Details for
Bug 185531
: REGRESSION (async policy delegate): Revoking an object URL immediately after triggering download breaks file download
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-185531-20180510160510.patch (text/plain), 12.26 KB, created by
Chris Dumez
on 2018-05-10 16:05:10 PDT
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Chris Dumez
Created:
2018-05-10 16:05:10 PDT
Size:
12.26 KB
patch
obsolete
>Subversion Revision: 231654 >diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog >index 8c7ef67f825183403d835e517da46ea83a49b4d0..8837b98b8eb21af7fdf00f63fe04490e22c262be 100644 >--- a/Source/WebCore/ChangeLog >+++ b/Source/WebCore/ChangeLog >@@ -1,3 +1,29 @@ >+2018-05-10 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 NOBODY (OOPS!). >+ >+ When a navigation to a blob URL is attempted, we now make sure the Blob is preserved >+ at least until the asynchronous navigation policy decision comes back. Otherwise, if >+ the JS revokes the URL right after scheduling the navigation, the navigation / download >+ will fail. >+ >+ Test: fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html >+ >+ * fileapi/ThreadableBlobRegistry.cpp: >+ (WebCore::ThreadableBlobRegistry::unregisterBlobURL): >+ * loader/PolicyChecker.cpp: >+ (WebCore::PolicyChecker::checkNavigationPolicy): >+ * platform/network/BlobRegistry.cpp: >+ (WebCore::BlobRegistry::unregisterBlobURLWhenPossible): >+ (WebCore::BlobRegistry::extendLifetime): >+ (WebCore::BlobRegistry::LifetimeExtension::LifetimeExtension): >+ (WebCore::BlobRegistry::LifetimeExtension::lifeTimeExtensionExpired): >+ * platform/network/BlobRegistry.h: >+ > 2018-05-10 Chris Dumez <cdumez@apple.com> > > 'Cross-Origin-Options header implementation follow-up >diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog >index 73ea77bcf106b446328624db550349b2aceafc04..e1464fb7f582bed7a357f59bea8a515c31c026b1 100644 >--- a/Source/WebKit/ChangeLog >+++ b/Source/WebKit/ChangeLog >@@ -1,3 +1,15 @@ >+2018-05-10 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 NOBODY (OOPS!). >+ >+ * NetworkProcess/FileAPI/NetworkBlobRegistry.cpp: >+ (WebKit::NetworkBlobRegistry::unregisterBlobURL): >+ (WebKit::NetworkBlobRegistry::connectionToWebProcessDidClose): >+ > 2018-05-10 Chris Dumez <cdumez@apple.com> > > 'Cross-Origin-Options header implementation follow-up >diff --git a/Source/WebCore/fileapi/ThreadableBlobRegistry.cpp b/Source/WebCore/fileapi/ThreadableBlobRegistry.cpp >index b6c57a31d8c2b5133ebbc938d349201640127893..83474d99faae459369cdf013d00724a9a3c72cd2 100644 >--- a/Source/WebCore/fileapi/ThreadableBlobRegistry.cpp >+++ b/Source/WebCore/fileapi/ThreadableBlobRegistry.cpp >@@ -161,10 +161,10 @@ void ThreadableBlobRegistry::unregisterBlobURL(const URL& url) > originMap()->remove(url.string()); > > if (isMainThread()) >- blobRegistry().unregisterBlobURL(url); >+ blobRegistry().unregisterBlobURLWhenPossible(url); > else { > callOnMainThread([url = url.isolatedCopy()] { >- blobRegistry().unregisterBlobURL(url); >+ blobRegistry().unregisterBlobURLWhenPossible(url); > }); > } > } >diff --git a/Source/WebCore/loader/PolicyChecker.cpp b/Source/WebCore/loader/PolicyChecker.cpp >index fc2419498384d22bdf6d7a7d926ec53c6b04ad58..0442ce5ea375bb11bb96727fa947d4fc096f061a 100644 >--- a/Source/WebCore/loader/PolicyChecker.cpp >+++ b/Source/WebCore/loader/PolicyChecker.cpp >@@ -31,6 +31,7 @@ > #include "config.h" > #include "PolicyChecker.h" > >+#include "BlobRegistry.h" > #include "ContentFilter.h" > #include "ContentSecurityPolicy.h" > #include "DOMWindow.h" >@@ -147,12 +148,15 @@ void PolicyChecker::checkNavigationPolicy(ResourceRequest&& request, bool didRec > > m_frame.loader().clearProvisionalLoadForPolicyCheck(); > >+ BlobRegistry::LifetimeExtensionToken blobURLLifetimeExtension; >+ if (request.url().protocolIsBlob()) >+ blobURLLifetimeExtension = blobRegistry().extendLifetime(request.url()); >+ > 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 = WTFMove(requestCopy), formState = makeRefPtr(formState), suggestedFilename = WTFMove(suggestedFilename), blobURLLifetimeExtension = WTFMove(blobURLLifetimeExtension)](PolicyAction policyAction) mutable { > m_delegateIsDecidingNavigationPolicy = false; >- > switch (policyAction) { > case PolicyAction::Download: > m_frame.loader().setOriginalURLForDownloadRequest(request); >diff --git a/Source/WebCore/platform/network/BlobRegistry.cpp b/Source/WebCore/platform/network/BlobRegistry.cpp >index 542293ac4223625085fd512d17de50a5a63712ee..42393a3a98c44423b05756dfac582d96374cedad 100644 >--- a/Source/WebCore/platform/network/BlobRegistry.cpp >+++ b/Source/WebCore/platform/network/BlobRegistry.cpp >@@ -39,4 +39,35 @@ BlobRegistry& blobRegistry() > > BlobRegistry::~BlobRegistry() = default; > >+void BlobRegistry::unregisterBlobURLWhenPossible(const URL& url) >+{ >+ if (auto* lifetimeExtension = m_lifetimeExtensions.get(url)) { >+ lifetimeExtension->setShouldUnregisterURLOnExpiration(); >+ return; >+ } >+ unregisterBlobURL(url); >+} >+ >+auto BlobRegistry::extendLifetime(const URL& url) -> LifetimeExtensionToken >+{ >+ return m_lifetimeExtensions.ensure(url, [url] { >+ return std::make_unique<LifetimeExtension>(url); >+ }).iterator->value->request(); >+} >+ >+BlobRegistry::LifetimeExtension::LifetimeExtension(const URL& url) >+ : m_url(url) >+ , m_counter([this](RefCounterEvent) { if (!m_counter.value()) lifeTimeExtensionExpired(); }) >+{ >+} >+ >+void BlobRegistry::LifetimeExtension::lifeTimeExtensionExpired() >+{ >+ if (m_shouldUnregisterURLOnExpiration) >+ blobRegistry().unregisterBlobURL(m_url); >+ >+ ASSERT(blobRegistry().m_lifetimeExtensions.contains(m_url)); >+ blobRegistry().m_lifetimeExtensions.remove(m_url); > } >+ >+} // namespace WebCore >diff --git a/Source/WebCore/platform/network/BlobRegistry.h b/Source/WebCore/platform/network/BlobRegistry.h >index 9ff600cac593d35945c10ea1145374dbc9728013..697e0a6aa97190ec985966747c015c1aa414746d 100644 >--- a/Source/WebCore/platform/network/BlobRegistry.h >+++ b/Source/WebCore/platform/network/BlobRegistry.h >@@ -31,16 +31,18 @@ > > #pragma once > >+#include "URLHash.h" > #include <functional> > #include <wtf/Forward.h> > #include <wtf/Function.h> >+#include <wtf/HashMap.h> >+#include <wtf/RefCounter.h> > > namespace WebCore { > > class BlobDataFileReference; > class BlobPart; > class BlobRegistry; >-class URL; > > WEBCORE_EXPORT BlobRegistry& blobRegistry(); > >@@ -63,7 +65,7 @@ public: > // Negative start and end values select from the end. > virtual void registerBlobURLForSlice(const URL&, const URL& srcURL, long long start, long long end) = 0; > >- virtual void unregisterBlobURL(const URL&) = 0; >+ void unregisterBlobURLWhenPossible(const URL&); > > virtual unsigned long long blobSize(const URL&) = 0; > >@@ -71,8 +73,32 @@ public: > > virtual bool isBlobRegistryImpl() const { return false; } > >+ enum LifetimeExtensionCounterType { }; >+ using LifetimeExtensionCounter = RefCounter<LifetimeExtensionCounterType>; >+ using LifetimeExtensionToken = LifetimeExtensionCounter::Token; >+ LifetimeExtensionToken extendLifetime(const URL&); >+ > protected: > virtual ~BlobRegistry(); >+ >+ virtual void unregisterBlobURL(const URL&) = 0; >+ >+ class LifetimeExtension { >+ public: >+ LifetimeExtension(const URL&); >+ >+ LifetimeExtensionToken request() { return LifetimeExtensionToken { m_counter.count() }; } >+ void setShouldUnregisterURLOnExpiration() { m_shouldUnregisterURLOnExpiration = true; } >+ >+ private: >+ void lifeTimeExtensionExpired(); >+ >+ URL m_url; >+ LifetimeExtensionCounter m_counter; >+ bool m_shouldUnregisterURLOnExpiration { false }; >+ }; >+ >+ HashMap<URL, std::unique_ptr<LifetimeExtension>> m_lifetimeExtensions; > }; > > } // namespace WebCore >diff --git a/Source/WebKit/NetworkProcess/FileAPI/NetworkBlobRegistry.cpp b/Source/WebKit/NetworkProcess/FileAPI/NetworkBlobRegistry.cpp >index f0824d736be2fcbccf1fb01a012bd7db7bffed48..012e0bce52d9ba0460d91f829981daba04c8aa45 100644 >--- a/Source/WebKit/NetworkProcess/FileAPI/NetworkBlobRegistry.cpp >+++ b/Source/WebKit/NetworkProcess/FileAPI/NetworkBlobRegistry.cpp >@@ -121,7 +121,7 @@ void NetworkBlobRegistry::unregisterBlobURL(NetworkConnectionToWebProcess* conne > if (mapIterator == m_blobsForConnection.end()) > return; > >- blobRegistry().unregisterBlobURL(url); >+ blobRegistry().unregisterBlobURLWhenPossible(url); > > ASSERT(mapIterator->value.contains(url)); > mapIterator->value.remove(url); >@@ -166,7 +166,7 @@ void NetworkBlobRegistry::connectionToWebProcessDidClose(NetworkConnectionToWebP > > HashSet<URL>& blobsForConnection = m_blobsForConnection.find(connection)->value; > for (HashSet<URL>::iterator iter = blobsForConnection.begin(), end = blobsForConnection.end(); iter != end; ++iter) >- blobRegistry().unregisterBlobURL(*iter); >+ blobRegistry().unregisterBlobURLWhenPossible(*iter); > > m_blobsForConnection.remove(connection); > } >diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog >index dd871f09cc6f5c49066135edcd7fdf9a65ee7eaa..6287122dc8c39d9cc8fb69a8560546ee5a8145dc 100644 >--- a/LayoutTests/ChangeLog >+++ b/LayoutTests/ChangeLog >@@ -1,3 +1,16 @@ >+2018-05-10 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 NOBODY (OOPS!). >+ >+ Add layout test coverage. >+ >+ * fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke-expected.txt: Added. >+ * fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html: Added. >+ > 2018-05-10 Chris Dumez <cdumez@apple.com> > > 'Cross-Origin-Options header implementation follow-up >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 >index 0000000000000000000000000000000000000000..a3d039017790ca3d7ac361aec9183e9fefed56f6 >--- /dev/null >+++ b/LayoutTests/fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke-expected.txt >@@ -0,0 +1,6 @@ >+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 >index 0000000000000000000000000000000000000000..38ec7806fb5fde414eef27ad1e7248ca4be0d26e >--- /dev/null >+++ b/LayoutTests/fast/dom/HTMLAnchorElement/anchor-file-blob-download-then-revoke.html >@@ -0,0 +1,30 @@ >+<!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); >+} >+runTest(); >+</script> >+</body> >+</html>
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 185531
:
340141
|
340148
|
340150
|
340156
|
340199