Thanks for the report!  Confirmed on Windows 66.0.3358.0 as well.  Also seems to repro in 65.0.3325.106, though OOPIF elements aren't inline there (which was added in  issue 800613 ).

@Alex: is there a way to find out whether navigation is going to end up in the out-of-process iframe given the NavigationHandleImpl? We need to know it from within NavigationThrottle::WillFailRequest to fix this.

Hmm, good question.  By that time, we've already determined the final RenderFrameHost for the error page in NavigationRequest::OnRequestFailedInternal(), but it looks like we haven't set it on the NavigationHandleImpl yet, so something like nav_handle->GetRenderFrameHost()->IsCrossProcessSubframe() wouldn't work.

Any chance you could do your work instead from ReadyToCommitNavigation or DidFinishNavigation, or is that too late?  That should be able to check navigation_handle->GetRenderFrameHost()->IsCrossProcessSubframe() as well as determine whether the navigation handle has an error.

+nasko@/clamy@ for any other thoughts re: how to check for this, or whether we could expose the error page's final RenderFrameHost to WillFailRequest throttles.  Note that nasko@ is also working on isolating error pages in their own process (https://crrev.com/c/762520), but that will only affect main frames, not subframes, so probably won't help here.

> we haven't set it on the NavigationHandleImpl

In case of a non-error navigation, we are getting navhandle->GetRenderFrameHost() from within WillProcessResponse  and are calling frh->IsCrossProcessSubframe on it. So things work for the WillProcessResponse branch.

Things are different for WillFailRequest - navhandle does not want to return RFH unless its state is >= WILL_PROCESS_RESPONSE.

All we need to know is whether the OOP-ness has changed for a given RFH. We need to handle this transition, throttle OOPIF if necessary and set up our agents.

> Any chance you could do your work instead from ReadyToCommitNavigation or DidFinishNavigation

We need to throttle the navigation in order to set up all the agents, other signals are emitted too late. We already have the code in our Throttle::WillProcessResponse that works for regular navigations. 

>> All we need to know is whether the OOP-ness has changed for a given RFH

read as "for a given frame tree node".

@alexmos: The main issue I would see is that it's possible to modify the net error code in NavigationThrottle::WillFailRequest. So, if the new error code is of the blocking variety, then we will always use the current process instead of using the process determined for navigation (and this applies to subframes as well). One possibility could be to call ReadyToCommitNavigation when we will commit an error page, since by that time we will know for sure the final process.

#6: I'm a bit confused by this.  Looking at the code, it doesn't look like we recompute the target RFH after WillFailRequest, so even if it ends up changing the error code, we'll use whatever RFH was determined in NavigationRequest::OnRequestFailedInternal().  Am I misunderstanding something?

I.e., here's the sequence of events that I see:

1. The target RFH for the error page is first determined here in NavigationRequest::OnRequestFailedInternal:
  RenderFrameHostImpl* render_frame_host = nullptr;
  if (net_error == net::ERR_BLOCKED_BY_CLIENT && !browser_initiated()) {
    render_frame_host = frame_tree_node_->current_frame_host();
  } else {
    render_frame_host =
        frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this); // <-- errors that end up in OOPIFs go through here.
  }

2. We run WillFailRequest throttles:
    navigation_handle_->WillFailRequest(
        ssl_info, base::Bind(&NavigationRequest::OnFailureChecksComplete,
                             base::Unretained(this), render_frame_host));

3. We run the OnFailureChecksComplete() callback, which is passed the |render_frame_host| from step 1.

4. OnFailureChecksComplete() calls CommitErrorPage(), which commits the error page in the RFH from step 1:
  frame_tree_node_->TransferNavigationRequestOwnership(render_frame_host);
  navigation_handle_->ReadyToCommitNavigation(render_frame_host, true);
  render_frame_host->FailedNavigation(common_params_, request_params_,
                                      has_stale_copy_in_cache_, net_error_,
                                      error_page_content);

Well that's not how I thought it would work, and might be an issue if we decide to change the error code in WillFailRequest to net::ERR_BLOCKED_BY_CLIENT. We should probably check it again in OnFailureChecksComplete(), and change it to the current RFh in that case.

Yes, that's one option.  If we take that route, is there any reason to keep the existing RFH calculation in OnRequestFailedInternal(), rather than just moving it to OnFailureChecksComplete()?

The other option, which Nasko and I discussed yesterday, is to just let the RFH calculated in OnRequestFailedInternal() be the final RFH, and let GetRenderFrameHost() return it during WILL_FAIL_REQUEST. I don't see any throttles actually changing the error code in WillFailRequest today (looking at the two non-test ones in SSLErrorNavigationThrottle and NewTabPageNavigationThrottle) - so I'm a bit unclear why we would want to support it and prioritize over letting WillFailRequest() access the final RFH, for which Pavel has a use case here?

+carlosil and ortuno in case they foresee any need to change the error code in WillFailRequest... I don't think committed interstitials will require that functionality but they can probably double-check.

Re #10: Yeah, I don't think committed interstitials requires functionality for changing the error code on WillFailRequest, so this shouldn't conflict with CI.

