PlzNavigate: Call NavigationHandle::WillProcessResponse for 204/205s

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 "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/devtools/render_frame_devtools_agent_host.h"
@@ skipping 16 matching lines @@
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_data.h"
 #include "content/public/browser/navigation_ui_data.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/browser/stream_handle.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/request_context_type.h"
 #include "content/public/common/resource_response.h"
 #include "content/public/common/url_constants.h"
 #include "net/base/load_flags.h"
+#include "net/base/net_errors.h"
 #include "net/base/url_util.h"
 #include "net/http/http_request_headers.h"
 #include "net/url_request/redirect_info.h"
 #include "third_party/WebKit/public/web/WebSandboxFlags.h"
 
 namespace content {
 
 namespace {
 
 // Returns the net load flags to use based on the navigation type.
@@ skipping 172 matching lines @@
     const NavigationEntryImpl* entry)
     : frame_tree_node_(frame_tree_node),
       common_params_(common_params),
       begin_params_(begin_params),
       request_params_(request_params),
       browser_initiated_(browser_initiated),
       state_(NOT_STARTED),
       restore_type_(RestoreType::NONE),
       is_view_source_(false),
       bindings_(NavigationEntryImpl::kInvalidBindings),
+      response_should_be_rendered_(true),
       associated_site_instance_type_(AssociatedSiteInstanceType::NONE) {
   DCHECK(!browser_initiated || (entry != nullptr && frame_entry != nullptr));
   if (browser_initiated) {
     FrameNavigationEntry* frame_entry = entry->GetFrameEntry(frame_tree_node);
     if (frame_entry) {
       source_site_instance_ = frame_entry->source_site_instance();
       dest_site_instance_ = frame_entry->site_instance();
     }
 
     restore_type_ = entry->restore_type();
@@ skipping 132 matching lines @@
 
 void NavigationRequest::OnResponseStarted(
     const scoped_refptr<ResourceResponse>& response,
     std::unique_ptr<StreamHandle> body,
     const SSLStatus& ssl_status,
     std::unique_ptr<NavigationData> navigation_data,
     const GlobalRequestID& request_id,
     bool is_download,
     bool is_stream) {
   DCHECK(state_ == STARTED);
+  DCHECK(response);
   state_ = RESPONSE_STARTED;
 
-  // HTTP 204 (No Content) and HTTP 205 (Reset Content) responses should not
-  // commit; they leave the frame showing the previous page.
-  DCHECK(response);
-  if (response->head.headers.get() &&
-      (response->head.headers->response_code() == 204 ||
-       response->head.headers->response_code() == 205)) {
-    frame_tree_node_->navigator()->DiscardPendingEntryIfNeeded(
-        navigation_handle_.get());
-    frame_tree_node_->ResetNavigationRequest(false);
-    return;
-  }
+  // Check if the response should be sent to a renderer.
+  response_should_be_rendered_ =
+      !is_download && (!response->head.headers.get() ||
+                       (response->head.headers->response_code() != 204 &&
+                        response->head.headers->response_code() != 205));
+
+  // Response that will not commit should be marked as aborted in the
+  // NavigationHandle.
+  if (!response_should_be_rendered_)
+    navigation_handle_->set_net_error_code(net::ERR_ABORTED);
 
   // Update the service worker params of the request params.
   bool did_create_service_worker_host =
       navigation_handle_->service_worker_handle() &&
       navigation_handle_->service_worker_handle()
               ->service_worker_provider_host_id() !=
           kInvalidServiceWorkerProviderId;
   request_params_.service_worker_provider_id =
       did_create_service_worker_host
           ? navigation_handle_->service_worker_handle()
                 ->service_worker_provider_host_id()
           : kInvalidServiceWorkerProviderId;
 
   // Update the lofi state of the request.
   if (response->head.is_using_lofi)
     common_params_.lofi_state = LOFI_ON;
   else
     common_params_.lofi_state = LOFI_OFF;
 
   // Select an appropriate renderer to commit the navigation.
-  RenderFrameHostImpl* render_frame_host =
-      frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this);
-  NavigatorImpl::CheckWebUIRendererDoesNotDisplayNormalURL(render_frame_host,
-                                                           common_params_.url);
+  RenderFrameHostImpl* render_frame_host = nullptr;

[clamy, 2016/12/06 17:01:58]

Note that this will mean having a null RFH if
NavigationHandle::GetRenderFrameHost is called during WillProcessResponse. I
considered the alternative of passing the current RFH, but I think this is less
error-prone for users of the API.

[nasko, 2016/12/08 18:25:24]

Will that be the case only during WillProcessResponse? During
WCO::DidFinishNavigation the correct (also current) RFH will be returned, right?

[clamy, 2016/12/16 17:07:14]

No, we won't set the RFH to current since there will be no commit.

+  if (response_should_be_rendered_) {
+    render_frame_host =
+        frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this);
+    NavigatorImpl::CheckWebUIRendererDoesNotDisplayNormalURL(
+        render_frame_host, common_params_.url);
+  }
+  DCHECK(render_frame_host || !response_should_be_rendered_);
 
   // For renderer-initiated navigations that are set to commit in a different
   // renderer, allow the embedder to cancel the transfer.
-  if (!browser_initiated_ &&
+  if (!browser_initiated_ && render_frame_host &&
       render_frame_host != frame_tree_node_->current_frame_host() &&
       !frame_tree_node_->navigator()->GetDelegate()->ShouldTransferNavigation(
           frame_tree_node_->IsMainFrame())) {
     frame_tree_node_->ResetNavigationRequest(false);
     return;
   }
 
   if (navigation_data)
     navigation_handle_->set_navigation_data(std::move(navigation_data));
 
@@ skipping 127 matching lines @@
     return;
   }
 
   loader_->FollowRedirect();
 }
 
 void NavigationRequest::OnWillProcessResponseChecksComplete(
     NavigationThrottle::ThrottleCheckResult result) {
   CHECK(result != NavigationThrottle::DEFER);
 
-  // Abort the request if needed. This will destroy the NavigationRequest.
+  // Abort the request if needed. This include requests that were blocked by

[nasko, 2016/12/08 18:25:24]

nit: includes

[clamy, 2016/12/16 17:07:14]

Done.

+  // NavigationThrottles and requests that should not commit (eg downloads,

[nasko, 2016/12/08 18:25:24]

nit.: e.g.

[clamy, 2016/12/16 17:07:14]

Done.

+  // 204/205s). This will destroy the NavigationRequest.
   if (result == NavigationThrottle::CANCEL_AND_IGNORE ||
-      result == NavigationThrottle::CANCEL) {
+      result == NavigationThrottle::CANCEL || !response_should_be_rendered_) {
     // TODO(clamy): distinguish between CANCEL and CANCEL_AND_IGNORE.

[yzshen1, 2016/12/06 17:57:37]

Do we need to call frame_tree_node_->navigator()->DiscardPendingEntryIfNeeded()
for 204/etc.?

If I recall correctly, if we don't do that the url of  204/etc. will remain in
the address bar.

[clamy, 2016/12/16 17:07:14]

Done.

     frame_tree_node_->ResetNavigationRequest(false);
     return;
   }
 
   // Have the processing of the response resume in the network stack.
   loader_->ProceedWithResponse();
 
   CommitNavigation();
 
   // DO NOT ADD CODE after this. The previous call to CommitNavigation caused
@@ skipping 15 matching lines @@
   TransferNavigationHandleOwnership(render_frame_host);
 
   render_frame_host->CommitNavigation(response_.get(), std::move(body_),
                                       common_params_, request_params_,
                                       is_view_source_);
 
   frame_tree_node_->ResetNavigationRequest(true);
 }
 
 }  // namespace content