Make ResourceFetcher return Resources with LoadError instead of nullptrs.

third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp

Index: third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
diff --git a/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
index 53805481da6e80d641d7c919c9e5b9dbff74b711..468ba2251fa2306fe9beb2b3ae15d381730cac10 100644
--- a/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
+++ b/third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp
@@ -379,6 +379,14 @@ Resource* ResourceFetcher::resourceForStaticData(const FetchRequest& request, co
     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.
@@ -423,8 +431,9 @@ Resource* ResourceFetcher::requestResource(FetchRequest& request, const Resource
     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));