PlzNavigate: Remove the RequestNavigation IPC

content/browser/frame_host/navigator_impl.cc

 // Copyright 2013 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/navigator_impl.h"
 
 #include "base/command_line.h"
 #include "base/metrics/histogram.h"
 #include "base/time/time.h"
 #include "content/browser/frame_host/frame_tree.h"
 #include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/frame_host/navigation_controller_impl.h"
 #include "content/browser/frame_host/navigation_entry_impl.h"
 #include "content/browser/frame_host/navigation_request.h"
 #include "content/browser/frame_host/navigation_request_info.h"
 #include "content/browser/frame_host/navigator_delegate.h"
 #include "content/browser/frame_host/render_frame_host_impl.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/site_instance_impl.h"
 #include "content/browser/webui/web_ui_controller_factory_registry.h"
 #include "content/browser/webui/web_ui_impl.h"
+#include "content/common/frame_messages.h"
 #include "content/common/navigation_params.h"
 #include "content/common/view_messages.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/global_request_id.h"
 #include "content/public/browser/invalidate_type.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_details.h"
 #include "content/public/browser/page_navigator.h"
 #include "content/public/browser/render_view_host.h"
 #include "content/public/browser/stream_handle.h"
 #include "content/public/browser/user_metrics.h"
 #include "content/public/common/bindings_policy.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/resource_response.h"
 #include "content/public/common/url_constants.h"
 #include "content/public/common/url_utils.h"
-#include "net/base/load_flags.h"
-#include "net/http/http_request_headers.h"
 
 namespace content {
 
 namespace {
 
 FrameMsg_Navigate_Type::Value GetNavigationType(
     BrowserContext* browser_context, const NavigationEntryImpl& entry,
     NavigationController::ReloadType reload_type) {
   switch (reload_type) {
     case NavigationControllerImpl::RELOAD:
@@ skipping 11 matching lines @@
   if (entry.restore_type() ==
       NavigationEntryImpl::RESTORE_LAST_SESSION_EXITED_CLEANLY) {
     if (entry.GetHasPostData())
       return FrameMsg_Navigate_Type::RESTORE_WITH_POST;
     return FrameMsg_Navigate_Type::RESTORE;
   }
 
   return FrameMsg_Navigate_Type::NORMAL;
 }
 
-// PlzNavigate
-// Returns the net load flags to use based on the navigation type.
-// TODO(clamy): unify the code with what is happening on the renderer side.
-int LoadFlagFromNavigationType(FrameMsg_Navigate_Type::Value navigation_type) {
-  int load_flags = net::LOAD_NORMAL;
-  switch (navigation_type) {
-    case FrameMsg_Navigate_Type::RELOAD:
-    case FrameMsg_Navigate_Type::RELOAD_ORIGINAL_REQUEST_URL:
-      load_flags |= net::LOAD_VALIDATE_CACHE;
-      break;
-    case FrameMsg_Navigate_Type::RELOAD_IGNORING_CACHE:
-      load_flags |= net::LOAD_BYPASS_CACHE;
-      break;
-    case FrameMsg_Navigate_Type::RESTORE:
-      load_flags |= net::LOAD_PREFERRING_CACHE;
-      break;
-    case FrameMsg_Navigate_Type::RESTORE_WITH_POST:
-      load_flags |= net::LOAD_ONLY_FROM_CACHE;
-      break;
-    case FrameMsg_Navigate_Type::NORMAL:
-    default:
-      break;
-  }
-  return load_flags;
-}
-
-// PlzNavigate
-// Generates a default FrameHostMsg_BeginNavigation_Params to be used when there
-// is no live renderer.
-FrameHostMsg_BeginNavigation_Params MakeDefaultBeginNavigation(
-    const RequestNavigationParams& request_params,
-    FrameMsg_Navigate_Type::Value navigation_type) {
-  FrameHostMsg_BeginNavigation_Params begin_navigation_params;
-  begin_navigation_params.method = request_params.is_post ? "POST" : "GET";
-  begin_navigation_params.load_flags =
-      LoadFlagFromNavigationType(navigation_type);
-
-  // Copy existing headers and add necessary headers that may not be present
-  // in the RequestNavigationParams.
-  net::HttpRequestHeaders headers;
-  headers.AddHeadersFromString(request_params.extra_headers);
-  headers.SetHeaderIfMissing(net::HttpRequestHeaders::kUserAgent,
-                             GetContentClient()->GetUserAgent());
-  headers.SetHeaderIfMissing("Accept", "*/*");
-  begin_navigation_params.headers = headers.ToString();
-
-  // Fill POST data from the browser in the request body.
-  if (request_params.is_post) {
-    begin_navigation_params.request_body = new ResourceRequestBody();
-    begin_navigation_params.request_body->AppendBytes(
-        reinterpret_cast<const char *>(
-            &request_params.browser_initiated_post_data.front()),
-        request_params.browser_initiated_post_data.size());
-  }
-
-  begin_navigation_params.has_user_gesture = false;
-  return begin_navigation_params;
-}
-
 RenderFrameHostManager* GetRenderManager(RenderFrameHostImpl* rfh) {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kSitePerProcess))
     return rfh->frame_tree_node()->render_manager();
 
   return rfh->frame_tree_node()->frame_tree()->root()->render_manager();
 }
 
 void MakeNavigateParams(const NavigationEntryImpl& entry,
                         NavigationControllerImpl* controller,
@@ skipping 597 matching lines @@
 
     // Navigations in Web UI pages count as browser-initiated navigations.
     params.is_renderer_initiated = false;
   }
 
   if (delegate_)
     delegate_->RequestOpenURL(render_frame_host, params);
 }
 
 // PlzNavigate
