Refactor the forPreload flag to mean speculative preload.

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

 /*
     Copyright (C) 1998 Lars Knoll (knoll@mpi-hd.mpg.de)
     Copyright (C) 2001 Dirk Mueller (mueller@kde.org)
     Copyright (C) 2002 Waldo Bastian (bastian@kde.org)
     Copyright (C) 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011 Apple Inc. All
     rights reserved.
     Copyright (C) 2009 Torch Mobile Inc. http://www.torchmobile.com/
 
     This library is free software; you can redistribute it and/or
     modify it under the terms of the GNU Library General Public
@@ skipping 139 matching lines @@
   return ResourceLoadPriorityUnresolved;
 }
 
 }  // namespace
 
 ResourceLoadPriority ResourceFetcher::computeLoadPriority(
     Resource::Type type,
     const ResourceRequest& resourceRequest,
     ResourcePriority::VisibilityStatus visibility,
     FetchRequest::DeferOption deferOption,
-    bool forPreload) {
+    bool speculativePreload) {
   ResourceLoadPriority priority = typeToPriority(type);
 
   // Visible resources (images in practice) get a boost to High priority.
   if (visibility == ResourcePriority::Visible)
     priority = ResourceLoadPriorityHigh;
 
   // Resources before the first image are considered "early" in the document and
   // resources after the first image are "late" in the document.  Important to
   // note that this is based on when the preload scanner discovers a resource
   // for the most part so the main parser may not have reached the image element
   // yet.
   if (type == Resource::Image)
     m_imageFetched = true;
 
   if (FetchRequest::IdleLoad == deferOption) {
     priority = ResourceLoadPriorityVeryLow;
   } else if (type == Resource::Script) {
     // Special handling for scripts.
     // Default/Parser-Blocking/Preload early in document: High (set in
     // typeToPriority)
     // Async/Defer: Low Priority (applies to both preload and parser-inserted)
     // Preload late in document: Medium
-    if (FetchRequest::LazyLoad == deferOption)
+    if (FetchRequest::LazyLoad == deferOption) {
       priority = ResourceLoadPriorityLow;
-    else if (forPreload && m_imageFetched)
+    } else if (speculativePreload && m_imageFetched) {
+      // TODO(yoav): It is not clear why this applies only to preloaded scripts.

[Charlie Harrison, 2017/02/06 19:54:16]

I did some blaming to trace this logic. It stems from pmeenans priority work. I
think the problem was that the preload scanner would scan too far ahead in some
cases, treating script way ahead in the document as more important than it
should be. Arguably we could apply that logic to *all* scripts late in the
document, but the preload bit is an OK signal that we can probably lower
priority.

[Yoav Weiss, 2017/02/06 23:15:49]

Changed to a comment stating the above

       priority = ResourceLoadPriorityMedium;
+    }
   } else if (FetchRequest::LazyLoad == deferOption) {
     priority = ResourceLoadPriorityVeryLow;
   }
 
   // A manually set priority acts as a floor. This is used to ensure that
   // synchronous requests are always given the highest possible priority, as
   // well as to ensure that there isn't priority churn if images move in and out
   // of the viewport, or is displayed more than once, both in and out of the
   // viewport.
   return std::max(context().modifyPriorityForExperiments(priority),
@@ skipping 72 matching lines @@
   return resource.get();
 }
 
 bool ResourceFetcher::isControlledByServiceWorker() const {
   return context().isControlledByServiceWorker();
 }
 
 bool ResourceFetcher::resourceNeedsLoad(Resource* resource,
                                         const FetchRequest& request,
                                         RevalidationPolicy policy) {
-  // Defer a font load until it is actually needed unless this is a preload.
-  if (resource->getType() == Resource::Font && !request.forPreload())
+  // Defer a font load until it is actually needed unless this is a link
+  // preload.
+  if (resource->getType() == Resource::Font && !request.isLinkPreload())
     return false;
   if (resource->isImage() && shouldDeferImageLoad(resource->url()))
     return false;
   return policy != Use || resource->stillNeedsLoad();
 }
 
 // Limit the number of URLs in m_validatedURLs to avoid memory bloat.
 // http://crbug.com/52411
 static const int kMaxValidatedURLsSize = 10000;
 
@@ skipping 117 matching lines @@
     const FetchRequest& request,
     const ResourceFactory& factory,
     ResourceRequestBlockedReason blockedReason) {
   Resource* resource = factory.create(request.resourceRequest(),
                                       request.options(), request.charset());
   resource->error(ResourceError::cancelledDueToAccessCheckError(request.url(),
                                                                 blockedReason));
   return resource;
 }
 
-void ResourceFetcher::moveCachedNonBlockingResourceToBlocking(
+void ResourceFetcher::makePreloadedResourceBlockOnloadIfNeeded(
     Resource* resource,
     const FetchRequest& request) {
   // TODO(yoav): Test that non-blocking resources (video/audio/track) continue
   // to not-block even after being preloaded and discovered.
   if (resource && resource->loader() &&
       resource->isLoadEventBlockingResourceType() &&
-      m_nonBlockingLoaders.contains(resource->loader()) &&
-      resource->isLinkPreload() && !request.forPreload()) {
+      resource->isLinkPreload() && !request.isLinkPreload() &&
+      m_nonBlockingLoaders.contains(resource->loader())) {
     m_nonBlockingLoaders.remove(resource->loader());
     m_loaders.insert(resource->loader());
   }
 }
 
 void ResourceFetcher::updateMemoryCacheStats(Resource* resource,
                                              RevalidationPolicy policy,
                                              const FetchRequest& request,
                                              const ResourceFactory& factory,
                                              bool isStaticData) const {
   if (isStaticData)
     return;
 
-  if (request.forPreload()) {
+  if (request.speculativePreload() || request.isLinkPreload()) {

[Charlie Harrison, 2017/02/06 19:54:16]

nit: seeing these together brings out the inconsistency in naming. Can we rename
to isSpeculativePreload? That better adheres to blink style.

[Yoav Weiss, 2017/02/06 23:15:49]

changed

     DEFINE_RESOURCE_HISTOGRAM("Preload.");
   } else {
     DEFINE_RESOURCE_HISTOGRAM("");
   }
 
   // Aims to count Resource only referenced from MemoryCache (i.e. what would be
   // 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() &&
@@ skipping 16 matching lines @@
 
   context().populateResourceRequest(
       factory.type(), request.clientHintsPreferences(),
       request.getResourceWidth(), resourceRequest);
 
   if (!request.url().isValid())
     return Abort;
 
   resourceRequest.setPriority(computeLoadPriority(
       factory.type(), request.resourceRequest(), ResourcePriority::NotVisible,
-      request.defer(), request.forPreload()));
+      request.defer(), request.speculativePreload()));
   initializeResourceRequest(resourceRequest, factory.type(), request.defer());
   network_instrumentation::resourcePrioritySet(identifier,
                                                resourceRequest.priority());
 
   blockedReason = context().canRequest(
       factory.type(), resourceRequest,
       MemoryCache::removeFragmentIdentifierIfNeeded(request.url()),
-      request.options(), request.forPreload(), request.getOriginRestriction());
+      request.options(), request.speculativePreload()
+                             ? FetchContext::ReportingPolicy::SuppressReporting
+                             : FetchContext::ReportingPolicy::Report,
+      request.getOriginRestriction());
   if (blockedReason != ResourceRequestBlockedReason::None) {
     DCHECK(!substituteData.forceSynchronousLoad());
     return Block;
   }
 
   context().willStartLoadingResource(
       identifier, resourceRequest, factory.type(),
-      request.options().initiatorInfo.name, request.forPreload());
+      request.options().initiatorInfo.name,
+      (request.speculativePreload()
+           ? FetchContext::LoggingPolicy::SuppressLogging
+           : FetchContext::LoggingPolicy::Log));
   if (!request.url().isValid())
     return Abort;
 
   resourceRequest.setAllowStoredCredentials(
       request.options().allowCredentials == AllowStoredCredentials);
   return Continue;
 }
 
 Resource* ResourceFetcher::requestResource(
     FetchRequest& request,
@@ skipping 28 matching lines @@
     // to be decoded only on demand.  These data URLs are allowed to be
     // processed using the normal ResourceFetcher machinery.
     if (!resource && !isDataUrl && m_archive)
       return nullptr;
   }
   if (!resource) {
     resource =
         memoryCache()->resourceForURL(request.url(), getCacheIdentifier());
   }
 
-  // See if we can use an existing resource from the cache. If so, we need to
-  // move it to be load blocking.
-  moveCachedNonBlockingResourceToBlocking(resource, request);
+  // 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, request);
 
   const RevalidationPolicy policy = determineRevalidationPolicy(
       factory.type(), request, resource, isStaticData);
   TRACE_EVENT_INSTANT1("blink", "ResourceFetcher::determineRevalidationPolicy",
                        TRACE_EVENT_SCOPE_THREAD, "revalidationPolicy", policy);
 
   updateMemoryCacheStats(resource, policy, request, factory, isStaticData);
 
   switch (policy) {
     case Reload:
       memoryCache()->remove(resource);
     // Fall through
     case Load:
       resource = createResourceForLoading(request, request.charset(), factory);
       break;
     case Revalidate:
       initializeRevalidation(resourceRequest, resource);
       break;
     case Use:
       if (resource->isLinkPreload() && !request.isLinkPreload())
         resource->setLinkPreload(false);
       break;
   }
   if (!resource)
     return nullptr;
   if (resource->getType() != factory.type()) {
-    DCHECK(request.forPreload());
+    DCHECK(request.speculativePreload() || request.isLinkPreload());
+    // TODO(yoav): What's the scenario where this can actually happen?

[Charlie Harrison, 2017/02/06 19:54:16]

I can't think/see of any and this code is decently old (~June 2015). I would
support removing this block and adding CHECK_EQ(resource->getType(),
factory.type()). Possibly CQ will complain if this actually gets hit.

[Yoav Weiss, 2017/02/06 23:15:49]

I'll do that in a followup

     return nullptr;
   }
 
   if (!resource->isAlive())
     m_deadStatsRecorder.update(policy);
 
   if (policy != Use)
     resource->setIdentifier(identifier);
 
-  if (!request.forPreload() || policy != Use) {
+  // TODO(yoav): It is not clear why preloads are exempt from this check. Can we

[Charlie Harrison, 2017/02/06 19:54:16]

This logic is very old [1], and priority calculations based on preloads should
not live here. Let's try changing it in a follow-up?

[1]
https://chromium.googlesource.com/chromium/src/+/79ff7ef8ea33fa66ff2792a7c432...

[Yoav Weiss, 2017/02/06 23:15:49]

sure

+  // remove the exemption?
+  if (!request.speculativePreload() || policy != Use) {
     // When issuing another request for a resource that is already in-flight
     // make sure to not demote the priority of the in-flight request. If the new
     // request isn't at the same priority as the in-flight request, only allow
     // promotions. This can happen when a visible image's priority is increased
     // and then another reference to the image is parsed (which would be at a
     // lower priority).
     if (resourceRequest.priority() > resource->resourceRequest().priority())
       resource->didChangePriority(resourceRequest.priority(), 0);
+    // TODO(yoav): I'd expect the stated scenario to not go here, as its policy
+    // would be Use.
   }
 
   // If only the fragment identifiers differ, it is the same resource.
   DCHECK(equalIgnoringFragmentIdentifier(resource->url(), request.url()));
   requestLoadStarted(
       identifier, resource, request,
       policy == Use ? ResourceLoadingFromCache : ResourceLoadingFromNetwork,
       isStaticData);
   m_documentResources.set(
       MemoryCache::removeFragmentIdentifierIfNeeded(request.url()), resource);
@@ skipping 101 matching lines @@
   const String cacheIdentifier = getCacheIdentifier();
   DCHECK(!memoryCache()->resourceForURL(request.resourceRequest().url(),
                                         cacheIdentifier));
 
   RESOURCE_LOADING_DVLOG(1) << "Loading Resource for "
                             << request.resourceRequest().url().elidedString();
 
   Resource* resource =
       factory.create(request.resourceRequest(), request.options(), charset);
   resource->setLinkPreload(request.isLinkPreload());
-  if (request.forPreload()) {
+  if (request.speculativePreload()) {
     resource->setPreloadDiscoveryTime(request.preloadDiscoveryTime());
   }
   resource->setCacheIdentifier(cacheIdentifier);
 
   // - Don't add main resource to cache to prevent reuse.
   // - Don't add the resource if its body will not be stored.
   if (factory.type() != Resource::MainResource &&
       request.options().dataBufferingPolicy != DoNotBufferData) {
     memoryCache()->add(resource);
   }
@@ skipping 82 matching lines @@
   if (existingResource->mustRefetchDueToIntegrityMetadata(fetchRequest)) {
     RecordSriResourceIntegrityMismatchEvent(RefetchDueToIntegrityMismatch);
     return Reload;
   }
 
   // Service Worker's CORS fallback message must not be cached.
   if (existingResource->response().wasFallbackRequiredByServiceWorker())
     return Reload;
 
   // We already have a preload going for this URL.
-  if (fetchRequest.forPreload() && existingResource->isPreloaded())
+  if (fetchRequest.speculativePreload() && existingResource->isPreloaded())
     return Use;
 
   // If the same URL has been loaded as a different type, we need to reload.
   if (existingResource->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.";
@@ skipping 647 matching lines @@
     const KURL& url,
     WebURLRequest::RequestContext requestContext,
     const AtomicString& initiatorName) {
   if (cachedResource(url))
     return;
   ResourceRequest resourceRequest(url);
   resourceRequest.setRequestContext(requestContext);
   FetchRequest request(resourceRequest, initiatorName, resource->options());
   context().canRequest(resource->getType(), resource->lastResourceRequest(),
                        resource->lastResourceRequest().url(), request.options(),
-                       false, request.getOriginRestriction());
+                       FetchContext::ReportingPolicy::Report,
+                       request.getOriginRestriction());
   requestLoadStarted(resource->identifier(), resource, request,
                      ResourceLoadingFromCache);
 }
 
 ResourceFetcher::DeadResourceStatsRecorder::DeadResourceStatsRecorder()
     : m_useCount(0), m_revalidateCount(0), m_loadCount(0) {}
 
 ResourceFetcher::DeadResourceStatsRecorder::~DeadResourceStatsRecorder() {
   DEFINE_THREAD_SAFE_STATIC_LOCAL(
       CustomCountHistogram, hitCountHistogram,
@@ skipping 32 matching lines @@
   visitor->trace(m_context);
   visitor->trace(m_archive);
   visitor->trace(m_loaders);
   visitor->trace(m_nonBlockingLoaders);
   visitor->trace(m_documentResources);
   visitor->trace(m_preloads);
   visitor->trace(m_resourceTimingInfoMap);
 }
 
 }  // namespace blink