Make ResourceFetcher return Resources with LoadError instead of nullptrs.

Make ResourceFetcher return Resources with LoadError instead of nullptrs.

Before this change, when a request was immediately blocked by access checks,
ResourceFetcher::requestResource() would return a nullptr, while returning a
Resource instance with a LoadError set if the request was blocked only after
following one or more redirects. This resulted in various resource types
failing in inconsistent ways in the two cases.

This CL makes this behavior consistent by making requestResource() always
return a Resource instance with a LoadError. This allows for cleaner and more
consistent handling of errors in ResourceClients, as wells as more flexibility
should the client choose to distinguish the underlying causes of errors. Due
to the infrequency of requests failing access checks during normal use, the
performance impact should be negligible.

BUG=616234
Committed: https://crrev.com/fd02740210e0152c94bd4c34a1e355280421730d
Cr-Commit-Position: refs/heads/master@{#423358}

[engedy, 2016-08-09 18:46:30]

Description was changed from

==========
Resource instance with LoadError vs. nullptr on access check failure.

When a request was immediately blocked by access checks,
ResourceFetcher::requestResource() used to a return a nullptr, while returning
a Resource instance with a LoadError if the request was blocked only after
following one or more redirects. This resulted in various resource types
failing in inconsistent ways in the two cases.

This CL makes this behavior consistent by making requestResource() always
return a Resource instance with a LoadError. This allows for cleaner and more
consistent handling of errors in ResourceClients. Due to the infrequency of
requests failing access checks during normal use, the performance impact should
be negligible.

BUG=616234
==========

to

==========
Return Resource with LoadError instead of nullptr.

When a request was immediately blocked by access checks,
ResourceFetcher::requestResource() used to a return a nullptr, while returning
a Resource instance with a LoadError if the request was blocked only after
following one or more redirects. This resulted in various resource types
failing in inconsistent ways in the two cases.

This CL makes this behavior consistent by making requestResource() always
return a Resource instance with a LoadError. This allows for cleaner and more
consistent handling of errors in ResourceClients. Due to the infrequency of
requests failing access checks during normal use, the performance impact should
be negligible.

BUG=616234
==========

[engedy, 2016-08-09 18:46:30]

engedy@chromium.org changed reviewers:
+ japhet@chromium.org, mkwst@chromium.org

[engedy, 2016-08-09 18:49:07]

Description was changed from

==========
Return Resource with LoadError instead of nullptr.

When a request was immediately blocked by access checks,
ResourceFetcher::requestResource() used to a return a nullptr, while returning
a Resource instance with a LoadError if the request was blocked only after
following one or more redirects. This resulted in various resource types
failing in inconsistent ways in the two cases.

This CL makes this behavior consistent by making requestResource() always
return a Resource instance with a LoadError. This allows for cleaner and more
consistent handling of errors in ResourceClients. Due to the infrequency of
requests failing access checks during normal use, the performance impact should
be negligible.

BUG=616234
==========

to

==========
Return Resource with LoadError instead of nullptr.

When a request was immediately blocked by access checks,
ResourceFetcher::requestResource() used to a return a nullptr, while returning
a Resource instance with a LoadError if the request was blocked only after
following one or more redirects. This resulted in various resource types
failing in inconsistent ways in the two cases.

This CL makes this behavior consistent by making requestResource() always
return a Resource instance with a LoadError. This allows for cleaner and more
consistent handling of errors in ResourceClients, as wells as more flexibility
should the client choose to distinguish the underlying causes  of errors. Due
to the infrequency of requests failing access checks during normal use, the
performance impact should be negligible.

BUG=616234
==========

[engedy, 2016-08-09 18:49:55]

engedy@chromium.org changed reviewers:
- mkwst@chromium.org

[engedy, 2016-08-09 18:50:16]

Description was changed from

==========
Return Resource with LoadError instead of nullptr.

When a request was immediately blocked by access checks,
ResourceFetcher::requestResource() used to a return a nullptr, while returning
a Resource instance with a LoadError if the request was blocked only after
following one or more redirects. This resulted in various resource types
failing in inconsistent ways in the two cases.

This CL makes this behavior consistent by making requestResource() always
return a Resource instance with a LoadError. This allows for cleaner and more
consistent handling of errors in ResourceClients, as wells as more flexibility
should the client choose to distinguish the underlying causes  of errors. Due
to the infrequency of requests failing access checks during normal use, the
performance impact should be negligible.

BUG=616234
==========

to

==========
Return Resource with LoadError instead of nullptr.

When a request was immediately blocked by access checks,
ResourceFetcher::requestResource() used to a return a nullptr, while returning
a Resource instance with a LoadError if the request was blocked only after
following one or more redirects. This resulted in various resource types
failing in inconsistent ways in the two cases.

This CL makes this behavior consistent by making requestResource() always
return a Resource instance with a LoadError. This allows for cleaner and more
consistent handling of errors in ResourceClients, as wells as more flexibility
should the client choose to distinguish the underlying causes of errors. Due
to the infrequency of requests failing access checks during normal use, the
performance impact should be negligible.

BUG=616234
==========

[engedy, 2016-08-09 19:23:39]

@Nate, could you please take an initial look?

What I have done so far is the following:

 -- Audited subclasses of Resource and ResourceClient and verified that they do
not extend base class functionality in a way that would be broken by the direct
state transition from NotStarted->LoadError.

 -- Also double-checked that overrides of checkNotify(), and friends, do not
interfere with the deferred callback mechanism implemented in Resource, so no
ResourceClients will get unexpected synchronous completions. This is true except
for ImageResource, whose observers are fired synchronously in any case (but
those seem to be able to handle it).

 -- Audited call sites that fetch resources of types Image, StyleSheet, Font and
Script, to ensure that they can handle not only synchronous results, but also
synchronous errors. I found one place, WorkletScriptLoader, that would
misbehave, but it misbehaves already for synchronous successes today.

 -- I have run all layout tests with an ASAN build, no issues found.

 -- I have manually stress-tested a custom ASAN build that was rigged to fail
requests randomly with 10% probability to exercise to new approach, no issues
found.

So at least the change should not fundamentally break the world, but still, I
would very much welcome additional scrutiny. Once you are satisfied with the
change, I propose monitoring crashes very closely on Canary, and reverting in
case any unexpected issues surface.

Before that, however, there are about 15 layout tests that will need to be
fixed, the most popular failure reason being that we now have placeholders for
failed images, and failed stylesheets show up in document.styleSheets with 0
stlye rules. Also, I wonder, do we want a specific unit tests for this?

[Nate Chapin, 2016-08-09 22:43:24]

Thanks for working on this!

https://codereview.chromium.org/2231523002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2231523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/Resource.cpp:659: static bool
shouldSendCachedDataOrErrorSynchronouslyForType(Resource::Type type)
This name was already too long :/  Maybe typeNeedsSynchronousCacheHit()?

https://codereview.chromium.org/2231523002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2231523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:385:
resource->setNeedsSynchronousCacheHit(needsSynchronousCacheHit);
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.

https://codereview.chromium.org/2231523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:434: if
(!context().canRequest(factory.type(), request.resourceRequest(),
MemoryCache::removeFragmentIdentifierIfNeeded(request.url()), request.options(),
request.forPreload(), request.getOriginRestriction())) {
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:57]

Sorry for the delay. Please see some follow-up questions below.

https://codereview.chromium.org/2231523002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/Resource.cpp (right):

https://codereview.chromium.org/2231523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/Resource.cpp:659: static bool
shouldSendCachedDataOrErrorSynchronouslyForType(Resource::Type type)
On 2016/08/09 22:43:23, Nate Chapin wrote:
> This name was already too long :/  Maybe typeNeedsSynchronousCacheHit()?

Done.

https://codereview.chromium.org/2231523002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2231523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:385:
resource->setNeedsSynchronousCacheHit(needsSynchronousCacheHit);
On 2016/08/09 22:43:23, Nate Chapin wrote:
> 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.

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.

https://codereview.chromium.org/2231523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:434: if
(!context().canRequest(factory.type(), request.resourceRequest(),
MemoryCache::removeFragmentIdentifierIfNeeded(request.url()), request.options(),
request.forPreload(), request.getOriginRestriction())) {
On 2016/08/09 22:43:23, Nate Chapin wrote:
> Is it possible to move this check later, so that we don't need a means for
> manually creating a special-case Resource?

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.

[engedy, 2016-08-17 16:43:04]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-17 16:43:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-17 17:50:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-17 17:50:57]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

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

https://codereview.chromium.org/2231523002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2231523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:385:
resource->setNeedsSynchronousCacheHit(needsSynchronousCacheHit);
On 2016/08/17 16:41:56, engedy wrote:
> On 2016/08/09 22:43:23, Nate Chapin wrote:
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/2231523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:434: if
(!context().canRequest(factory.type(), request.resourceRequest(),
MemoryCache::removeFragmentIdentifierIfNeeded(request.url()), request.options(),
request.forPreload(), request.getOriginRestriction())) {
On 2016/08/17 16:41:56, engedy wrote:
> On 2016/08/09 22:43:23, Nate Chapin wrote:
> > Is it possible to move this check later, so that we don't need a means for
> > manually creating a special-case Resource?
> 
> 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.

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 :)

[engedy, 2016-09-21 16:47:22]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[engedy, 2016-09-21 16:47:47]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 16:48:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 18:05:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 18:05:33]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[engedy, 2016-09-21 20:47:04]

