CachedResourceLoader should not need to remove fragment identifier

https://bugs.webkit.org/show_bug.cgi?id=163015 Patch by Youenn Fablet <youenn@apple.com> on 2016-10-18 Reviewed by Darin Adler. No expected change for non-window port. For window port, CachedResourceLoader will strip the fragment identifier of the URL passed to subresourceForURL before querying the memory cache. Removing the fragment identifier from the request stored in CachedResourceRequest. The fragment identifier is stored in a separate field. This allows CachedResourceLoader to not care about fragment identifier. CachedResource can then get access to it. * loader/cache/CachedResource.cpp: (WebCore::CachedResource::CachedResource): (WebCore::CachedResource::finishRequestInitialization): Deleted. * loader/cache/CachedResource.h: * loader/cache/CachedResourceLoader.cpp: (WebCore::CachedResourceLoader::cachedResource): Updated the method taking a const String& to strip the fragment identifier if needed. Updated the method taking a const URL& to assert if the fragment identifier is present. (WebCore::CachedResourceLoader::requestUserCSSStyleSheet): (WebCore::CachedResourceLoader::requestResource): * loader/cache/CachedResourceRequest.cpp: (WebCore::CachedResourceRequest::CachedResourceRequest): (WebCore::CachedResourceRequest::splitFragmentIdentifierFromRequestURL): * loader/cache/CachedResourceRequest.h: (WebCore::CachedResourceRequest::releaseFragmentIdentifier): (WebCore::CachedResourceRequest::clearFragmentIdentifier): * loader/cache/MemoryCache.cpp: (WebCore::MemoryCache::shouldRemoveFragmentIdentifier): (WebCore::MemoryCache::removeFragmentIdentifierIfNeeded): (WebCore::MemoryCache::revalidationSucceeded): (WebCore::MemoryCache::resourceForRequest): * loader/cache/MemoryCache.h: Canonical link: https://commits.webkit.org/181372@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@207459 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Oct 18, 2016 · 23a079d · 23a079d
1 parent e46a96b
commit 23a079d
Show file tree

Hide file tree

Showing 8 changed files with 87 additions and 35 deletions.
diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,43 @@
+2016-10-18  Youenn Fablet  <youenn@apple.com>
+
+        CachedResourceLoader should not need to remove fragment identifier
+        https://bugs.webkit.org/show_bug.cgi?id=163015
+
+        Reviewed by Darin Adler.
+
+        No expected change for non-window port.
+        For window port, CachedResourceLoader will strip the fragment identifier of the URL passed to subresourceForURL
+        before querying the memory cache.
+
+        Removing the fragment identifier from the request stored in CachedResourceRequest.
+        The fragment identifier is stored in a separate field.
+
+        This allows CachedResourceLoader to not care about fragment identifier.
+        CachedResource can then get access to it.
+
+        * loader/cache/CachedResource.cpp:
+        (WebCore::CachedResource::CachedResource):
+        (WebCore::CachedResource::finishRequestInitialization): Deleted.
+        * loader/cache/CachedResource.h:
+        * loader/cache/CachedResourceLoader.cpp:
+        (WebCore::CachedResourceLoader::cachedResource):
+        Updated the method taking a const String& to strip the fragment identifier if needed.
+        Updated the method taking a const URL& to assert if the fragment identifier is present.
+        (WebCore::CachedResourceLoader::requestUserCSSStyleSheet):
+        (WebCore::CachedResourceLoader::requestResource):
+        * loader/cache/CachedResourceRequest.cpp:
+        (WebCore::CachedResourceRequest::CachedResourceRequest):
+        (WebCore::CachedResourceRequest::splitFragmentIdentifierFromRequestURL):
+        * loader/cache/CachedResourceRequest.h:
+        (WebCore::CachedResourceRequest::releaseFragmentIdentifier):
+        (WebCore::CachedResourceRequest::clearFragmentIdentifier):
+        * loader/cache/MemoryCache.cpp:
+        (WebCore::MemoryCache::shouldRemoveFragmentIdentifier):
+        (WebCore::MemoryCache::removeFragmentIdentifierIfNeeded):
+        (WebCore::MemoryCache::revalidationSucceeded):
+        (WebCore::MemoryCache::resourceForRequest):
+        * loader/cache/MemoryCache.h:
+
 2016-10-18  Antti Koivisto  <antti@apple.com>
 
         Rename setNeedsStyleRecalc to invalidateStyle