-void NavigatorImpl::OnBeginNavigation(
-    FrameTreeNode* frame_tree_node,
-    const FrameHostMsg_BeginNavigation_Params& params,
-    const CommonNavigationParams& common_params) {
+void NavigatorImpl::OnBeforeUnloadACK(FrameTreeNode* frame_tree_node,
+                                      bool proceed) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   DCHECK(frame_tree_node);
 
   NavigationRequest* navigation_request =
       navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
 
-  if (!navigation_request) {
-    // This is a renderer initiated navigation, so generate a new
-    // NavigationRequest and store it in the map.
-    // TODO(clamy): Check if some PageState should be provided here.
-    // TODO(clamy): See how we should handle override of the user agent when the
-    // navigation may start in a renderer and commit in another one.
-    // TODO(clamy): See if the navigation start time should be measured in the
-    // renderer and sent to the browser instead of being measured here.
-    scoped_ptr<NavigationRequest> scoped_request(new NavigationRequest(
-        frame_tree_node, common_params,
-        CommitNavigationParams(PageState(), false, base::TimeTicks::Now()),
-        nullptr));
-    navigation_request = scoped_request.get();
-    navigation_request_map_.set(
-        frame_tree_node->frame_tree_node_id(), scoped_request.Pass());
-  }
-  DCHECK(navigation_request);
+  // The NavigationRequest may have been canceled while the renderer was
+  // executing the BeforeUnload event.
+  if (!navigation_request)
+    return;
 
-  // Update the referrer with the one received from the renderer.
-  navigation_request->common_params().referrer = common_params.referrer;
+  DCHECK(navigation_request->state() ==
+      NavigationRequest::WAITING_FOR_RENDERER_RESPONSE);

[davidben, 2015/02/03 02:23:21]

Nit: DCHECK_EQ

[carlosk, 2015/02/03 16:06:02]

Isn't it possible that the original request was cancelled AND another request
was created in the meantime? 

[clamy, 2015/02/03 16:17:09]

Yes but in that case we do not send another BeforeUnload IPC. So we should not
be receiving an ACK when we are in another state.

[clamy, 2015/02/03 16:17:09]

Done.

 
-  scoped_ptr<NavigationRequestInfo> info(new NavigationRequestInfo(params));
-
-  info->first_party_for_cookies =
-      frame_tree_node->IsMainFrame()
-          ? navigation_request->common_params().url
-          : frame_tree_node->frame_tree()->root()->current_url();
-  info->is_main_frame = frame_tree_node->IsMainFrame();
-  info->parent_is_main_frame = !frame_tree_node->parent() ?
-      false : frame_tree_node->parent()->IsMainFrame();
-
-  // First start the request on the IO thread.
-  navigation_request->BeginNavigation(info.Pass(), params.request_body);
-
-  // Then notify the RenderFrameHostManager so it can speculatively create a
-  // RenderFrameHost (and potentially a new renderer process) in parallel.
-  frame_tree_node->render_manager()->BeginNavigation(*navigation_request);
+  if (proceed)
+    BeginNavigation(frame_tree_node);
+  else
+    CancelNavigation(frame_tree_node);
 }
 
 // PlzNavigate
