Make ResourceFetcher return Resources with LoadError instead of nullptrs.

third_party/WebKit/Source/core/loader/DocumentLoader.cpp

 /*
  * Copyright (C) 2006, 2007, 2008 Apple Inc. All rights reserved.
  * Copyright (C) 2011 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  *
  * 1.  Redistributions of source code must retain the above copyright
  *     notice, this list of conditions and the following disclaimer.
@@ skipping 181 matching lines @@
     case Resource::ImportResource:
         resource = RawResource::fetchImport(request, fetcher());
         break;
     case Resource::Raw:
         resource = RawResource::fetch(request, fetcher());
         break;
     default:
         NOTREACHED();
     }
 
-    if (resource)
+    // 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 && !resource->resourceError().isAccessCheck())
         fetcher()->preloadStarted(resource);
     return resource;
 }
 
 void DocumentLoader::didRedirect(const KURL& oldURL, const KURL& newURL)
 {
     timing().addRedirect(oldURL, newURL);
 
     // If a redirection happens during a back/forward navigation, don't restore
     // any state from the old HistoryItem.
@@ skipping 442 matching lines @@
     // The fetch has already started in the browser. Don't mark it again.
     if (!m_frame->settings()->browserSideNavigationEnabled()) {
         DCHECK(!timing().fetchStart());
         timing().markFetchStart();
     }
 
     DEFINE_STATIC_LOCAL(ResourceLoaderOptions, mainResourceLoadOptions,
         (DoNotBufferData, AllowStoredCredentials, ClientRequestedCredentials, CheckContentSecurityPolicy, DocumentContext));
     FetchRequest fetchRequest(m_request, FetchInitiatorTypeNames::document, mainResourceLoadOptions);
     m_mainResource = RawResource::fetchMainResource(fetchRequest, fetcher(), m_substituteData);
-    if (!m_mainResource) {
+
+    // PlzNavigate:
+    // The final access checks are still performed here, potentially rejecting
+    // the "provisional" load, but the browser side already expects the renderer
+    // to be able to unconditionally commit.
+    if (!m_mainResource || (m_frame->settings()->browserSideNavigationEnabled() && m_mainResource->errorOccurred())) {

[engedy, 2016/09/30 16:03:15]

Nate, this is new, please take a look.

The theoretical reason behind this, IIUC, is described in the comment.

The practical reason is that if this is an OOPIF, not having having the
provisional load committed will lead to no RenderFrameProxyHost (corresponding
to the parent) being created on the browser side, which will lead to
OnDidStopLoading not getting propagated to eventually call checkCompleted() on
the parent, which, in turn, will lead to browser tests hanging while waiting on
the navigation to finish.

[clamy, 2016/10/04 14:49:53]

I'm not sure I understand fully: is this needed for OOPIF, PlzNavigate or the
two of them together?

If for PlzNavigate only, I think we made a change to allow the "provisional
load" to fail. Or if we haven't done it we should, because there are other valid
reasons that may cause the renderer not ot actually commit the navigation, eg it
doesn't support the MIME type of the response. Therefore, I don't think that
this should be needed in the PlzNavigate case.

[engedy, 2016/10/04 23:11:22]

Thanks for clarifying.

And yes -- then the proximate cause indeed seems to be an issue with OOPIF,
namely, we cannot handle if the (first real) provisional load fails in a remote
child frame. In this case, the RenderFrameProxyHost representing the child
RenderFrame for the parent's SiteInstance never gets created, so there is
nothing to propagate DidStopLoading.

Even without PlzNavigate, it looks like (?) we could get into such a scenario if
a cross-site document loaded into a subframe has an unsupported MIME type.

What is causing the issue here, though, are access checks, e.g. mixed content
checks. Without PlzNavigate, these would (first) be performed in the parent's
process, killing the load before the transfer takes place.

However, we no longer perform these access checks in the parent (and not yet? in
the browser) with PlzNavigate. So turning that on as well triggers the issue
indirectly by opening up another avenue for OOPIF to encounter such a scenario.

[alexmos, 2016/10/05 20:58:52]

I ran into similar issues with OOPIFs and CSP blocking in
https://crbug.com/584845.  There, the browser process was deciding that the main
resource load was going to commit, but then it was canceled due to CSP access
checks.  I ended up fixing that in r378762 by converting it to commit an empty
document instead of canceling the load.  This looks similar.

So, IIUC, this particular check is needed only when both OOPIFs and PlzNavigate
are enabled, due to mixed content checks not stopping the load in the parent's
process with PlzNavigate?  If yes, this seems fine; as Charlie mentioned,
PlzNavigate won't be on for M55. But let's file a bug to follow up on this -- I
guess the concern is that we'll be committing an empty document for a load that
used to be canceled earlier, and we've seen cases where developers relied on an
old page still being visible after a block like this (see issue 632710).  I'm
also curious where these mixed content checks are supposed to be performed with
PlzNavigate (and whether they exist yet in the browser process); maybe Camille
can comment on that.

Also, I don't really know how unsupported MIME types would behave in OOPIFs (or
how they behave in general), but it's plausible that they could lead to similar
issues. It'd be good to file another bug for that for us to investigate this
further.

[engedy, 2016/10/05 23:26:25]

Yes, that's my understanding.
To provide context, the rest of the CL will actually result in such a change in
strictly the opposite direction, that is, some navigations where we previously
"committed an empty document" will now "fail provisional load". Adding this line
will just narrow the scope of the change by keeping PlzNavigate behavior intact.

I'll file a bug to follow-up on this. Do we already have tests to verify that
the page's origin is not made unique in this scenario?  
I'll file a bug on that too.

[alexmos, 2016/10/06 00:24:06]

Not sure about layout tests for this.  On the browser side, we've got some mixed
content and CSP blocking tests in SitePerProcessBrowserTest that exercise the
current behavior. 

Anyway, I see that linux_site_isolation is green, so this change LGTM.  

[engedy, 2016/10/06 00:32:14]

All right, will make mention of this on the bug.

         m_request = ResourceRequest(blankURL());
         maybeLoadEmpty();
         return;
     }
     // A bunch of headers are set when the underlying resource load begins, and m_request needs to include those.
     // Even when using a cached resource, we may make some modification to the request, e.g. adding the referer header.
     m_request = m_mainResource->isLoading() ? m_mainResource->resourceRequest() : fetchRequest.resourceRequest();
     m_mainResource->addClient(this);
 }
 
@@ skipping 44 matching lines @@
 {
     m_writer = createWriterFor(init, mimeType(), m_writer ? m_writer->encoding() : emptyAtom, true, ForceSynchronousParsing);
     if (!source.isNull())
         m_writer->appendReplacingData(source);
     endWriting(m_writer.get());
 }
 
 DEFINE_WEAK_IDENTIFIER_MAP(DocumentLoader);
 
 } // namespace blink