Attachment 340141 Details for Bug 185531 – Patch

[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:

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>

Actions: View | Formatted Diff | Diff

Attachments on bug 185531: 340141 | 340148 | 340150 | 340156 | 340199