Update: per revised design, instead of adding new ResourceLoader -> NavigationThrottle for plumbing, we'll add a new NavigationThrottle::OnRequestFailed() method, triggered from the existing NavigaitonRequest::OnRequestFailed. We'll also need to add net::SSLInfo to the calling code to pass the request's SSL info into OnRequestFailed: the throttle will need this information to decide what error page to show.

estark@:

Now that we've moving back to a "negative" case ("Failed" instead of "continue"), the ThrottleCheckResult [1] semantics are reversed again – they describe what happens to the *navigation*, not what happens to the event we are notifying the throttle about.

We could change the names and comments of ThrottleCheckResult so that they describe the event e.g. "PROCEED" for OnRequestFailed means "proceed with the failure". But the opposite meanings of some of the other values don't make natural sense.

Do you think it makes sense to keep the existing semantics? (ThrottleCheckResult describes what happens to the navigation.) If so, any opinions on reusing "CANCEL" vs. adding a "FAIL" value?

[1] https://cs.chromium.org/chromium/src/content/public/browser/navigation_throttle.h?type=cs&q=ThrottleCheckResult&sq=package:chromium&l=19

Re comment 6: yeah, clamy, nasko, and I have been discussing this a bit... There are a few different options, but personally the one that I think is simplest and most intuitive is to give the return values the following meanings:
- PROCEED means "proceed to fail the request as normal", i.e. the throttle doesn't want to do anything special to handle the navigation
- CANCEL/CANCEL_AND_IGNORE cancel the navigation, meaning that they cause it to fail with an ERR_ABORTED (IIRC) error code instead of whatever error code actually happened. (We might want to rename from CANCEL to something else to make it more clear...?)
- DEFER would allow the throttle to later call Resume() (the equivalent of returning PROCEED) or CancelDeferredNavigation() (the equivalent of CANCEL) or, once we add it in  issue 751946 , CancelDeferredNavigationWithCustomError() to specify the error code and error page HTML once it's ready.

clamy also raised the option of getting rid of the return values of the NavigationThrottle methods and just expecting the throttles to call the appropriate Resume()/Cancel()/etc. methods directly, but nasko pointed out that it gets a little messy with the NavigationHandle -> NavigationThrottle -> NavigationHandle re-entrancy.

> CANCEL/CANCEL_AND_IGNORE cancel the navigation, meaning that they cause it to fail with an ERR_ABORTED (IIRC) error code instead of whatever error code actually happened.

If we copy the other ___ChecksComplete() implementations in NavigationRequest, this would mean calling OnRequestFailed() again with ERR_ABORTED, which would get sent to the throttles again.

Come to think of it, the outcome of most NavigationThrottle::CANCEL/BLOCK values for existing throttle methods all go through the throttles via OnRequestFailed again.

Is that okay, or should we create a version of NavigationRequest::OnRequestFailed() that bypasses throttles?

