Implement error page commit policy in PlzNavigate.

content/browser/frame_host/navigation_request.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/frame_host/navigation_request.h"
 
 #include <utility>
 
 #include "base/memory/ptr_util.h"
 #include "content/browser/appcache/appcache_navigation_handle.h"
@@ skipping 551 matching lines @@
     frame_tree_node_->navigator()->DiscardPendingEntryIfNeeded(
         navigation_handle_.get());
   }
 
   // If the request was canceled by the user do not show an error page.
   if (net_error == net::ERR_ABORTED) {
     frame_tree_node_->ResetNavigationRequest(false);
     return;
   }
 
-  // Select an appropriate renderer to show the error page.
-  RenderFrameHostImpl* render_frame_host =
-      frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this);
+  // There are two types of error pages that need to be handled differently.
+  // * Error pages resulting from blocking the request, because the original
+  //   document wasn't even allowed to make the request. In such case,
+  //   the error pages should be committed in the process of the original
+  //   document, to avoid creating a process for the destination.
+  // * Error pages resulting from either network outage (no network, DNS
+  //   error, etc) or similar cases, where the user can reasonably expect that
+  //   a reload at a later point in time can be successful. Such error pages
+  //   do belong to the process that will host the destination URL, as a
+  //   reload will end up committing in that process anyway.
+  RenderFrameHostImpl* render_frame_host = nullptr;
+  if (net_error == net::ERR_BLOCKED_BY_CLIENT) {

[nasko, 2017/03/07 07:08:12]

It turns out that the webRequest API blocking navigations go through here as
well. A small patch I tried locally to only take this branch on calls
originating internally in NavigationRequest (On*ChecksComplete) passed the test.

[Charlie Reis, 2017/03/09 00:43:34]

What's the problem with using the current RFH for webRequest API blocking cases?
 Those seem like they're more in the first category than the second, since a
reload wouldn't be expected to work.

Would it be better to find a way to update the
ExtensionWebRequestApiTest.WebRequestBlocking test for the new expectation?

(I'm not fully opposed to your workaround suggestion, but I like how simple this
is at the moment.)

[nasko, 2017/03/09 07:24:09]

I've done a lot of debugging but cannot see why this CL makes the test time out.
It boils down to a callback not firing, which leaves ref-count unbalanced and
therefore it cannot finish.

Interestingly enough, if I just move around one test case, it works fine :(.
Since I'm all out of ideas, I will actually try the move this test case and
enlist the help of rdevlin.cronin@ debugging this further.

+    render_frame_host = frame_tree_node_->current_frame_host();
+  } else {
+    render_frame_host =
+        frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this);
+  }
 
   // Don't ask the renderer to commit an URL if the browser will kill it when
   // it does.
   DCHECK(render_frame_host->CanCommitURL(common_params_.url));
 
   NavigatorImpl::CheckWebUIRendererDoesNotDisplayNormalURL(render_frame_host,
                                                            common_params_.url);
 
   TransferNavigationHandleOwnership(render_frame_host);
   render_frame_host->navigation_handle()->ReadyToCommitNavigation(
@@ skipping 184 matching lines @@
   DCHECK_EQ(request_params_.has_user_gesture, begin_params_.has_user_gesture);
 
   render_frame_host->CommitNavigation(response_.get(), std::move(body_),
                                       common_params_, request_params_,
                                       is_view_source_);
 
   frame_tree_node_->ResetNavigationRequest(true);
 }
 
 }  // namespace content