+void NavigatorImpl::OnBeginNavigation(
+    FrameTreeNode* frame_tree_node,
+    const CommonNavigationParams& common_params,
+    const BeginNavigationParams& begin_params,
+    scoped_refptr<ResourceRequestBody> body) {
+  CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
+      switches::kEnableBrowserSideNavigation));
+  DCHECK(frame_tree_node);
+
+  // This is a renderer initiated navigation, so generate a new

[nasko, 2015/02/02 21:17:43]

nit: renderer-initiated

[clamy, 2015/02/03 16:17:09]

Done.

+  // NavigationRequest and store it in the map.
+  // TODO(clamy): Renderer-initiated navigations should not always cancel the
+  // current one.
+  scoped_ptr<NavigationRequest> navigation_request =
+      NavigationRequest::CreateRendererInitiated(
+          frame_tree_node, common_params, begin_params, body);
+  navigation_request_map_.set(

[carlosk, 2015/02/03 16:06:02]

Don't we need to verify first if another request is already ongoing?

[clamy, 2015/02/03 16:17:09]

That's the TODO above this block of code. The patch is large enough that I did
not want to go in fully implementing the navigation cancelation and associated
tests.

+      frame_tree_node->frame_tree_node_id(), navigation_request.Pass());
+
+  BeginNavigation(frame_tree_node);
+}
+
+// PlzNavigate
 void NavigatorImpl::CommitNavigation(FrameTreeNode* frame_tree_node,
                                      ResourceResponse* response,
                                      scoped_ptr<StreamHandle> body) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
 
   // HTTP 204 (No Content) and HTTP 205 (Reset Content) responses should not
   // commit; they leave the frame showing the previous page.
   if (response->head.headers.get() &&
       (response->head.headers->response_code() == 204 ||
@@ skipping 88 matching lines @@
     FrameTreeNode* frame_tree_node,
     const NavigationEntryImpl& entry,
     NavigationController::ReloadType reload_type,
     base::TimeTicks navigation_start) {
   CHECK(base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableBrowserSideNavigation));
   DCHECK(frame_tree_node);
   int64 frame_tree_node_id = frame_tree_node->frame_tree_node_id();
   FrameMsg_Navigate_Type::Value navigation_type =
       GetNavigationType(controller_->GetBrowserContext(), entry, reload_type);
-  scoped_ptr<NavigationRequest> navigation_request = NavigationRequest::Create(
-      frame_tree_node, entry, navigation_type, navigation_start);
-  RequestNavigationParams request_params(entry.GetHasPostData(),
-                                         entry.extra_headers(),
-                                         entry.GetBrowserInitiatedPostData());
+  scoped_ptr<NavigationRequest> navigation_request =
+      NavigationRequest::CreateBrowserInitiated(
+          frame_tree_node, entry, navigation_type, navigation_start);
   // TODO(clamy): Check if navigations are blocked and if so store the
   // parameters.
 
   // If there is an ongoing request, replace it.
   navigation_request_map_.set(frame_tree_node_id, navigation_request.Pass());
 
-  if (frame_tree_node->current_frame_host()->IsRenderFrameLive()) {
-    NavigationRequest* request_to_send =
-        navigation_request_map_.get(frame_tree_node_id);
-    frame_tree_node->current_frame_host()->Send(new FrameMsg_RequestNavigation(
-        frame_tree_node->current_frame_host()->GetRoutingID(),
-        request_to_send->common_params(), request_params));
-    request_to_send->SetWaitingForRendererResponse();
-    return true;
-  }
+  // Have the current renderer execute its BeforeUnload event if needed. If it

[nasko, 2015/02/02 21:17:43]

nit: beforeUnload

[clamy, 2015/02/03 16:17:09]

Done.

+  // is not needed (eg. the renderer is not live), BeginNavigation should get
+  // called.
+  NavigationRequest* request_to_send =
+      navigation_request_map_.get(frame_tree_node_id);
+  request_to_send->SetWaitingForRendererResponse();
+  frame_tree_node->current_frame_host()->DispatchBeforeUnload(true);

[nasko, 2015/02/02 21:17:43]

Do we know this is a cross-site transition? It could be just regular
browser-initiated navigation for the same site.

[davidben, 2015/02/03 02:23:21]

Even if the navigation is same-site, I think we'll want beforeunload to be
driven by the browser.

I think the reason it's special in today's flow is that today we ask a renderer
to navigate on the browser's behalf. And if it's the same renderer, it'll run
beforeunload for us because that's what Blink does. But, in PlzNavigate, by the
time the renderer finds out about the navigation, it'll be too late for it to
beforeunload anyway.

[clamy, 2015/02/03 16:17:09]

My understanding is that this flag is used by RenderFrameHostImpl to distinguish
whether we are waiting for the BeforeUnloadACK because we are navigating
(cross-site only in the current implementation) or because we are closing the
page. Given that we are navigating here, having for for_cross_site_transition
true made more sense. Maybe it could be renamed to for_navigation, which would
make it clearer.

[nasko, 2015/02/04 00:14:19]

Indeed renaming it makes sense.

[clamy, 2015/02/05 15:33:13]

Included the renaming in this CL.

+  return true;

[carlosk, 2015/02/03 16:06:02]

This return doesn't seem to be meaningful anymore.

[clamy, 2015/02/03 16:17:09]

We either always return true here, or in NavigateToEntry (since NavigateToEntry
needs a return value). Not sure which one is better.

[nasko, 2015/02/04 00:14:19]

If this method cannot fail, it shouldn't be returning a boolean. I'm fine with
doing it in a separate CL to keep this one from exploding.

[clamy, 2015/02/05 15:33:13]

It's a simple enough change, so including it in this CL.

+}
 