Sounds like  issue 824576  might potentially unblock this by allowing access to navhandle->GetRenderFrameHost() in more states.

Comment 12:  Issue 824576  was WontFixed.  Is there another way to make progress on the fix for this one?

I think we can still implement the plan from #9 (let the RFH calculated in OnRequestFailedInternal() be the final RFH, and let GetRenderFrameHost() return it during WILL_FAIL_REQUEST).  It sounds like committed interstitials won't need to change the error code in WillFailRequest, so I don't see any reason to support that.

"let GetRenderFrameHost() return it during WILL_FAIL_REQUEST" - could you do that for us? I don't want to break anything - things look fragile around there.

I've got a CL started for this at https://crrev.com/c/1045266.

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

commit 1a66b1d944516a3d6959fbea7ae2f710db4b3e50
Author: Alex Moshchuk <alexmos@chromium.org>
Date: Tue May 15 21:18:26 2018

Allow NavigationHandle::GetRenderFrameHost() in WillFailRequest throttles.

This should be safe since the final RenderFrameHost for the error page
is already determined in NavigationRequest::OnRequestFailedInternal().
As part of this change, pass that final RenderFrameHost to
NavigationHandle::WillFailRequest and store it on NavigationHandle
before running WillFailRequest through the throttles.

One use case for this will be DevTools, which needs to know whether a
given subframe is transitioning to/from an OOPIF in the throttles,
even for error pages.  That logic is already in WillProcessResponse(),
and with this change it can also be used in WillFailRequest() to fix
inspecting error pages loaded in OOPIFs.  Note that DevTools needs to
defer the response, so simply doing the work at ReadyToCommit time
doesn't work.

Today, WillFailRequest throttles are allowed to change the error code,
but they should not be changing it in a way that alters the final
error page RenderFrameHost.  Previously, this was implicitly true
because OnFailureChecksComplete() did not check if the error page
RenderFrameHost needed to be updated.  This CL makes that assumption
explicit.  Note that with error page isolation, this won't be an issue
since all error pages will end up in the error page process, which
won't change regardless of what WillFailRequest returns.

Bug:  817881 
Change-Id: Ifc1a53a70f8a649a8d9ddc67f6fd39ebc5c1efd7
Reviewed-on: https://chromium-review.googlesource.com/1045266
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558835}
[modify] https://crrev.com/1a66b1d944516a3d6959fbea7ae2f710db4b3e50/chrome/browser/ssl/ssl_error_navigation_throttle_unittest.cc
[modify] https://crrev.com/1a66b1d944516a3d6959fbea7ae2f710db4b3e50/content/browser/frame_host/navigation_handle_impl.cc
[modify] https://crrev.com/1a66b1d944516a3d6959fbea7ae2f710db4b3e50/content/browser/frame_host/navigation_handle_impl.h
[modify] https://crrev.com/1a66b1d944516a3d6959fbea7ae2f710db4b3e50/content/browser/frame_host/navigation_handle_impl_unittest.cc
[modify] https://crrev.com/1a66b1d944516a3d6959fbea7ae2f710db4b3e50/content/browser/frame_host/navigation_request.cc
[modify] https://crrev.com/1a66b1d944516a3d6959fbea7ae2f710db4b3e50/content/browser/frame_host/navigation_request.h
[modify] https://crrev.com/1a66b1d944516a3d6959fbea7ae2f710db4b3e50/content/public/browser/navigation_handle.h

Hey Pavel (or anyone else from DevTools) - the change in c#18 should let you access NavigationHandle::GetRenderFrameHost() from WillFailRequest().  Would you be able to finish this bug up?  Per c#4, it seems you can just do the same work you're already doing in WillProcessResponse() there (I'm guessing it's mostly about TargetAutoAttacher::AutoAttachToFrame()).

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

commit 81cbb1741bd77536a9e0369b15a6fee7f48fc53c
Author: Dmitry Gozman <dgozman@chromium.org>
Date: Tue Jun 12 17:26:44 2018

[DevTools] Throttle error pages so that we attach to them in time

Bug:  817881 
Change-Id: Iea3e0b94fd241326f9f03065b0e36fa40b74a63a
Reviewed-on: https://chromium-review.googlesource.com/1096395
Reviewed-by: Alexei Filippov <alph@chromium.org>
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566480}
[modify] https://crrev.com/81cbb1741bd77536a9e0369b15a6fee7f48fc53c/content/browser/devtools/protocol/target_handler.cc
[add] https://crrev.com/81cbb1741bd77536a9e0369b15a6fee7f48fc53c/third_party/WebKit/LayoutTests/http/tests/devtools/oopif/oopif-elements-nesting-error-page-expected.txt
[add] https://crrev.com/81cbb1741bd77536a9e0369b15a6fee7f48fc53c/third_party/WebKit/LayoutTests/http/tests/devtools/oopif/oopif-elements-nesting-error-page.js
[add] https://crrev.com/81cbb1741bd77536a9e0369b15a6fee7f48fc53c/third_party/WebKit/LayoutTests/http/tests/devtools/oopif/resources/page-error.html