third_party/WebKit/Source/core/loader/DocumentLoader.cpp - Issue 2231523002: Make ResourceFetcher return Resources with LoadError instead of nullptrs.

Unified Diff: third_party/WebKit/Source/core/loader/DocumentLoader.cpp

Issue 2231523002: Make ResourceFetcher return Resources with LoadError instead of nullptrs. (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Comment phrasing. Created 4 years, 3 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« no previous file with comments | « third_party/WebKit/Source/core/fetch/ResourceFetcherTest.cpp ('k') | third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

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

diff --git a/third_party/WebKit/Source/core/loader/DocumentLoader.cpp b/third_party/WebKit/Source/core/loader/DocumentLoader.cpp

index b78888927b7e4e32fbf65f20c453d3e5a5db2026..65e23c22b2dea98df58efebc71c146c932617a25 100644

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

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

@@ -199,7 +199,10 @@ Resource* DocumentLoader::startPreload(Resource::Type type, FetchRequest& reques

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;

}

@@ -662,7 +665,12 @@ void DocumentLoader::startLoadingMainResource()

(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 theore

clamy 2016/10/04 14:49:53 I'm not sure I understand fully: is this needed fo

engedy 2016/10/04 23:11:22 Thanks for clarifying. And yes -- then the proxim

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 bloc

On 2016/10/04 23:11:22, engedy wrote: > On 2016/10/04 14:49:53, clamy wrote: > > On 2016/09/30 16:03:15, engedy wrote: > > > 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. > > > > 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. > > 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.

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.

alexmos 2016/10/06 00:24:06 Not sure about layout tests for this. On the brow

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;