PlzNavigate: store POST data in the FrameNavigationEntry

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/devtools/render_frame_devtools_agent_host.h"
 #include "content/browser/frame_host/frame_tree.h"
@@ skipping 62 matching lines @@
     bool is_same_document_history_load,
     const base::TimeTicks& navigation_start,
     NavigationControllerImpl* controller) {
   // Copy existing headers and add necessary headers that may not be present
   // in the RequestNavigationParams.
   net::HttpRequestHeaders headers;
   headers.AddHeadersFromString(entry.extra_headers());
   headers.SetHeaderIfMissing(net::HttpRequestHeaders::kUserAgent,
                              GetContentClient()->GetUserAgent());
 
-  // Fill POST data from the browser in the request body.
+  // Fill POST data in the request body.
   scoped_refptr<ResourceRequestBody> request_body;
-  if (entry.GetHasPostData()) {
-    request_body = new ResourceRequestBody();
-    request_body->AppendBytes(
-        reinterpret_cast<const char *>(
-            entry.GetBrowserInitiatedPostData()->front()),
-        entry.GetBrowserInitiatedPostData()->size());
+  if (frame_entry.method() == "POST") {
+    request_body = frame_entry.GetPostData();
+    if (!request_body && entry.GetBrowserInitiatedPostData()) {
+      request_body = new ResourceRequestBody();
+      request_body->AppendBytes(
+          reinterpret_cast<const char*>(
+              entry.GetBrowserInitiatedPostData()->front()),
+          entry.GetBrowserInitiatedPostData()->size());
+    }
   }
 
   std::unique_ptr<NavigationRequest> navigation_request(new NavigationRequest(
       frame_tree_node, entry.ConstructCommonNavigationParams(
-                           dest_url, dest_referrer, navigation_type, lofi_state,
-                           navigation_start),
+                           frame_entry, dest_url, dest_referrer,
+                           navigation_type, lofi_state, navigation_start),
       BeginNavigationParams(headers.ToString(),
                             LoadFlagFromNavigationType(navigation_type),
                             false,  // has_user_gestures
                             false,  // skip_service_worker
                             REQUEST_CONTEXT_TYPE_LOCATION),
       entry.ConstructRequestNavigationParams(
           frame_entry, is_same_document_history_load,
           frame_tree_node->has_committed_real_load(),
           controller->GetPendingEntryIndex() == -1,
           controller->GetIndexOfEntry(&entry),
@@ skipping 82 matching lines @@
   const GURL& first_party_for_cookies =
       frame_tree_node->IsMainFrame()
           ? common_params.url
           : frame_tree_node->frame_tree()->root()->current_url();
   bool parent_is_main_frame = !frame_tree_node->parent() ?
       false : frame_tree_node->parent()->IsMainFrame();
   info_.reset(new NavigationRequestInfo(
       common_params, begin_params, first_party_for_cookies,
       frame_tree_node->current_origin(), frame_tree_node->IsMainFrame(),
       parent_is_main_frame, frame_tree_node->frame_tree_node_id(), body));
+
+  if (body)
+    post_data_ = body->MakeCopy();

[Łukasz Anforowicz, 2016/05/12 02:53:59]

Can you help me understand why we need to make a deep copy here (rather than
just take another ref-count)?  I thought that ResourceRequestBody would never be
changed after initial construction - is this not the case / is there a need to
mutate ResourceRequestBody in some cases?

[clamy, 2016/05/12 08:53:13]

I wasn't sure about this so I made a copy to stay safe. If someone from the net
stack team explained that it would not be changed, then it's probably safe not
to make a copy.

[Łukasz Anforowicz, 2016/05/12 19:44:12]

I think it should be safe not to make a copy - I've tried to look at the callers
of ResourceRequestBody's non-const methods (AppendBytes, AppendFileRange,
AppendBlob, AppendFileSystemFileRange, elements_mutable, swap_elements,
set_identifier) and they appear to only come from code that constructs a fresh
ResourceRequestBody (or from test code).

Interestingly Cr Code Search was not able to find users of elements_mutable. 
Seems like a job for Scythe :-)

So - I would vote from removing the deep copy, but you are right, that it might
be a good idea to double-check this with a //net person.

[Charlie Reis, 2016/05/16 21:16:41]

+1 to removing the deep copy if we can.

[clamy, 2016/05/19 13:11:57]

Done.

 }
 
 NavigationRequest::~NavigationRequest() {
 }
 
 void NavigationRequest::BeginNavigation() {
   DCHECK(!loader_);
   DCHECK(state_ == NOT_STARTED || state_ == WAITING_FOR_RENDERER_RESPONSE);
   state_ = STARTED;
   RenderFrameDevToolsAgentHost::OnBeforeNavigation(navigation_handle_.get());
@@ skipping 40 matching lines @@
 }
 
 void NavigationRequest::TransferNavigationHandleOwnership(
     RenderFrameHostImpl* render_frame_host) {
   render_frame_host->SetNavigationHandle(std::move(navigation_handle_));
 }
 
 void NavigationRequest::OnRequestRedirected(
     const net::RedirectInfo& redirect_info,
     const scoped_refptr<ResourceResponse>& response) {
+  // If the navigation is no longer a POST, the POST data should be reset.
+  if (common_params_.method == "POST" && redirect_info.new_method != "POST")

[Łukasz Anforowicz, 2016/05/12 02:53:59]

Shouldn't the second part of the condition be sufficient?  (i.e. we don't really
need to consult |common_params_.method|, right?)

[clamy, 2016/05/12 08:53:13]

No but I thought it made a bit clearer the case in which we actually reset the
request. Can remove the second part if needed.

[Łukasz Anforowicz, 2016/05/12 19:44:12]

Acknowledged.  I don't strongly feel one way or another.

[Charlie Reis, 2016/05/16 21:16:41]

I think we should remove the first part (common_params_.method == "POST").  It's
not likely that post_data_ will exist when common_params_.method != "POST", but
if it did, it would be bad to preserve it after a redirect to a GET.

+    post_data_ = nullptr;
+
   common_params_.url = redirect_info.new_url;
   common_params_.method = redirect_info.new_method;
   common_params_.referrer.url = GURL(redirect_info.new_referrer);
 
   // TODO(clamy): Have CSP + security upgrade checks here.
   // TODO(clamy): Kill the renderer if FilterURL fails?
 
   // It's safe to use base::Unretained because this NavigationRequest owns the
   // NavigationHandle where the callback will be stored.
   // TODO(clamy): pass the real value for |is_external_protocol| if needed.
@@ skipping 145 matching lines @@
   RenderFrameHostImpl* render_frame_host =
       navigation_handle_->GetRenderFrameHost();
   DCHECK(render_frame_host ==
              frame_tree_node_->render_manager()->current_frame_host() ||
          render_frame_host ==
              frame_tree_node_->render_manager()->speculative_frame_host());
 
   TransferNavigationHandleOwnership(render_frame_host);
   render_frame_host->CommitNavigation(response_.get(), std::move(body_),
                                       common_params_, request_params_,
-                                      is_view_source_);
+                                      is_view_source_, post_data_);
 
   // When navigating to a Javascript url, the NavigationRequest is not stored
   // in the FrameTreeNode. Therefore do not reset it, as this could cancel an
   // existing pending navigation.
   if (!common_params_.url.SchemeIs(url::kJavaScriptScheme))
     frame_tree_node_->ResetNavigationRequest(true);
 }
 
 void NavigationRequest::InitializeServiceWorkerHandleIfNeeded() {
   // Only initialize the ServiceWorkerNavigationHandle if it can be created for
@@ skipping 19 matching lines @@
       browser_context, navigating_frame_host->GetSiteInstance());
   DCHECK(partition);
 
   ServiceWorkerContextWrapper* service_worker_context =
       static_cast<ServiceWorkerContextWrapper*>(
           partition->GetServiceWorkerContext());
   navigation_handle_->InitServiceWorkerHandle(service_worker_context);
 }
 
 }  // namespace content