PlzNavigate: store POST data in the FrameNavigationEntry

content/browser/frame_host/render_frame_host_manager.cc

Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index 58c17d54c34798e0e070878e4e572a594f41a3a3..4d85ec99965d395c286bee188e1e16860d799a68 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -732,18 +732,16 @@ void RenderFrameHostManager::ClearWebUIInstances() {
 void RenderFrameHostManager::DidCreateNavigationRequest(
     NavigationRequest* request) {
   CHECK(IsBrowserSideNavigationEnabled());
-  RenderFrameHostImpl* dest_rfh = GetFrameHostForNavigation(*request);
+  RenderFrameHostImpl* dest_rfh = GetFrameHostForNavigation(request, false);
   DCHECK(dest_rfh);
-  request->set_associated_site_instance_type(
-      dest_rfh == render_frame_host_.get()
-          ? NavigationRequest::AssociatedSiteInstanceType::CURRENT
-          : NavigationRequest::AssociatedSiteInstanceType::SPECULATIVE);
 }
 
 // PlzNavigate
 RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
-    const NavigationRequest& request) {
+    NavigationRequest* request,
+    bool is_commit) {
   CHECK(IsBrowserSideNavigationEnabled());
+  DCHECK(request);
 
   SiteInstance* current_site_instance = render_frame_host_->GetSiteInstance();
 
@@ -753,15 +751,19 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
           : nullptr;
 
   scoped_refptr<SiteInstance> dest_site_instance = GetSiteInstanceForNavigation(
-      request.common_params().url, request.source_site_instance(),
-      request.dest_site_instance(), candidate_site_instance,
-      request.common_params().transition,
-      request.restore_type() != NavigationEntryImpl::RESTORE_NONE,
-      request.is_view_source());
+      request->common_params().url, request->source_site_instance(),
+      request->dest_site_instance(), candidate_site_instance,
+      request->common_params().transition,
+      request->restore_type() != NavigationEntryImpl::RESTORE_NONE,
+      request->is_view_source());
 
   // The appropriate RenderFrameHost to commit the navigation.
   RenderFrameHostImpl* navigation_rfh = nullptr;
 
+  // Whether the RenderFrameHost changed between the start and the commit of
+  // the navigation.
+  bool switched_rfh = false;
+
   bool notify_webui_of_rv_creation = false;
 
   // Reuse the current RenderFrameHost if its SiteInstance matches the
@@ -778,18 +780,18 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
     // go cross-process. Check it first.
     bool can_renderer_initiate_transfer =
         render_frame_host_->IsRenderFrameLive() &&
-        ShouldMakeNetworkRequestForURL(request.common_params().url) &&
+        ShouldMakeNetworkRequestForURL(request->common_params().url) &&
         IsRendererTransferNeededForNavigation(render_frame_host_.get(),
-                                              request.common_params().url);
+                                              request->common_params().url);
 
     no_renderer_swap |=
-        !request.browser_initiated() && !can_renderer_initiate_transfer;
+        !request->browser_initiated() && !can_renderer_initiate_transfer;
   } else {
     // Subframe navigations will use the current renderer, unless specifically
     // allowed to swap processes.
-    no_renderer_swap |= !CanSubframeSwapProcess(request.common_params().url,
-                                                request.source_site_instance(),
-                                                request.dest_site_instance());
+    no_renderer_swap |= !CanSubframeSwapProcess(request->common_params().url,
+                                                request->source_site_instance(),
+                                                request->dest_site_instance());
   }
 
   if (no_renderer_swap) {
@@ -799,11 +801,15 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
     // not be recreated if the URL didn't change. So instead of calling
     // CleanUpNavigation just discard the speculative RenderFrameHost if one
     // exists.
-    if (speculative_render_frame_host_)
+    if (speculative_render_frame_host_) {
       DiscardUnusedFrame(UnsetSpeculativeRenderFrameHost());
 
-    UpdatePendingWebUIOnCurrentFrameHost(request.common_params().url,
-                                         request.bindings());
+      if (is_commit)
+        switched_rfh = true;
+    }
+
+    UpdatePendingWebUIOnCurrentFrameHost(request->common_params().url,
+                                         request->bindings());
 
     navigation_rfh = render_frame_host_.get();
 
@@ -825,11 +831,14 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
       bool success = CreateSpeculativeRenderFrameHost(current_site_instance,
                                                       dest_site_instance.get());
       DCHECK(success);
+
+      if (is_commit)
+        switched_rfh = true;
     }
     DCHECK(speculative_render_frame_host_);
 
     bool changed_web_ui = speculative_render_frame_host_->UpdatePendingWebUI(
-        request.common_params().url, request.bindings());
+        request->common_params().url, request->bindings());
     speculative_render_frame_host_->CommitPendingWebUI();
     DCHECK_EQ(GetNavigatingWebUI(), speculative_render_frame_host_->web_ui());
     notify_webui_of_rv_creation =
@@ -888,6 +897,21 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
   if (notify_webui_of_rv_creation && GetNavigatingWebUI())
     GetNavigatingWebUI()->RenderViewCreated(navigation_rfh->render_view_host());
 
+  // Update the type of SiteInstance in the NavigationRequest.
+  request->set_associated_site_instance_type(
+      navigation_rfh == render_frame_host_.get()
+          ? NavigationRequest::AssociatedSiteInstanceType::CURRENT
+          : NavigationRequest::AssociatedSiteInstanceType::SPECULATIVE);
+
+  // If the SiteInstance changed between the start and the end of the
+  // navigation, do not send POST data. This avoids POST data from one
+  // SiteInstance to be sent to a renderer of another SiteInstance when the
+  // navigation encountered a cross-site redirect.

[Charlie Reis, 2016/05/11 00:00:23]

See my other comment on ResetPostData.  Based on what Lukasz found, we might not
need to do this at all.  (And hopefully not, because the is_commit parameter
feels awkward.)

[clamy, 2016/05/11 08:54:41]

Done. We indeed don't need to do this.

+  if (switched_rfh) {
+    DCHECK(is_commit);
+    request->ResetPostData();
+  }
+
   return navigation_rfh;
 }