Patchset #3 (id:40001) has been deleted

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

I have finally had some time to get immersed in the layout test failures, and
have finished a rough first pass at addressing breakages.

Summary of changes:

 1.) When images are blocked by access checks, we will now expect fallback
content to be shown.

 2.) When style sheets are blocked by access checks, we now expect empty
stylesheets inserted into document.styleSheets.

 3.) ResourceFetcher no longer returning NULL exposed some code new paths in XHR
such that now we need to take special care to raise a NetworkError and not
AbortError when the request is blocked by access checks.
   (a) We might want to add new tests here?
   (b) We might want to change isAccessCheck errors everywhere so that they are
not isCancellation's?

Other debatable aspects are:

 4.) Should requestResource still return nullptr if the FetchContext is nullptr?

 5.) How to better handle not treating errored-out resources as `preloaded
resources`?

https://codereview.chromium.org/2231523002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2231523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:385:
resource->setNeedsSynchronousCacheHit(needsSynchronousCacheHit);
On 2016/08/17 20:05:05, Nate Chapin wrote:
> On 2016/08/17 16:41:56, engedy wrote:
> > On 2016/08/09 22:43:23, Nate Chapin wrote:
> > > 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.
> > 
> > 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.
> 
> 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.

What do you think about adding a DCHECK?