I feel like for existing throttle methods, it's okay if cancelling them goes through NavigationRequest::OnRequestFailed, but for NavigationThrottle::OnRequestFailed itself, it shouldn't recurse (i.e. cancelling a request from NavigationThrottle::OnRequestFailed shouldn't result in the NavigationThrottle::OnRequestFailed throttles getting called again). So I guess that would mean instead of copying the __ChecksComplete implementation exactly, you'd do something a little different. e.g. separate the part of OnRequestFailed that runs after the throttles into a helper method and call that from the OnRequestFailedChecksComplete method, instead of calling OnRequestFailed.

That's what I'd try first but clamy might have another preferred option.

Actually, looks like I had copied some code incorrectly and NavigationThrottle::OnRequestFailed won't call itself recursively.  😌

For the record, comment #10 was wrong.

I *think* estark@ is correct and we can call the OnRequestFailedChecksComplete() helper like normal, as long as we update the stored net error code first.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0efab6d6a82803047ab4b0181170d611d847f275

commit 0efab6d6a82803047ab4b0181170d611d847f275
Author: Lucas Garron <lgarron@chromium.org>
Date: Wed Aug 30 22:28:51 2017

Plumb certificate error information to NavigationRequest.

This CL is the first step of the Committed Interstitials project
(https://crbug.com/448486). We are planning to make certificate errors fail the
main navigation so that we can trigger SSL interstitials (and similar error
pages) from NavigationRequest::OnRequestFailed().

This CL adds plumbing to calculate and pass the necessary information to
OnRequestFailed() in the case of a certificate error:
- The net::SSLInfo for the request.
- A boolean indicating whether SSL errors for the request's host should be fatal.

In follow-up CLs, we will:
- Pass the certificate error information into a NavigationThrottle in order to
  trigger interstitials.
- Modify ChromeContentBrowserClient::AllowCertificateError() to cancel the
  request and trigger the certificate error code path in this CL.
- Remove the `should_ssl_errors_be_fatal` bool from this code path (and attach
  it to the SSLInfo directly).
- Attach SSLInfo to ResourceRequestCompletionStatus and implement the navigation
  service equivalent code path.

Bug:  751941 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I24243a191b621857b38218fe4aae2d94bf4f7cf3
Reviewed-on: https://chromium-review.googlesource.com/609542
Commit-Queue: Lucas Garron <lgarron@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Reviewed-by: Randy Smith <rdsmith@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498645}
[modify] https://crrev.com/0efab6d6a82803047ab4b0181170d611d847f275/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/0efab6d6a82803047ab4b0181170d611d847f275/content/browser/frame_host/navigation_request.h
[modify] https://crrev.com/0efab6d6a82803047ab4b0181170d611d847f275/content/browser/loader/navigation_resource_handler.cc
[modify] https://crrev.com/0efab6d6a82803047ab4b0181170d611d847f275/content/browser/loader/navigation_url_loader_delegate.h
[modify] https://crrev.com/0efab6d6a82803047ab4b0181170d611d847f275/content/browser/loader/navigation_url_loader_impl.cc
[modify] https://crrev.com/0efab6d6a82803047ab4b0181170d611d847f275/content/browser/loader/navigation_url_loader_impl.h
[modify] https://crrev.com/0efab6d6a82803047ab4b0181170d611d847f275/content/browser/loader/navigation_url_loader_impl_core.cc
[modify] https://crrev.com/0efab6d6a82803047ab4b0181170d611d847f275/content/browser/loader/navigation_url_loader_impl_core.h
[modify] https://crrev.com/0efab6d6a82803047ab4b0181170d611d847f275/content/browser/loader/navigation_url_loader_network_service.cc
[modify] https://crrev.com/0efab6d6a82803047ab4b0181170d611d847f275/content/browser/loader/navigation_url_loader_unittest.cc
[modify] https://crrev.com/0efab6d6a82803047ab4b0181170d611d847f275/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/0efab6d6a82803047ab4b0181170d611d847f275/content/test/test_navigation_url_loader.cc
[modify] https://crrev.com/0efab6d6a82803047ab4b0181170d611d847f275/content/test/test_navigation_url_loader_delegate.cc
[modify] https://crrev.com/0efab6d6a82803047ab4b0181170d611d847f275/content/test/test_navigation_url_loader_delegate.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b906d28563a176507117208aaeb2bc141738e541

commit b906d28563a176507117208aaeb2bc141738e541
Author: John Abd-El-Malek <jam@chromium.org>
Date: Thu Aug 31 19:36:43 2017

Fix crashes in network service path after r498645.

Also take care of the TODO to pass in SSLInfo while changing this code path.
https://build.chromium.org/p/chromium.fyi/builders/Mojo%20Linux?numbuilds=200

Bug:  751941 
Change-Id: Ib6f41cdf828f23d4c55c2c8c0cd3d56d6660525e
Reviewed-on: https://chromium-review.googlesource.com/646231
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Lucas Garron <lgarron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498954}
[modify] https://crrev.com/b906d28563a176507117208aaeb2bc141738e541/content/browser/loader/navigation_url_loader_network_service.cc
[modify] https://crrev.com/b906d28563a176507117208aaeb2bc141738e541/content/browser/loader/navigation_url_loader_network_service.h

 Issue 762333  has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0cedd9686604c1fdfaa5d037ee5c250e4dbcf297

commit 0cedd9686604c1fdfaa5d037ee5c250e4dbcf297
Author: Lucas Garron <lgarron@chromium.org>
Date: Tue Oct 17 07:23:33 2017

Committed interstitials cert info (3/3): Add WillFailRequest() throttle method.

The committed interstitial project will use a navigation throttle [1] to watch
for requests that failed due to cert errors. WillFailRequest() will allow it to
decide how to respond, and provide custom error HTML for SSL interstitials.

[1] crrev.com/c/621236

Bug:  751941 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: I9a597851d018e3e42d884c185ba2c619b9d31923
Reviewed-on: https://chromium-review.googlesource.com/621873
Commit-Queue: Lucas Garron <lgarron@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#509314}
[modify] https://crrev.com/0cedd9686604c1fdfaa5d037ee5c250e4dbcf297/content/browser/frame_host/navigation_handle_impl.cc
[modify] https://crrev.com/0cedd9686604c1fdfaa5d037ee5c250e4dbcf297/content/browser/frame_host/navigation_handle_impl.h
[modify] https://crrev.com/0cedd9686604c1fdfaa5d037ee5c250e4dbcf297/content/browser/frame_host/navigation_handle_impl_browsertest.cc
[modify] https://crrev.com/0cedd9686604c1fdfaa5d037ee5c250e4dbcf297/content/browser/frame_host/navigation_handle_impl_unittest.cc
[modify] https://crrev.com/0cedd9686604c1fdfaa5d037ee5c250e4dbcf297/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/0cedd9686604c1fdfaa5d037ee5c250e4dbcf297/content/browser/frame_host/navigation_request.h
[modify] https://crrev.com/0cedd9686604c1fdfaa5d037ee5c250e4dbcf297/content/public/browser/navigation_throttle.cc
[modify] https://crrev.com/0cedd9686604c1fdfaa5d037ee5c250e4dbcf297/content/public/browser/navigation_throttle.h
[modify] https://crrev.com/0cedd9686604c1fdfaa5d037ee5c250e4dbcf297/content/public/test/navigation_simulator.cc
[modify] https://crrev.com/0cedd9686604c1fdfaa5d037ee5c250e4dbcf297/content/public/test/navigation_simulator.h
[modify] https://crrev.com/0cedd9686604c1fdfaa5d037ee5c250e4dbcf297/content/public/test/test_navigation_throttle.cc
[modify] https://crrev.com/0cedd9686604c1fdfaa5d037ee5c250e4dbcf297/content/public/test/test_navigation_throttle.h
[modify] https://crrev.com/0cedd9686604c1fdfaa5d037ee5c250e4dbcf297/content/test/navigation_simulator_unittest.cc