-  // The navigation request is sent directly to the IO thread.
-  OnBeginNavigation(
-      frame_tree_node,
-      MakeDefaultBeginNavigation(request_params, navigation_type),
-      navigation_request_map_.get(frame_tree_node_id)->common_params());
-  return true;
+void NavigatorImpl::BeginNavigation(FrameTreeNode* frame_tree_node) {
+  NavigationRequest* navigation_request =
+      navigation_request_map_.get(frame_tree_node->frame_tree_node_id());
+
+  // A browser-initiated navigation could have been cancelled while it was
+  // waiting for the BeforeUnload event to execute.
+  if (!navigation_request)
+    return;
+
+  // First start the request on the IO thread.
+  navigation_request->BeginNavigation();
+
+  // Then notify the RenderFrameHostManager so it can speculatively create a
+  // RenderFrameHost (and potentially a new renderer process) in parallel.
+  frame_tree_node->render_manager()->BeginNavigation(*navigation_request);
 }
 
 void NavigatorImpl::RecordNavigationMetrics(
     const LoadCommittedDetails& details,
     const FrameHostMsg_DidCommitProvisionalLoad_Params& params,
     SiteInstance* site_instance) {
   DCHECK(site_instance->HasProcess());
 
   if (!details.is_in_page)
     RecordAction(base::UserMetricsAction("FrameLoad"));
@@ skipping 36 matching lines @@
         "Navigation.TimeToCommit_ExistingRenderer_BeforeUnloadDiscounted",
         time_to_commit);
     UMA_HISTOGRAM_TIMES(
         "Navigation.TimeToURLJobStart_ExistingRenderer_BeforeUnloadDiscounted",
         time_to_network);
   }
   navigation_data_.reset();
 }
 
 }  // namespace content