[Nate Chapin, 2016-09-21 23:23:40]

japhet@chromium.org changed reviewers:
+ mkwst@chromium.org

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

Code changes seem pretty good, but looks like you still have a couple of failing
tests

mkwst, FYI and/or sanity check?

https://codereview.chromium.org/2231523002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp (right):

https://codereview.chromium.org/2231523002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/fetch/ResourceFetcher.cpp:385:
resource->setNeedsSynchronousCacheHit(needsSynchronousCacheHit);
On 2016/09/21 21:04:01, engedy wrote:
> On 2016/08/17 20:05:05, Nate Chapin wrote:
> > On 2016/08/17 16:41:56, engedy wrote:
> > > On 2016/08/09 22:43:23, Nate Chapin wrote:
> > > > 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.
> > > 
> > > 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.
> > 
> > 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.
> 
> What do you think about adding a DCHECK?

Works for me.

https://codereview.chromium.org/2231523002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2231523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:202: if (resource &&
!(resource->errorOccurred() && resource->resourceError().isAccessCheck()))
I don't know if this is too cute, but you don't strictly need to check
errorOccurred(). isAccessCheck() will return false if resourceError().isNull(),
so errorOccurred() should be a strictly superset of isAccessCheck().

[engedy, 2016-09-22 13:21:54]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 13:22:03]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2016-09-22 13:29:29]

Description was changed from

==========
Return Resource with LoadError instead of nullptr.

When a request was immediately blocked by access checks,
ResourceFetcher::requestResource() used to a return a nullptr, while returning
a Resource instance with a LoadError if the request was blocked only after
following one or more redirects. This resulted in various resource types
failing in inconsistent ways in the two cases.

This CL makes this behavior consistent by making requestResource() always
return a Resource instance with a LoadError. This allows for cleaner and more
consistent handling of errors in ResourceClients, as wells as more flexibility
should the client choose to distinguish the underlying causes of errors. Due
to the infrequency of requests failing access checks during normal use, the
performance impact should be negligible.

BUG=616234
==========

to

==========
Make ResourceFetcher return Resources with LoadError instead of nullptrs.

When a request was immediately blocked by access checks,
ResourceFetcher::requestResource() used to a return a nullptr, while it used
to a Resource instance with a LoadError if the request was blocked only after
following one or more redirects. This resulted in various resource types
failing in inconsistent ways in the two cases.

This CL makes this behavior consistent by making requestResource() always
return a Resource instance with a LoadError. This allows for cleaner and more
consistent handling of errors in ResourceClients, as wells as more flexibility
should the client choose to distinguish the underlying causes of errors. Due
to the infrequency of requests failing access checks during normal use, the
performance impact should be negligible.

