NS memory cache: Record blocked by request header reasons

Also move the request header checks after cache entry existence
check so that we prioritize not-in-the cache cases.

Bug: 1339708
Change-Id: I1d8c9697431a2d685f027e5d9dfbe8b4de86cea8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3781946
Auto-Submit: Kenichi Ishibashi <bashi@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Commit-Queue: Kenichi Ishibashi <bashi@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1027640}

services/network/network_service_memory_cache.cc

@@ -47,26 +47,46 @@ namespace network {
 
 namespace {
 
+// These values are persisted to logs. Entries should not be renumbered and
+// numeric values should never be reused.
+enum class BlockedByRequestHeaderReason {
+  kIfUnmodifiedSince = 0,
+  kIfMatch = 1,
+  kIfRange = 2,
+  kIfModifiedSince = 3,
+  kIfNoneMatch = 4,
+  kCacheControlNoCache = 5,
+  kPragmaNoCache = 6,
+  kCacheControlMaxAgeZero = 7,
+  kRange = 8,
+  kMaxValue = kRange,
+};
+
 struct HeaderNameAndValue {
   const char* name;
   const char* value;
+  BlockedByRequestHeaderReason reason;
 };
 
 // Collected from kPassThroughHeaders, kValidationHeaders, kForceFetchHeaders,
 // kForceValidateHeaders, in //net/http/http_cache_transaction.cc.
 // TODO(https://crbug.com/1339708): It'd be worthwhile to remove the
 // duplication.
 constexpr HeaderNameAndValue kSpecialHeaders[] = {
-    {"if-unmodified-since", nullptr},
-    {"if-match", nullptr},
-    {"if-range", nullptr},
-    {"if-modified-since", nullptr},
-    {"if-none-match", nullptr},
-    {"cache-control", "no-cache"},
-    {"pragma", "no-cache"},
-    {"cache-control", "max-age=0"},
+    {"if-unmodified-since", nullptr,
+     BlockedByRequestHeaderReason::kIfUnmodifiedSince},
+    {"if-match", nullptr, BlockedByRequestHeaderReason::kIfMatch},
+    {"if-range", nullptr, BlockedByRequestHeaderReason::kIfRange},
+    {"if-modified-since", nullptr,
+     BlockedByRequestHeaderReason::kIfModifiedSince},
+    {"if-none-match", nullptr, BlockedByRequestHeaderReason::kIfNoneMatch},
+    {"cache-control", "no-cache",
+     BlockedByRequestHeaderReason::kCacheControlNoCache},
+    {"pragma", "no-cache", BlockedByRequestHeaderReason::kPragmaNoCache},
+    {"cache-control", "max-age=0",
+     BlockedByRequestHeaderReason::kCacheControlMaxAgeZero},
     // The in-memory cache doesn't support range requests.
-    {"range", nullptr},
+    {"range", nullptr, BlockedByRequestHeaderReason::kRange},
 };
 
 // TODO(https://crbug.com/1339708): Adjust these parameters based on stats.
@@ -187,22 +207,23 @@ bool VaryHasSupportedHeadersOnly(
   return true;
 }
 
-bool CheckSpecialRequestHeaders(const net::HttpRequestHeaders& headers) {
-  for (const auto& [name, value] : kSpecialHeaders) {
+absl::optional<BlockedByRequestHeaderReason> CheckSpecialRequestHeaders(
+    const net::HttpRequestHeaders& headers) {
+  for (const auto& [name, value, reason] : kSpecialHeaders) {
     std::string header_value;
     if (!headers.GetHeader(name, &header_value))
       continue;
     // `nullptr` means `header_value` doesn't matter.
     if (value == nullptr)
-      return false;
+      return reason;
     net::HttpUtil::ValuesIterator v(header_value.begin(), header_value.end(),
                                     ',');
     while (v.GetNext()) {
       if (base::EqualsCaseInsensitiveASCII(v.value_piece(), value))
-        return false;
+        return reason;
     }
   }
-  return true;
+  return absl::nullopt;
 }
 
 bool MatchVaryHeader(const ResourceRequest& resource_request,
@@ -337,8 +358,10 @@ NetworkServiceMemoryCache::MaybeCreateWriter(
   if (net::IsCertStatusError(url_request->ssl_info().cert_status))
     return nullptr;
 
-  if (!CheckSpecialRequestHeaders(url_request->extra_request_headers()))
+  if (CheckSpecialRequestHeaders(url_request->extra_request_headers())
+          .has_value()) {
     return nullptr;
+  }
 
   if (response->content_length > static_cast<int>(max_per_entry_bytes_))
     return nullptr;
@@ -438,11 +461,6 @@ absl::optional<std::string> NetworkServiceMemoryCache::CanServe(
     return absl::nullopt;
   }
 
-  if (!CheckSpecialRequestHeaders(resource_request.headers)) {
-    RecordEntryStatus(EntryStatus::kBlockedByRequestHeaders);
-    return absl::nullopt;
-  }
-
   absl::optional<std::string> cache_key = GenerateCacheKeyForResourceRequest(
       resource_request, network_isolation_key);
   if (!cache_key.has_value())
@@ -454,6 +472,16 @@ absl::optional<std::string> NetworkServiceMemoryCache::CanServe(
     return absl::nullopt;
   }
 
+  absl::optional<BlockedByRequestHeaderReason> blocked_by_headers =
+      CheckSpecialRequestHeaders(resource_request.headers);
+  if (blocked_by_headers.has_value()) {
+    RecordEntryStatus(EntryStatus::kBlockedByRequestHeaders);
+    base::UmaHistogramEnumeration(
+        "NetworkService.MemoryCache.BlockedByRequestHeaderReason",
+        *blocked_by_headers);
+    return absl::nullopt;
+  }
+
   if (!CheckPrivateNetworkAccess(load_options, resource_request,
                                  client_security_state,
                                  it->second->transport_info)) {

tools/metrics/histograms/enums.xml

@@ -69744,6 +69744,18 @@ Called by update_net_trust_anchors.py.-->
   <int value="26" label="SIM_LOCKED"/>
 </enum>
 
+<enum name="NetworkServiceMemoryCacheBlockedByRequestHeaderReason">
+  <int value="0" label="IfUnmodifiedSince"/>
+  <int value="1" label="IfMatch"/>
+  <int value="2" label="IfRange"/>
+  <int value="3" label="IfModifiedSince"/>
+  <int value="4" label="IfNoneMatch"/>
+  <int value="5" label="CacheControlNoCache"/>
+  <int value="6" label="PragmaNoCache"/>
+  <int value="7" label="CacheControlMaxAgeZero"/>
+  <int value="8" label="Range"/>
+</enum>
+
 <enum name="NetworkServiceMemoryCacheEntryStatus">
   <int value="0" label="NotInCache"/>
   <int value="1" label="Stale"/>

tools/metrics/histograms/metadata/network/histograms.xml

@@ -3000,6 +3000,18 @@ chromium-metrics-reviews@google.com.
   </summary>
 </histogram>
 
+<histogram name="NetworkService.MemoryCache.BlockedByRequestHeaderReason"
+    enum="NetworkServiceMemoryCacheBlockedByRequestHeaderReason"
+    expires_after="2022-10-05">
+  <owner>bashi@chromium.org</owner>
+  <owner>blink-network-stack@google.com</owner>
+  <summary>
+    Records a reason why a request cannot be served from the in-memory cache due
+    to request header checks. Recorded when the in-memory cache tries to serve a
+    stored response.
+  </summary>
+</histogram>
+
 <histogram name="NetworkService.MemoryCache.ContentLength.{RequestDestination}"
     units="bytes" expires_after="2022-10-05">
   <owner>bashi@chromium.org</owner>