diff --git a/Source/WebCore/loader/cache/CachedResource.cpp b/Source/WebCore/loader/cache/CachedResource.cpp
@@ -121,13 +121,16 @@ CachedResource::CachedResource(CachedResourceRequest&& request, Type type, Sessi
     , m_sessionID(sessionID)
     , m_loadPriority(defaultPriorityForResourceType(type))
     , m_responseTimestamp(std::chrono::system_clock::now())
+    , m_fragmentIdentifierForRequest(request.releaseFragmentIdentifier())
     , m_origin(request.releaseOrigin())
     , m_type(type)
 {
     ASSERT(sessionID.isValid());
 
     setLoadPriority(request.priority());
-    finishRequestInitialization();
+#ifndef NDEBUG
+    cachedResourceLeakCounter.increment();
+#endif
 
     // FIXME: We should have a better way of checking for Navigation loads, maybe FetchMode::Options::Navigate.
     ASSERT(m_origin || m_type == CachedResource::MainResource);
@@ -138,31 +141,20 @@ CachedResource::CachedResource(CachedResourceRequest&& request, Type type, Sessi
         setCrossOrigin();
 }
 
+// FIXME: For this constructor, we should probably mandate that the URL has no fragment identifier.
 CachedResource::CachedResource(const URL& url, Type type, SessionID sessionID)
     : m_resourceRequest(url)
     , m_decodedDataDeletionTimer(*this, &CachedResource::destroyDecodedData, deadDecodedDataDeletionIntervalForResourceType(type))
     , m_sessionID(sessionID)
     , m_responseTimestamp(std::chrono::system_clock::now())
+    , m_fragmentIdentifierForRequest(CachedResourceRequest::splitFragmentIdentifierFromRequestURL(m_resourceRequest))
     , m_type(type)
     , m_status(Cached)
 {
     ASSERT(sessionID.isValid());
-    finishRequestInitialization();
-}
-
-void CachedResource::finishRequestInitialization()
-{
 #ifndef NDEBUG
     cachedResourceLeakCounter.increment();
 #endif
-
-    if (!m_resourceRequest.url().hasFragmentIdentifier())
-        return;
-    URL urlForCache = MemoryCache::removeFragmentIdentifierIfNeeded(m_resourceRequest.url());
-    if (urlForCache.hasFragmentIdentifier())
-        return;
-    m_fragmentIdentifierForRequest = m_resourceRequest.url().fragmentIdentifier();
-    m_resourceRequest.setURL(urlForCache);
 }
 
 CachedResource::~CachedResource()

diff --git a/Source/WebCore/loader/cache/CachedResource.h b/Source/WebCore/loader/cache/CachedResource.h
@@ -302,8 +302,6 @@ class CachedResource {
 private:
     class Callback;
 
-    void finishRequestInitialization();
-
     bool addClientToSet(CachedResourceClient&);
 
     void decodedDataDeletionTimerFired();

diff --git a/Source/WebCore/loader/cache/CachedResourceLoader.cpp b/Source/WebCore/loader/cache/CachedResourceLoader.cpp
@@ -148,17 +148,16 @@ CachedResourceLoader::~CachedResourceLoader()
     ASSERT(m_requestCount == 0);
 }
 
-CachedResource* CachedResourceLoader::cachedResource(const String& resourceURL) const 
+CachedResource* CachedResourceLoader::cachedResource(const String& resourceURL) const
 {
     ASSERT(!resourceURL.isNull());
-    URL url = m_document->completeURL(resourceURL);
-    return cachedResource(url); 
+    return cachedResource(MemoryCache::removeFragmentIdentifierIfNeeded(m_document->completeURL(resourceURL)));
 }
 
-CachedResource* CachedResourceLoader::cachedResource(const URL& resourceURL) const
+CachedResource* CachedResourceLoader::cachedResource(const URL& url) const
 {
-    URL url = MemoryCache::removeFragmentIdentifierIfNeeded(resourceURL);
-    return m_documentResources.get(url).get(); 
+    ASSERT(!MemoryCache::shouldRemoveFragmentIdentifier(url));
+    return m_documentResources.get(url).get();
 }
 
 Frame* CachedResourceLoader::frame() const
@@ -657,9 +656,6 @@ CachedResourceHandle<CachedResource> CachedResourceLoader::requestResource(Cache
 
     LOG(ResourceLoading, "CachedResourceLoader::requestResource '%s', charset '%s', priority=%d, forPreload=%u", url.stringCenterEllipsizedToLength().latin1().data(), request.charset().latin1().data(), request.priority() ? static_cast<int>(request.priority().value()) : -1, forPreload == ForPreload::Yes);
 
-    // If only the fragment identifiers differ, it is the same resource.
-    url = MemoryCache::removeFragmentIdentifierIfNeeded(url);
-
     if (!url.isValid()) {
         RELEASE_LOG_IF_ALLOWED("requestResource: URL is invalid (frame = %p)", frame());
         return nullptr;

diff --git a/Source/WebCore/loader/cache/CachedResourceRequest.cpp b/Source/WebCore/loader/cache/CachedResourceRequest.cpp
@@ -42,9 +42,21 @@ CachedResourceRequest::CachedResourceRequest(ResourceRequest&& resourceRequest,
     , m_charset(WTFMove(charset))
     , m_options(options)
     , m_priority(priority)
+    , m_fragmentIdentifier(splitFragmentIdentifierFromRequestURL(m_resourceRequest))
 {
 }
 
+String CachedResourceRequest::splitFragmentIdentifierFromRequestURL(ResourceRequest& request)
+{
+    if (!MemoryCache::shouldRemoveFragmentIdentifier(request.url()))
+        return { };
+    URL url = request.url();
+    String fragmentIdentifier = url.fragmentIdentifier();
+    url.removeFragmentIdentifier();
+    request.setURL(url);
+    return fragmentIdentifier;
+}
+
 void CachedResourceRequest::setInitiator(PassRefPtr<Element> element)
 {
     ASSERT(!m_initiatorElement && m_initiatorName.isEmpty());

diff --git a/Source/WebCore/loader/cache/CachedResourceRequest.h b/Source/WebCore/loader/cache/CachedResourceRequest.h
@@ -78,6 +78,11 @@ class CachedResourceRequest {
     RefPtr<SecurityOrigin> releaseOrigin() { return WTFMove(m_origin); }
     SecurityOrigin* origin() const { return m_origin.get(); }
 
+    String&& releaseFragmentIdentifier() { return WTFMove(m_fragmentIdentifier); }
+    void clearFragmentIdentifier() { m_fragmentIdentifier = { }; }
+
+    static String splitFragmentIdentifierFromRequestURL(ResourceRequest&);
+
 private:
     ResourceRequest m_resourceRequest;
     String m_charset;
@@ -86,6 +91,7 @@ class CachedResourceRequest {
     RefPtr<Element> m_initiatorElement;
     AtomicString m_initiatorName;
     RefPtr<SecurityOrigin> m_origin;
+    String m_fragmentIdentifier;
 };
 
 } // namespace WebCore

diff --git a/Source/WebCore/loader/cache/MemoryCache.cpp b/Source/WebCore/loader/cache/MemoryCache.cpp
@@ -89,14 +89,19 @@ auto MemoryCache::ensureSessionResourceMap(SessionID sessionID) -> CachedResourc
     return *map;
 }
 
-URL MemoryCache::removeFragmentIdentifierIfNeeded(const URL& originalURL)
+bool MemoryCache::shouldRemoveFragmentIdentifier(const URL& originalURL)
 {
     if (!originalURL.hasFragmentIdentifier())
-        return originalURL;
+        return false;
     // Strip away fragment identifier from HTTP URLs.
-    // Data URLs must be unmodified. For file and custom URLs clients may expect resources 
+    // Data URLs must be unmodified. For file and custom URLs clients may expect resources
     // to be unique even when they differ by the fragment identifier only.
-    if (!originalURL.protocolIsInHTTPFamily())
+    return originalURL.protocolIsInHTTPFamily();
+}
+
+URL MemoryCache::removeFragmentIdentifierIfNeeded(const URL& originalURL)
+{
+    if (!shouldRemoveFragmentIdentifier(originalURL))
         return originalURL;
     URL url = originalURL;
     url.removeFragmentIdentifier();
@@ -154,7 +159,7 @@ void MemoryCache::revalidationSucceeded(CachedResource& revalidatingResource, co
         insertInLiveDecodedResourcesList(resource);
     if (delta)
         adjustSize(resource.hasClients(), delta);
-    
+
     revalidatingResource.switchClientsToRevalidatedResource();
     ASSERT(!revalidatingResource.m_deleted);
     // this deletes the revalidating resource
@@ -171,6 +176,8 @@ void MemoryCache::revalidationFailed(CachedResource& revalidatingResource)
 
 CachedResource* MemoryCache::resourceForRequest(const ResourceRequest& request, SessionID sessionID)
 {
+    // FIXME: Change all clients to make sure HTTP(s) URLs have no fragment identifiers before calling here.
+    // CachedResourceLoader is now doing this. Add an assertion once all other clients are doing it too.
     auto* resources = sessionResourceMap(sessionID);
     if (!resources)
         return nullptr;

diff --git a/Source/WebCore/loader/cache/MemoryCache.h b/Source/WebCore/loader/cache/MemoryCache.h
@@ -97,16 +97,17 @@ class MemoryCache {
     bool add(CachedResource&);
     void remove(CachedResource&);
 
-    static URL removeFragmentIdentifierIfNeeded(const URL& originalURL);
-
+    static bool shouldRemoveFragmentIdentifier(const URL&);
+    static URL removeFragmentIdentifierIfNeeded(const URL&);
+
     void revalidationSucceeded(CachedResource& revalidatingResource, const ResourceResponse&);
     void revalidationFailed(CachedResource& revalidatingResource);
 
     void forEachResource(const std::function<void(CachedResource&)>&);
     void forEachSessionResource(SessionID, const std::function<void(CachedResource&)>&);
     void destroyDecodedDataForAllImages();
-    
-    // Sets the cache's memory capacities, in bytes. These will hold only approximately, 
+
+    // Sets the cache's memory capacities, in bytes. These will hold only approximately,
     // since the decoded cost of resources like scripts and stylesheets is not known.
     //  - minDeadBytes: The maximum number of bytes that dead resources should consume when the cache is under pressure.
     //  - maxDeadBytes: The maximum number of bytes that dead resources should consume when the cache is not under pressure.
@@ -120,7 +121,7 @@ class MemoryCache {
 
     WEBCORE_EXPORT void evictResources();
     WEBCORE_EXPORT void evictResources(SessionID);
-    
+
     void prune();
     void pruneSoon();
     unsigned size() const { return m_liveSize + m_deadSize; }