BUG=616234
==========

[engedy, 2016-09-22 13:29:36]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[engedy, 2016-09-22 13:30:03]

Addressed leftover test failures. Please take a final look.

https://codereview.chromium.org/2231523002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2231523002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:202: if (resource &&
!(resource->errorOccurred() && resource->resourceError().isAccessCheck()))
On 2016/09/21 23:23:41, Nate Chapin wrote:
> I don't know if this is too cute, but you don't strictly need to check
> errorOccurred(). isAccessCheck() will return false if
resourceError().isNull(),
> so errorOccurred() should be a strictly superset of isAccessCheck().

Done.

[commit-bot: I haz the power, 2016-09-22 13:30:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-22 15:40:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 15:40:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[engedy, 2016-09-22 16:54:48]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-22 16:55:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Nate Chapin, 2016-09-22 18:30:44]

lgtm with one last nitpick...

https://codereview.chromium.org/2231523002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2231523002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:202: if (resource &&
!resource->resourceError().isAccessCheck())
Here and in XMLHttpRequest.cpp, we should probably comment why this only applies
to isAccessCheck() errors.

[commit-bot: I haz the power, 2016-09-22 18:51:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-22 18:51:37]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[engedy, 2016-09-26 09:07:31]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 09:07:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 10:06:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 10:06:24]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[engedy, 2016-09-30 14:38:37]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 14:39:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 15:50:32]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 15:50:33]

Dry run: This issue passed the CQ dry run.

[engedy, 2016-09-30 16:01:15]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 16:01:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[engedy, 2016-09-30 16:03:14]

engedy@chromium.org changed reviewers:
+ clamy@chromium.org
- mkwst@chromium.org

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

Nate, could you please take a final look?

Camille, could you please sanity check DocumentLoader changes?

https://codereview.chromium.org/2231523002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2231523002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:202: if (resource &&
!resource->resourceError().isAccessCheck())
On 2016/09/22 18:30:44, Nate Chapin wrote:
> Here and in XMLHttpRequest.cpp, we should probably comment why this only
applies
> to isAccessCheck() errors.

Done. Please let me know if the information I put in the comments matches what
you wanted to convey.

