Attachment 359127 Details for Bug 193429 – Patch

[patch] Patch

bug-193429-20190114205226.patch (text/plain), 15.99 KB, created by Alex Christensen on 2019-01-14 20:52:27 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Alex Christensen

Created: 2019-01-14 20:52:27 PST

Size: 15.99 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 239959)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,30 @@
>+2019-01-14  Alex Christensen  <achristensen@webkit.org>
>+
>+        Split headerValueForVary into specialized functions for NetworkProcess and WebProcess/WebKitLegacy
>+        https://bugs.webkit.org/show_bug.cgi?id=193429
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        headerValueForVary is a strange function that is causing trouble with my NetworkProcess global state removal project.
>+        It currently accesses the cookie storage to see if there's a match in two different ways currently written as fallbacks.
>+        In the WebProcess or in WebKitLegacy, it uses cookiesStrategy to access cookies via IPC or directly, respectively,
>+        depending on the PlatformStrategies implementation of cookiesStrategy for that process.
>+        In the NetworkProcess, it uses WebCore::NetworkStorageSession to access cookies directly.
>+        Both of these cookie accessing methods use global state in the process, and I must split them to refactor them separately.
>+        This patch does the split by passing in the method of cookie access: a CookiesStrategy& or a NetworkStorageSession&.
>+        Further refactoring will be done in bug 193368 and bug 161106 to build on this and replace the global state with
>+        member variables of the correct containing objects.
>+
>+        * loader/cache/CachedResource.cpp:
>+        (WebCore::CachedResource::setResponse):
>+        (WebCore::CachedResource::varyHeaderValuesMatch):
>+        * platform/network/CacheValidation.cpp:
>+        (WebCore::cookieRequestHeaderFieldValue):
>+        (WebCore::headerValueForVary):
>+        (WebCore::collectVaryingRequestHeaders):
>+        (WebCore::verifyVaryingRequestHeaders):
>+        * platform/network/CacheValidation.h:
>+
> 2019-01-14  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         Remove redundant check for alignAttr and hiddenAttr in various isPresentationAttribute overrides
>Index: Source/WebCore/loader/cache/CachedResource.cpp
>===================================================================
>--- Source/WebCore/loader/cache/CachedResource.cpp	(revision 239959)
>+++ Source/WebCore/loader/cache/CachedResource.cpp	(working copy)
>@@ -476,7 +476,7 @@ void CachedResource::setResponse(const R
> {
>     ASSERT(m_response.type() == ResourceResponse::Type::Default);
>     m_response = response;
>-    m_varyingHeaderValues = collectVaryingRequestHeaders(m_resourceRequest, m_response, m_sessionID);
>+    m_varyingHeaderValues = collectVaryingRequestHeaders(*platformStrategies()->cookiesStrategy(), m_resourceRequest, m_response, m_sessionID);
> 
> #if ENABLE(SERVICE_WORKER)
>     if (m_response.source() == ResourceResponse::Source::ServiceWorker) {
>@@ -858,7 +858,7 @@ bool CachedResource::varyHeaderValuesMat
>     if (m_varyingHeaderValues.isEmpty())
>         return true;
> 
>-    return verifyVaryingRequestHeaders(m_varyingHeaderValues, request, m_sessionID);
>+    return verifyVaryingRequestHeaders(*platformStrategies()->cookiesStrategy(), m_varyingHeaderValues, request, m_sessionID);
> }
> 
> unsigned CachedResource::overheadSize() const
>Index: Source/WebCore/platform/network/CacheValidation.cpp
>===================================================================
>--- Source/WebCore/platform/network/CacheValidation.cpp	(revision 239959)
>+++ Source/WebCore/platform/network/CacheValidation.cpp	(working copy)
>@@ -326,27 +326,30 @@ CacheControlDirectives parseCacheControl
>     return result;
> }
> 
>-static String headerValueForVary(const ResourceRequest& request, const String& headerName, PAL::SessionID sessionID)
>+static String cookieRequestHeaderFieldValue(const NetworkStorageSession& session, const ResourceRequest& request)
>+{
>+    return session.cookieRequestHeaderFieldValue(request.firstPartyForCookies(), SameSiteInfo::create(request), request.url(), WTF::nullopt, WTF::nullopt, request.url().protocolIs("https") ? IncludeSecureCookies::Yes : IncludeSecureCookies::No).first;
>+}
>+
>+static String cookieRequestHeaderFieldValue(CookiesStrategy& cookiesStrategy, const PAL::SessionID& sessionID, const ResourceRequest& request)
>+{
>+    return cookiesStrategy.cookieRequestHeaderFieldValue(sessionID, request.firstPartyForCookies(), SameSiteInfo::create(request), request.url(), WTF::nullopt, WTF::nullopt, request.url().protocolIs("https") ? IncludeSecureCookies::Yes : IncludeSecureCookies::No).first;
>+}
>+
>+static String headerValueForVary(const ResourceRequest& request, const String& headerName, Function<String()>&& cookieRequestHeaderFieldValue)
> {
>     // Explicit handling for cookies is needed because they are added magically by the networking layer.
>     // FIXME: The value might have changed between making the request and retrieving the cookie here.
>     // We could fetch the cookie when making the request but that seems overkill as the case is very rare and it
>     // is a blocking operation. This should be sufficient to cover reasonable cases.
>-    if (headerName == httpHeaderNameString(HTTPHeaderName::Cookie)) {
>-        auto includeSecureCookies = request.url().protocolIs("https") ? IncludeSecureCookies::Yes : IncludeSecureCookies::No;
>-        auto* cookieStrategy = platformStrategies() ? platformStrategies()->cookiesStrategy() : nullptr;
>-        if (!cookieStrategy) {
>-            ASSERT(sessionID == PAL::SessionID::defaultSessionID());
>-            return NetworkStorageSession::defaultStorageSession().cookieRequestHeaderFieldValue(request.firstPartyForCookies(), SameSiteInfo::create(request), request.url(), WTF::nullopt, WTF::nullopt, includeSecureCookies).first;
>-        }
>-        return cookieStrategy->cookieRequestHeaderFieldValue(sessionID, request.firstPartyForCookies(), SameSiteInfo::create(request), request.url(), WTF::nullopt, WTF::nullopt, includeSecureCookies).first;
>-    }
>+    if (headerName == httpHeaderNameString(HTTPHeaderName::Cookie))
>+        return cookieRequestHeaderFieldValue();
>     return request.httpHeaderField(headerName);
> }
> 
>-Vector<std::pair<String, String>> collectVaryingRequestHeaders(const WebCore::ResourceRequest& request, const WebCore::ResourceResponse& response, PAL::SessionID sessionID)
>+static Vector<std::pair<String, String>> collectVaryingRequestHeaders(const ResourceResponse& response, Function<String(const String& headerName)>&& headerValueForVary)
> {
>-    String varyValue = response.httpHeaderField(WebCore::HTTPHeaderName::Vary);
>+    String varyValue = response.httpHeaderField(HTTPHeaderName::Vary);
>     if (varyValue.isEmpty())
>         return { };
>     Vector<String> varyingHeaderNames = varyValue.split(',');
>@@ -354,25 +357,60 @@ Vector<std::pair<String, String>> collec
>     varyingRequestHeaders.reserveCapacity(varyingHeaderNames.size());
>     for (auto& varyHeaderName : varyingHeaderNames) {
>         String headerName = varyHeaderName.stripWhiteSpace();
>-        String headerValue = headerValueForVary(request, headerName, sessionID);
>+        String headerValue = headerValueForVary(headerName);
>         varyingRequestHeaders.append(std::make_pair(headerName, headerValue));
>     }
>     return varyingRequestHeaders;
> }
> 
>-bool verifyVaryingRequestHeaders(const Vector<std::pair<String, String>>& varyingRequestHeaders, const WebCore::ResourceRequest& request, PAL::SessionID sessionID)
>+Vector<std::pair<String, String>> collectVaryingRequestHeaders(NetworkStorageSession& storageSession, const ResourceRequest& request, const ResourceResponse& response)
>+{
>+    return collectVaryingRequestHeaders(response, [&] (const String& headerName) {
>+        return headerValueForVary(request, headerName, [&] {
>+            return cookieRequestHeaderFieldValue(storageSession, request);
>+        });
>+    });
>+}
>+
>+Vector<std::pair<String, String>> collectVaryingRequestHeaders(CookiesStrategy& cookiesStrategy, const ResourceRequest& request, const ResourceResponse& response, const PAL::SessionID& sessionID)
>+{
>+    return collectVaryingRequestHeaders(response, [&] (const String& headerName) {
>+        return headerValueForVary(request, headerName, [&] {
>+            return cookieRequestHeaderFieldValue(cookiesStrategy, sessionID, request);
>+        });
>+    });
>+}
>+
>+static bool verifyVaryingRequestHeaders(const Vector<std::pair<String, String>>& varyingRequestHeaders, Function<String(const String&)>&& headerValueForVary)
> {
>     for (auto& varyingRequestHeader : varyingRequestHeaders) {
>         // FIXME: Vary: * in response would ideally trigger a cache delete instead of a store.
>         if (varyingRequestHeader.first == "*")
>             return false;
>-        String headerValue = headerValueForVary(request, varyingRequestHeader.first, sessionID);
>-        if (headerValue != varyingRequestHeader.second)
>+        if (headerValueForVary(varyingRequestHeader.first) != varyingRequestHeader.second)
>             return false;
>     }
>     return true;
> }
> 
>+bool verifyVaryingRequestHeaders(NetworkStorageSession& storageSession, const Vector<std::pair<String, String>>& varyingRequestHeaders, const ResourceRequest& request)
>+{
>+    return verifyVaryingRequestHeaders(varyingRequestHeaders, [&] (const String& headerName) {
>+        return headerValueForVary(request, headerName, [&] {
>+            return cookieRequestHeaderFieldValue(storageSession, request);
>+        });
>+    });
>+}
>+
>+bool verifyVaryingRequestHeaders(CookiesStrategy& cookiesStrategy, const Vector<std::pair<String, String>>& varyingRequestHeaders, const ResourceRequest& request, const PAL::SessionID& sessionID)
>+{
>+    return verifyVaryingRequestHeaders(varyingRequestHeaders, [&] (const String& headerName) {
>+        return headerValueForVary(request, headerName, [&] {
>+            return cookieRequestHeaderFieldValue(cookiesStrategy, sessionID, request);
>+        });
>+    });
>+}
>+
> // http://tools.ietf.org/html/rfc7231#page-48
> bool isStatusCodeCacheableByDefault(int statusCode)
> {
>Index: Source/WebCore/platform/network/CacheValidation.h
>===================================================================
>--- Source/WebCore/platform/network/CacheValidation.h	(revision 239959)
>+++ Source/WebCore/platform/network/CacheValidation.h	(working copy)
>@@ -34,7 +34,9 @@
> 
> namespace WebCore {
> 
>+class CookiesStrategy;
> class HTTPHeaderMap;
>+class NetworkStorageSession;
> class ResourceRequest;
> class ResourceResponse;
> 
>@@ -77,8 +79,10 @@ struct CacheControlDirectives {
> };
> WEBCORE_EXPORT CacheControlDirectives parseCacheControlDirectives(const HTTPHeaderMap&);
> 
>-WEBCORE_EXPORT Vector<std::pair<String, String>> collectVaryingRequestHeaders(const ResourceRequest&, const ResourceResponse&, PAL::SessionID = PAL::SessionID::defaultSessionID());
>-WEBCORE_EXPORT bool verifyVaryingRequestHeaders(const Vector<std::pair<String, String>>& varyingRequestHeaders, const ResourceRequest&, PAL::SessionID = PAL::SessionID::defaultSessionID());
>+WEBCORE_EXPORT Vector<std::pair<String, String>> collectVaryingRequestHeaders(NetworkStorageSession&, const ResourceRequest&, const ResourceResponse&);
>+WEBCORE_EXPORT Vector<std::pair<String, String>> collectVaryingRequestHeaders(CookiesStrategy&, const ResourceRequest&, const ResourceResponse&, const PAL::SessionID&);
>+WEBCORE_EXPORT bool verifyVaryingRequestHeaders(NetworkStorageSession&, const Vector<std::pair<String, String>>& varyingRequestHeaders, const ResourceRequest&);
>+WEBCORE_EXPORT bool verifyVaryingRequestHeaders(CookiesStrategy&, const Vector<std::pair<String, String>>& varyingRequestHeaders, const ResourceRequest&, const PAL::SessionID&);
> 
> WEBCORE_EXPORT bool isStatusCodeCacheableByDefault(int statusCode);
> WEBCORE_EXPORT bool isStatusCodePotentiallyCacheable(int statusCode);
>Index: Source/WebKit/ChangeLog
>===================================================================
>--- Source/WebKit/ChangeLog	(revision 239973)
>+++ Source/WebKit/ChangeLog	(working copy)
>@@ -1,3 +1,17 @@
>+2019-01-14  Alex Christensen  <achristensen@webkit.org>
>+
>+        Split headerValueForVary into specialized functions for NetworkProcess and WebProcess/WebKitLegacy
>+        https://bugs.webkit.org/show_bug.cgi?id=193429
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * NetworkProcess/cache/NetworkCache.cpp:
>+        (WebKit::NetworkCache::makeUseDecision):
>+        (WebKit::NetworkCache::Cache::retrieve):
>+        (WebKit::NetworkCache::Cache::makeEntry):
>+        (WebKit::NetworkCache::Cache::makeRedirectEntry):
>+        (WebKit::NetworkCache::Cache::update):
>+
> 2019-01-14  Tim Horton  <timothy_horton@apple.com>
> 
>         Fix a style mistake in PageClientImplMac
>Index: Source/WebKit/NetworkProcess/cache/NetworkCache.cpp
>===================================================================
>--- Source/WebKit/NetworkProcess/cache/NetworkCache.cpp	(revision 239959)
>+++ Source/WebKit/NetworkProcess/cache/NetworkCache.cpp	(working copy)
>@@ -191,7 +191,7 @@ static UseDecision makeUseDecision(const
>     if (request.isConditional() && !entry.redirectRequest())
>         return UseDecision::Validate;
> 
>-    if (!WebCore::verifyVaryingRequestHeaders(entry.varyingRequestHeaders(), request))
>+    if (!WebCore::verifyVaryingRequestHeaders(WebCore::NetworkStorageSession::defaultStorageSession(), entry.varyingRequestHeaders(), request))
>         return UseDecision::NoDueToVaryingHeaderMismatch;
> 
>     // We never revalidate in the case of a history navigation.
>@@ -307,7 +307,7 @@ void Cache::retrieve(const WebCore::Reso
>     if (canUseSpeculativeRevalidation && m_speculativeLoadManager->canRetrieve(storageKey, request, frameID)) {
>         m_speculativeLoadManager->retrieve(storageKey, [request, completionHandler = WTFMove(completionHandler), info = WTFMove(info)](std::unique_ptr<Entry> entry) mutable {
>             info.wasSpeculativeLoad = true;
>-            if (entry && WebCore::verifyVaryingRequestHeaders(entry->varyingRequestHeaders(), request))
>+            if (entry && WebCore::verifyVaryingRequestHeaders(WebCore::NetworkStorageSession::defaultStorageSession(), entry->varyingRequestHeaders(), request))
>                 completeRetrieve(WTFMove(completionHandler), WTFMove(entry), info);
>             else
>                 completeRetrieve(WTFMove(completionHandler), nullptr, info);
>@@ -364,12 +364,12 @@ void Cache::completeRetrieve(RetrieveCom
>     
> std::unique_ptr<Entry> Cache::makeEntry(const WebCore::ResourceRequest& request, const WebCore::ResourceResponse& response, RefPtr<WebCore::SharedBuffer>&& responseData)
> {
>-    return std::make_unique<Entry>(makeCacheKey(request), response, WTFMove(responseData), WebCore::collectVaryingRequestHeaders(request, response));
>+    return std::make_unique<Entry>(makeCacheKey(request), response, WTFMove(responseData), WebCore::collectVaryingRequestHeaders(WebCore::NetworkStorageSession::defaultStorageSession(), request, response));
> }
> 
> std::unique_ptr<Entry> Cache::makeRedirectEntry(const WebCore::ResourceRequest& request, const WebCore::ResourceResponse& response, const WebCore::ResourceRequest& redirectRequest)
> {
>-    return std::make_unique<Entry>(makeCacheKey(request), response, redirectRequest, WebCore::collectVaryingRequestHeaders(request, response));
>+    return std::make_unique<Entry>(makeCacheKey(request), response, redirectRequest, WebCore::collectVaryingRequestHeaders(WebCore::NetworkStorageSession::defaultStorageSession(), request, response));
> }
> 
> std::unique_ptr<Entry> Cache::store(const WebCore::ResourceRequest& request, const WebCore::ResourceResponse& response, RefPtr<WebCore::SharedBuffer>&& responseData, Function<void(MappedBody&)>&& completionHandler)
>@@ -453,7 +453,7 @@ std::unique_ptr<Entry> Cache::update(con
>     WebCore::ResourceResponse response = existingEntry.response();
>     WebCore::updateResponseHeadersAfterRevalidation(response, validatingResponse);
> 
>-    auto updateEntry = std::make_unique<Entry>(existingEntry.key(), response, existingEntry.buffer(), WebCore::collectVaryingRequestHeaders(originalRequest, response));
>+    auto updateEntry = std::make_unique<Entry>(existingEntry.key(), response, existingEntry.buffer(), WebCore::collectVaryingRequestHeaders(WebCore::NetworkStorageSession::defaultStorageSession(), originalRequest, response));
>     auto updateRecord = updateEntry->encodeAsStorageRecord();
> 
>     m_storage->store(updateRecord, { });

Flags:

joepeck: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193429: 359127