Make ResourceFetcher return Resources with LoadError instead of nullptrs.

third_party/WebKit/Source/core/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
     License as published by the Free Software Foundation; either
@@ skipping 361 matching lines @@
     resource->setIdentifier(createUniqueIdentifier());
     resource->setCacheIdentifier(cacheIdentifier);
     resource->finish();
 
     if (!substituteData.isValid())
         memoryCache()->add(resource);
 
     return resource;
 }
 
+Resource* ResourceFetcher::resourceForBlockedRequest(const FetchRequest& request, const ResourceFactory& factory, bool needsSynchronousCacheHit)
+{
+    Resource* resource = factory.create(request.resourceRequest(), request.options(), request.charset());
+    resource->setNeedsSynchronousCacheHit(needsSynchronousCacheHit);

[Nate Chapin, 2016/08/09 22:43:23]

Is there a case where needsSynchronousCacheHit is true and we would block the
load? That should only be used by SubstituteData loads or simliar, which I
would've thought are never blocked.

[engedy, 2016/08/17 16:41:56]

Looking through the clients that use `needsSynchronousCacheHit`, I would not be
surprised if that was the case, but hard to say for sure. I essentially added
this line for consistency with the cache hit scenario, regardless of whether it
can happen in practice.

Happy to look into it more, and remove if you think it is misleading.

[Nate Chapin, 2016/08/17 20:05:05]

SubstituteData is very trusted and gets to short circuit a lot of things, it
should be pretty safe to assume it's not failing canRequest(). Do a cursory
check at least, but having refreshed my memory slightly, I'd be very surprised
if it ever fails.

[engedy, 2016/09/21 21:04:01]

What do you think about adding a DCHECK?

[Nate Chapin, 2016/09/21 23:23:41]

Works for me.

+    resource->error(ResourceError::cancelledDueToAccessCheckError(request.url()));
+    return resource;
+}
+
 void ResourceFetcher::moveCachedNonBlockingResourceToBlocking(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() && resource->isLinkPreload() && !request.forPreload()) {
         m_nonBlockingLoaders.remove(resource->loader());
         m_loaders.add(resource->loader());
     }
 }
 
 void ResourceFetcher::updateMemoryCacheStats(Resource* resource, RevalidationPolicy policy, const FetchRequest& request,  const ResourceFactory& factory, bool isStaticData) const
@@ skipping 24 matching lines @@
     if (request.resourceRequest().httpHeaderField("Upgrade-Insecure-Requests") != AtomicString("1"))
         context().upgradeInsecureRequest(request.mutableResourceRequest());
     context().addClientHintsIfNecessary(request);
     context().addCSPHeaderIfNecessary(factory.type(), request);
 
     TRACE_EVENT1("blink", "ResourceFetcher::requestResource", "url", urlForTraceEvent(request.url()));
 
     if (!request.url().isValid())
         return nullptr;
 
-    if (!context().canRequest(factory.type(), request.resourceRequest(), MemoryCache::removeFragmentIdentifierIfNeeded(request.url()), request.options(), request.forPreload(), request.getOriginRestriction()))
-        return nullptr;
+    if (!context().canRequest(factory.type(), request.resourceRequest(), MemoryCache::removeFragmentIdentifierIfNeeded(request.url()), request.options(), request.forPreload(), request.getOriginRestriction())) {

[Nate Chapin, 2016/08/09 22:43:23]

Is it possible to move this check later, so that we don't need a means for
manually creating a special-case Resource?

[engedy, 2016/08/17 16:41:56]

It is possible, but unless I misunderstood your suggestion, it makes the code
later somewhat awkward, and make it less obvious that the load gets blocked:

 1.) Try to get existing Resource from MemoryCache.
 2.) Force RevalidationPolicy to be Load/Reload.
 3.) Remove existing Resource from cache.
 4.) Call createResourceForLoading.
 5.) Fail resource without loading.

[Nate Chapin, 2016/08/17 20:05:05]

Hrm, I guess the hard case here is for the Resource that is already in the cache
and has valid uses, but that needs to be forbidden for this request. Evicting
resources because someone requested them illegally is definitely not ideal.

You've sold me on leaving it here :)

+        return resourceForBlockedRequest(request, factory, substituteData.forceSynchronousLoad());
+    }
 
     unsigned long identifier = createUniqueIdentifier();
     request.mutableResourceRequest().setPriority(computeLoadPriority(factory.type(), request, ResourcePriority::NotVisible));
     initializeResourceRequest(request.mutableResourceRequest(), factory.type(), request.defer());
     context().willStartLoadingResource(identifier, request.mutableResourceRequest(), factory.type());
     if (!request.url().isValid())
         return nullptr;
 
     if (!request.forPreload()) {
         V8DOMActivityLogger* activityLogger = nullptr;
@@ skipping 876 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