https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:673: if
(!m_mainResource || (m_frame->settings()->browserSideNavigationEnabled() &&
m_mainResource->errorOccurred())) {
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.

[engedy, 2016-09-30 16:51:05]

Description was changed from

==========
Make ResourceFetcher return Resources with LoadError instead of nullptrs.

When a request was immediately blocked by access checks,
ResourceFetcher::requestResource() used to a return a nullptr, while it used
to a Resource instance with a LoadError if the request was blocked only after
following one or more redirects. This resulted in various resource types
failing in inconsistent ways in the two cases.

This CL makes this behavior consistent by making requestResource() always
return a Resource instance with a LoadError. This allows for cleaner and more
consistent handling of errors in ResourceClients, as wells as more flexibility
should the client choose to distinguish the underlying causes of errors. Due
to the infrequency of requests failing access checks during normal use, the
performance impact should be negligible.

BUG=616234
==========

to

==========
Make ResourceFetcher return Resources with LoadError instead of nullptrs.

Before this change, when a request was immediately blocked by access checks,
ResourceFetcher::requestResource() would return a nullptr, while returning a
Resource instance with a LoadError set if the request was blocked only after
following one or more redirects. This resulted in various resource types
failing in inconsistent ways in the two cases.

This CL makes this behavior consistent by making requestResource() always
return a Resource instance with a LoadError. This allows for cleaner and more
consistent handling of errors in ResourceClients, as wells as more flexibility
should the client choose to distinguish the underlying causes of errors. Due
to the infrequency of requests failing access checks during normal use, the
performance impact should be negligible.

BUG=616234
==========

[commit-bot: I haz the power, 2016-09-30 17:06:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 17:06:25]

Dry run: This issue passed the CQ dry run.

[Nate Chapin, 2016-09-30 23:10:51]

lgtm

[jkarlin, 2016-10-03 17:14:25]

jkarlin@chromium.org changed reviewers:
+ jkarlin@chromium.org

[jkarlin, 2016-10-03 17:14:26]

Ping, any chance this will land soon? I'd like to land a dependent CL. Thanks!

[jkarlin, 2016-10-03 17:17:16]

On 2016/10/03 17:14:26, jkarlin wrote:
> Ping, any chance this will land soon? I'd like to land a dependent CL. Thanks!

Oops, looks like this was just updated. Sorry for being impatient!

[engedy, 2016-10-03 18:16:21]

@Camille, friendly ping.  Could you please sanity check DocumentLoader changes?

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

Thanks! One comment below.

https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:673: if
(!m_mainResource || (m_frame->settings()->browserSideNavigationEnabled() &&
m_mainResource->errorOccurred())) {
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.

[engedy, 2016-10-04 23:09:48]

engedy@chromium.org changed reviewers:
+ creis@chromium.org, nick@chromium.org

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

Adding Charlie and Nick for their thoughts.

https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:673: if
(!m_mainResource || (m_frame->settings()->browserSideNavigationEnabled() &&
m_mainResource->errorOccurred())) {
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.

[Bryan McQuade, 2016-10-05 14:52:21]

On 2016/10/04 at 23:11:22, engedy wrote:
> Adding Charlie and Nick for their thoughts.
> 
>
https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):
> 
>
https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/loader/DocumentLoader.cpp:673: if
(!m_mainResource || (m_frame->settings()->browserSideNavigationEnabled() &&
m_mainResource->errorOccurred())) {
> 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.

Thanks for working through this. As Josh mentioned, we are hoping to land a
change dependent on this one for an M55 launch, so if possible, it'd be very
helpful to get this landed in some form soon (today?), and file a bug to address
any remaining issues looking forward, if that's feasible. Thank you!

[Charlie Reis, 2016-10-05 19:53:15]

creis@chromium.org changed reviewers:
+ alexmos@chromium.org

[Charlie Reis, 2016-10-05 19:53:15]

[+alexmos]

Sorry for the delayed reply.  Alex, can you take a look at the OOPIF /
renderer-side mixed content blocking issue brought up in
https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
?  I think you've dealt with similar bugs before, where we didn't commit after
deciding we were going to.

If something needs to land today, then we should make sure it supports OOPIFs in
non-PlzNavigate mode at least.  (PlzNavigate won't be enabled in M55, but OOPIFs
are.)  Thanks!

[alexmos, 2016-10-05 20:58:53]

On 2016/10/05 19:53:15, Charlie Reis wrote:
> [+alexmos]
> 
> Sorry for the delayed reply.  Alex, can you take a look at the OOPIF /
> renderer-side mixed content blocking issue brought up in
>
https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
> ?  I think you've dealt with similar bugs before, where we didn't commit after
> deciding we were going to.
> 
> If something needs to land today, then we should make sure it supports OOPIFs
in
> non-PlzNavigate mode at least.  (PlzNavigate won't be enabled in M55, but
OOPIFs
> are.)  Thanks!

See my comment below.  Overall, I think this is ok, as it seems to affect only
the OOPIFs+PlzNavigate combination, but can you please run a
linux_site_isolation try job to confirm this isn't breaking any tests with
OOPIFs?  The default try set only runs a subset of OOPIF tests.

https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:673: if
(!m_mainResource || (m_frame->settings()->browserSideNavigationEnabled() &&
m_mainResource->errorOccurred())) {
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 22:42:34]

The CQ bit was checked by engedy@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 22:43:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

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

Thank you both. Alex, please see my responses inline.

https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:673: if
(!m_mainResource || (m_frame->settings()->browserSideNavigationEnabled() &&
m_mainResource->errorOccurred())) {
> 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?

Yes, that's my understanding.

> 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). 

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'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.

I'll file a bug on that too.

[commit-bot: I haz the power, 2016-10-06 00:18:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 00:18:24]

Dry run: This issue passed the CQ dry run.

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

https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:673: if
(!m_mainResource || (m_frame->settings()->browserSideNavigationEnabled() &&
m_mainResource->errorOccurred())) {
On 2016/10/05 23:26:25, engedy wrote:
> > 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?
> 
> Yes, that's my understanding.
> 
> > 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). 
> 
> 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?  

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.  

> 
> > 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.
> 
> I'll file a bug on that too.

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

https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/2231523002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:673: if
(!m_mainResource || (m_frame->settings()->browserSideNavigationEnabled() &&
m_mainResource->errorOccurred())) {
On 2016/10/06 00:24:06, alexmos wrote:
> On 2016/10/05 23:26:25, engedy wrote:
> > > 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?
> > 
> > Yes, that's my understanding.
> > 
> > > 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). 
> > 
> > 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?  
> 
> 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. 

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

[engedy, 2016-10-06 00:32:37]

engedy@chromium.org changed reviewers:
- clamy@chromium.org, creis@chromium.org, jkarlin@chromium.org,
nick@chromium.org

[engedy, 2016-10-06 00:37:40]

Okay, looks like all specific concerns have been addressed, landing this now to
unblock dependent CL. Will file the two tracking bugs mentioned above, and happy
to follow-up in case there are any other concerns.

[engedy, 2016-10-06 00:39:50]

The CQ bit was checked by engedy@chromium.org

[engedy, 2016-10-06 00:39:50]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/2231523002/#ps160001
(title: "Rebase, resolve comment rewrap conflicts.")

[commit-bot: I haz the power, 2016-10-06 00:40:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-06 00:47:56]

Description was changed from

==========
Make ResourceFetcher return Resources with LoadError instead of nullptrs.

Before this change, when a request was immediately blocked by access checks,
ResourceFetcher::requestResource() would return a nullptr, while returning a
Resource instance with a LoadError set if the request was blocked only after
following one or more redirects. This resulted in various resource types
failing in inconsistent ways in the two cases.

This CL makes this behavior consistent by making requestResource() always
return a Resource instance with a LoadError. This allows for cleaner and more
consistent handling of errors in ResourceClients, as wells as more flexibility
should the client choose to distinguish the underlying causes of errors. Due
to the infrequency of requests failing access checks during normal use, the
performance impact should be negligible.

BUG=616234
==========

to

==========
Make ResourceFetcher return Resources with LoadError instead of nullptrs.

Before this change, when a request was immediately blocked by access checks,
ResourceFetcher::requestResource() would return a nullptr, while returning a
Resource instance with a LoadError set if the request was blocked only after
following one or more redirects. This resulted in various resource types
failing in inconsistent ways in the two cases.

This CL makes this behavior consistent by making requestResource() always
return a Resource instance with a LoadError. This allows for cleaner and more
consistent handling of errors in ResourceClients, as wells as more flexibility
should the client choose to distinguish the underlying causes of errors. Due
to the infrequency of requests failing access checks during normal use, the
performance impact should be negligible.

BUG=616234
==========

[commit-bot: I haz the power, 2016-10-06 00:47:57]

Committed patchset #8 (id:160001)

[commit-bot: I haz the power, 2016-10-06 00:50:58]

Description was changed from

==========
Make ResourceFetcher return Resources with LoadError instead of nullptrs.

Before this change, when a request was immediately blocked by access checks,
ResourceFetcher::requestResource() would return a nullptr, while returning a
Resource instance with a LoadError set if the request was blocked only after
following one or more redirects. This resulted in various resource types
failing in inconsistent ways in the two cases.

This CL makes this behavior consistent by making requestResource() always
return a Resource instance with a LoadError. This allows for cleaner and more
consistent handling of errors in ResourceClients, as wells as more flexibility
should the client choose to distinguish the underlying causes of errors. Due
to the infrequency of requests failing access checks during normal use, the
performance impact should be negligible.

BUG=616234
==========

to

==========
Make ResourceFetcher return Resources with LoadError instead of nullptrs.

Before this change, when a request was immediately blocked by access checks,
ResourceFetcher::requestResource() would return a nullptr, while returning a
Resource instance with a LoadError set if the request was blocked only after
following one or more redirects. This resulted in various resource types
failing in inconsistent ways in the two cases.

This CL makes this behavior consistent by making requestResource() always
return a Resource instance with a LoadError. This allows for cleaner and more
consistent handling of errors in ResourceClients, as wells as more flexibility
should the client choose to distinguish the underlying causes of errors. Due
to the infrequency of requests failing access checks during normal use, the
performance impact should be negligible.

BUG=616234

Committed: https://crrev.com/fd02740210e0152c94bd4c34a1e355280421730d
Cr-Commit-Position: refs/heads/master@{#423358}
==========

[commit-bot: I haz the power, 2016-10-06 00:51:00]

Patchset 8 (id:??) landed as
https://crrev.com/fd02740210e0152c94bd4c34a1e355280421730d
Cr-Commit-Position: refs/heads/master@{#423358}

[nasko, 2016-10-06 22:16:07]

A revert of this CL (patchset #8 id:160001) has been created in
https://codereview.chromium.org/2395313002/ by nasko@chromium.org.

The reason for reverting is: Reverting since it is causing flakiness on the
blink bots. Latest flake:

https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win7%20%28dbg%...

unexpected_failures:
http/tests/security/contentSecurityPolicy/nonces/import-enforce-blocked.php.