Separate preload matching from MemoryCache

third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp

Index: third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
diff --git a/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp b/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
index a638d74e061bf8ef0505ffac18665d74dce644d9..35b0fb0047da4bbf59941a93b7127fe46ef16e85 100644
--- a/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
+++ b/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
@@ -493,11 +493,26 @@ void ResourceFetcher::UpdateMemoryCacheStats(Resource* resource,
   // dead if MemoryCache holds weak references to Resource). Currently we check
   // references to Resource from ResourceClient and |m_preloads| only, because
   // they are major sources of references.
-  if (resource && !resource->IsAlive() && !preloads_.Contains(resource)) {
+  if (resource && !resource->IsAlive() && !ContainsAsPreload(resource)) {
     DEFINE_RESOURCE_HISTOGRAM("Dead.");
   }
 }
 
+bool ResourceFetcher::ContainsAsPreload(Resource* resource) const {
+  auto it = preloads_.find(PreloadKey(resource->Url(), resource->GetType()));
+  return it != preloads_.end() && it->value == resource;
+}
+
+void ResourceFetcher::RemovePreload(Resource* resource) {
+  auto it = preloads_.find(PreloadKey(resource->Url(), resource->GetType()));
+  if (it == preloads_.end())
+    return;
+  if (it->value == resource) {
+    resource->DecreasePreloadCount();
+    preloads_.erase(it);
+  }
+}
+
 ResourceFetcher::PrepareRequestResult ResourceFetcher::PrepareRequest(
     FetchParameters& params,
     const ResourceFactory& factory,
@@ -604,19 +619,30 @@ Resource* ResourceFetcher::RequestResource(
     if (!resource && !is_data_url && archive_)
       return nullptr;
   }
+  RevalidationPolicy policy = kLoad;
+  bool preload_found = false;
   if (!resource) {
-    resource =
-        GetMemoryCache()->ResourceForURL(params.Url(), GetCacheIdentifier());
+    resource = MatchPreload(params, factory.GetType());
+    if (resource) {
+      preload_found = true;
+      policy = kUse;
+      // If |param| is for a blocking resource and a preloaded resource is
+      // found, we may need to make it block the onload event.
+      MakePreloadedResourceBlockOnloadIfNeeded(resource, params);
+    }
   }
+  if (!preload_found) {
+    if (!resource) {
+      resource =
+          GetMemoryCache()->ResourceForURL(params.Url(), GetCacheIdentifier());
+    }
 
-  // If we got a preloaded resource from the cache for a non-preload request,
-  // we may need to make it block the onload event.
-  MakePreloadedResourceBlockOnloadIfNeeded(resource, params);
-
-  const RevalidationPolicy policy = DetermineRevalidationPolicy(
-      factory.GetType(), params, resource, is_static_data);
-  TRACE_EVENT_INSTANT1("blink", "ResourceFetcher::determineRevalidationPolicy",
-                       TRACE_EVENT_SCOPE_THREAD, "revalidationPolicy", policy);
+    policy = DetermineRevalidationPolicy(factory.GetType(), params, resource,
+                                         is_static_data);
+    TRACE_EVENT_INSTANT1(
+        "blink", "ResourceFetcher::determineRevalidationPolicy",
+        TRACE_EVENT_SCOPE_THREAD, "revalidationPolicy", policy);
+  }
 
   UpdateMemoryCacheStats(resource, policy, params, factory, is_static_data);
 
@@ -669,11 +695,17 @@ Resource* ResourceFetcher::RequestResource(
   // loading immediately. If revalidation policy was determined as |Revalidate|,
   // the resource was already initialized for the revalidation here, but won't
   // start loading.
-  if (!ResourceNeedsLoad(resource, params, policy))
+  if (!ResourceNeedsLoad(resource, params, policy)) {
+    if (policy != kUse)
+      InsertAsPreloadIfNecessary(resource, params, factory.GetType());
     return resource;
+  }
 
   if (!StartLoad(resource))
     return nullptr;
+
+  if (policy != kUse)
+    InsertAsPreloadIfNecessary(resource, params, factory.GetType());
   scoped_resource_load_tracker.ResourceLoadContinuesBeyondScope();
 
   DCHECK(!resource->ErrorOccurred() ||
@@ -827,61 +859,57 @@ void ResourceFetcher::RecordResourceTimingOnRedirect(
   }
 }
 
-ResourceFetcher::RevalidationPolicy
-ResourceFetcher::DetermineRevalidationPolicy(
-    Resource::Type type,
-    const FetchParameters& fetch_params,
-    Resource* existing_resource,
-    bool is_static_data) const {
-  const ResourceRequest& request = fetch_params.GetResourceRequest();
-
-  if (!existing_resource)
-    return kLoad;
+Resource* ResourceFetcher::MatchPreload(const FetchParameters& params,
+                                        Resource::Type type) {
+  auto it = preloads_.find(PreloadKey(params.Url(), type));
+  if (it == preloads_.end())
+    return nullptr;
 
-  // If the existing resource is loading and the associated fetcher is not equal
-  // to |this|, we must not use the resource. Otherwise, CSP violation may
-  // happen in redirect handling.
-  if (existing_resource->Loader() &&
-      existing_resource->Loader()->Fetcher() != this) {
-    return kReload;
-  }
+  Resource* resource = it->value;
 
-  // Checks if the resource has an explicit policy about integrity metadata.
-  //
-  // This is necessary because ScriptResource and CSSStyleSheetResource objects
-  // do not keep the raw data around after the source is accessed once, so if
-  // the resource is accessed from the MemoryCache for a second time, there is
-  // no way to redo an integrity check.
-  //
-  // Thus, Blink implements a scheme where it caches the integrity information
-  // for those resources after the first time it is checked, and if there is
-  // another request for that resource, with the same integrity metadata, Blink
-  // skips the integrity calculation. However, if the integrity metadata is a
-  // mismatch, the MemoryCache must be skipped here, and a new request for the
-  // resource must be made to get the raw data. This is expected to be an
-  // uncommon case, however, as it implies two same-origin requests to the same
-  // resource, but with different integrity metadata.
   RecordSriResourceIntegrityMismatchEvent(kCheckingForIntegrityMismatch);
-  if (existing_resource->MustRefetchDueToIntegrityMetadata(fetch_params)) {
-    RecordSriResourceIntegrityMismatchEvent(kRefetchDueToIntegrityMismatch);
-    return kReload;
-  }
+  if (resource->MustRefetchDueToIntegrityMetadata(params))
+    return nullptr;
 
-  // If the same URL has been loaded as a different type, we need to reload.
-  if (existing_resource->GetType() != type) {
-    // FIXME: If existingResource is a Preload and the new type is LinkPrefetch
-    // We really should discard the new prefetch since the preload has more
-    // specific type information! crbug.com/379893
-    // fast/dom/HTMLLinkElement/link-and-subresource-test hits this case.
-    RESOURCE_LOADING_DVLOG(1) << "ResourceFetcher::determineRevalidationPolicy "
-                                 "reloading due to type mismatch.";
-    return kReload;
+  if (params.IsSpeculativePreload())
+    return resource;
+  if (params.IsLinkPreload()) {
+    resource->SetLinkPreload(true);
+    return resource;
   }
 
-  // We already have a preload going for this URL.
-  if (fetch_params.IsSpeculativePreload() && existing_resource->IsPreloaded())
-    return kUse;
+  if (!IsReusableForPreloading(params, resource, false))
+    return nullptr;
+
+  resource->DecreasePreloadCount();
+  preloads_.erase(it);
+  matched_preloads_.push_back(resource);
+  return resource;
+}
 
+void ResourceFetcher::InsertAsPreloadIfNecessary(Resource* resource,
+                                                 const FetchParameters& params,
+                                                 Resource::Type type) {
+  if (!params.IsSpeculativePreload() && !params.IsLinkPreload())
+    return;
+  // CSP layout tests verify that preloads are subject to access checks by
+  // seeing if they are in the `preload started` list. Therefore do not add
+  // them to the list if the load is immediately denied.
+  if (resource->GetResourceError().IsAccessCheck())
+    return;
+  PreloadKey key(params.Url(), type);
+  if (preloads_.find(key) == preloads_.end()) {
+    preloads_.insert(key, resource);
+    resource->IncreasePreloadCount();
+    if (preloaded_urls_for_test_)
+      preloaded_urls_for_test_->insert(resource->Url().GetString());
+  }
+}
+
+bool ResourceFetcher::IsReusableForPreloading(const FetchParameters& params,
+                                              Resource* existing_resource,
+                                              bool is_static_data) const {
+  const ResourceRequest& request = params.GetResourceRequest();
   // Do not load from cache if images are not enabled. There are two general
   // cases:
   //
@@ -898,13 +926,13 @@ ResourceFetcher::DetermineRevalidationPolicy(
   // TODO(japhet): Can we get rid of one of these settings?
   if (existing_resource->IsImage() &&
       !Context().AllowImage(images_enabled_, existing_resource->Url())) {
-    return kReload;
+    return false;
   }
 
   // Never use cache entries for downloadToFile / useStreamOnResponse requests.
   // The data will be delivered through other paths.
   if (request.DownloadToFile() || request.UseStreamOnResponse())
-    return kReload;
+    return false;
 
   // Never reuse opaque responses from a service worker for requests that are
   // not no-cors. https://crbug.com/625575
@@ -912,11 +940,11 @@ ResourceFetcher::DetermineRevalidationPolicy(
       existing_resource->GetResponse().ServiceWorkerResponseType() ==
           kWebServiceWorkerResponseTypeOpaque &&
       request.GetFetchRequestMode() != WebURLRequest::kFetchRequestModeNoCORS) {
-    return kReload;
+    return false;
   }
 
-  if (!is_static_data && !existing_resource->CanReuse(fetch_params))
-    return kReload;
+  if (!is_static_data && !existing_resource->CanReuse(params))
+    return false;
 
   // Certain requests (e.g., XHRs) might have manually set headers that require
   // revalidation. In theory, this should be a Revalidate case. In practice, the
@@ -932,17 +960,72 @@ ResourceFetcher::DetermineRevalidationPolicy(
   if (!is_static_data &&
       (request.IsConditional() ||
        existing_resource->GetResponse().HttpStatusCode() == 304)) {
-    return kReload;
+    return false;
   }
 
   if (!is_static_data &&
-      !fetch_params.Options().CanReuseRequest(existing_resource->Options())) {
+      !params.Options().CanReuseRequest(existing_resource->Options())) {
+    return false;
+  }
+
+  return true;
+}
+
+ResourceFetcher::RevalidationPolicy
+ResourceFetcher::DetermineRevalidationPolicy(
+    Resource::Type type,
+    const FetchParameters& fetch_params,
+    Resource* existing_resource,
+    bool is_static_data) const {
+  const ResourceRequest& request = fetch_params.GetResourceRequest();
+
+  if (!existing_resource)
+    return kLoad;
+
+  // If the existing resource is loading and the associated fetcher is not equal
+  // to |this|, we must not use the resource. Otherwise, CSP violation may
+  // happen in redirect handling.
+  if (existing_resource->Loader() &&
+      existing_resource->Loader()->Fetcher() != this) {
     return kReload;
   }
 
-  // Always use preloads.
-  if (existing_resource->IsPreloaded())
-    return kUse;
+  // Checks if the resource has an explicit policy about integrity metadata.
+  //
+  // This is necessary because ScriptResource and CSSStyleSheetResource objects
+  // do not keep the raw data around after the source is accessed once, so if
+  // the resource is accessed from the MemoryCache for a second time, there is
+  // no way to redo an integrity check.
+  //
+  // Thus, Blink implements a scheme where it caches the integrity information
+  // for those resources after the first time it is checked, and if there is
+  // another request for that resource, with the same integrity metadata, Blink
+  // skips the integrity calculation. However, if the integrity metadata is a
+  // mismatch, the MemoryCache must be skipped here, and a new request for the
+  // resource must be made to get the raw data. This is expected to be an
+  // uncommon case, however, as it implies two same-origin requests to the same
+  // resource, but with different integrity metadata.
+  RecordSriResourceIntegrityMismatchEvent(kCheckingForIntegrityMismatch);
+  if (existing_resource->MustRefetchDueToIntegrityMetadata(fetch_params)) {
+    RecordSriResourceIntegrityMismatchEvent(kRefetchDueToIntegrityMismatch);
+    return kReload;
+  }
+
+  // If the same URL has been loaded as a different type, we need to reload.
+  if (existing_resource->GetType() != type) {
+    // FIXME: If existingResource is a Preload and the new type is LinkPrefetch
+    // We really should discard the new prefetch since the preload has more
+    // specific type information! crbug.com/379893
+    // fast/dom/HTMLLinkElement/link-and-subresource-test hits this case.
+    RESOURCE_LOADING_DVLOG(1) << "ResourceFetcher::determineRevalidationPolicy "
+                                 "reloading due to type mismatch.";
+    return kReload;
+  }
+
+  // If |existing_resource| is not reusable as a preloaded resource, it should
+  // not be reusable as a normal resource as well.
+  if (!IsReusableForPreloading(fetch_params, existing_resource, is_static_data))

[Yoav Weiss, 2017/05/12 16:33:57]

Can we maybe change the name of `isReusableForPreloading`, as it's applied to to
all resources?

[kinuko, 2017/05/15 02:09:04]

+1, felt slightly confusing.

[yhirano, 2017/05/16 11:19:24]

I couldn't come up with a better name. Do you have any recommendation?

[kinuko, 2017/05/16 22:58:53]

IsReusableAlsoForPreloading ...?  (I'm not good at naming either)

[yhirano, 2017/05/17 04:24:44]

Done.

+    return kReload;
 
   // If resource was populated from a SubstituteData load or data: url, use it.
   if (is_static_data)
@@ -1103,24 +1186,15 @@ int ResourceFetcher::NonblockingRequestCount() const {
   return non_blocking_loaders_.size();
 }
 
-void ResourceFetcher::PreloadStarted(Resource* resource) {
-  if (preloads_.Contains(resource))
-    return;
-  resource->IncreasePreloadCount();
-
-  preloads_.insert(resource);
-
-  if (preloaded_urls_for_test_)
-    preloaded_urls_for_test_->insert(resource->Url().GetString());
-}
-
 void ResourceFetcher::EnableIsPreloadedForTest() {
   if (preloaded_urls_for_test_)
     return;
   preloaded_urls_for_test_ = WTF::WrapUnique(new HashSet<String>);
 
-  for (const auto& resource : preloads_)
+  for (const auto& pair : preloads_) {
+    Resource* resource = pair.value;
     preloaded_urls_for_test_->insert(resource->Url().GetString());
+  }
 }
 
 bool ResourceFetcher::IsPreloadedForTest(const KURL& url) const {
@@ -1131,18 +1205,24 @@ bool ResourceFetcher::IsPreloadedForTest(const KURL& url) const {
 void ResourceFetcher::ClearPreloads(ClearPreloadsPolicy policy) {
   LogPreloadStats(policy);
 
-  for (const auto& resource : preloads_) {
+  Vector<PreloadKey> keys_to_be_removed;
+  for (const auto& pair : preloads_) {
+    Resource* resource = pair.value;
     if (policy == kClearAllPreloads || !resource->IsLinkPreload()) {
       resource->DecreasePreloadCount();
       if (resource->GetPreloadResult() == Resource::kPreloadNotReferenced)
-        GetMemoryCache()->Remove(resource.Get());
-      preloads_.erase(resource);
+        GetMemoryCache()->Remove(resource);
+      keys_to_be_removed.push_back(pair.key);
     }
   }
+  preloads_.RemoveAll(keys_to_be_removed);
+
+  matched_preloads_.clear();
 }
 
 void ResourceFetcher::WarnUnusedPreloads() {
-  for (const auto& resource : preloads_) {
+  for (const auto& pair : preloads_) {
+    Resource* resource = pair.value;
     if (resource && resource->IsLinkPreload() &&
         resource->GetPreloadResult() == Resource::kPreloadNotReferenced) {
       Context().AddConsoleMessage(
@@ -1249,6 +1329,8 @@ void ResourceFetcher::HandleLoaderError(Resource* resource,
                             resource->GetResponse().EncodedDataLength(),
                             is_internal_request);
 
+  if (error.IsCancellation())
+    RemovePreload(resource);
   resource->FinishAsError(error);
 
   HandleLoadCompletion(resource);
@@ -1407,7 +1489,8 @@ void ResourceFetcher::LogPreloadStats(ClearPreloadsPolicy policy) {
   unsigned import_misses = 0;
   unsigned raws = 0;
   unsigned raw_misses = 0;
-  for (const auto& resource : preloads_) {
+  for (const auto& pair : preloads_) {
+    Resource* resource = pair.value;
     // Do not double count link rel preloads. These do not get cleared if the
     // ClearPreloadsPolicy is only clearing speculative markup preloads.
     if (resource->IsLinkPreload() &&
@@ -1561,6 +1644,7 @@ DEFINE_TRACE(ResourceFetcher) {
   visitor->Trace(non_blocking_loaders_);
   visitor->Trace(document_resources_);
   visitor->Trace(preloads_);
+  visitor->Trace(matched_preloads_);
   visitor->Trace(resource_timing_info_map_);
 }