Forwarding POST body into renderer after a cross-site transfer.

content/browser/frame_host/navigator_impl.cc

Index: content/browser/frame_host/navigator_impl.cc
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index 961b37d27b9c034e13fa69c3078acc900c62efbf..97067f9cae66bdd261c92eb6275b6e5aa9084799 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -269,7 +269,8 @@ bool NavigatorImpl::NavigateToEntry(
     const NavigationEntryImpl& entry,
     NavigationController::ReloadType reload_type,
     bool is_same_document_history_load,
-    bool is_pending_entry) {
+    bool is_pending_entry,
+    const scoped_refptr<ResourceRequestBody>& post_body) {
   TRACE_EVENT0("browser,navigation", "NavigatorImpl::NavigateToEntry");
 
   GURL dest_url = frame_entry.url();
@@ -387,9 +388,9 @@ bool NavigatorImpl::NavigateToEntry(
       FrameMsg_Navigate_Type::Value navigation_type = GetNavigationType(
           controller_->GetBrowserContext(), entry, reload_type);
       dest_render_frame_host->Navigate(
-          entry.ConstructCommonNavigationParams(frame_entry, nullptr, dest_url,
-                                                dest_referrer, navigation_type,
-                                                lofi_state, navigation_start),
+          entry.ConstructCommonNavigationParams(
+              frame_entry, post_body, dest_url, dest_referrer, navigation_type,
+              lofi_state, navigation_start),
           entry.ConstructStartNavigationParams(),
           entry.ConstructRequestNavigationParams(
               frame_entry, is_same_document_history_load,
@@ -436,7 +437,7 @@ bool NavigatorImpl::NavigateToPendingEntry(
     bool is_same_document_history_load) {
   return NavigateToEntry(frame_tree_node, frame_entry,
                          *controller_->GetPendingEntry(), reload_type,
-                         is_same_document_history_load, true);
+                         is_same_document_history_load, true, nullptr);
 }
 
 bool NavigatorImpl::NavigateNewChildFrame(
@@ -457,7 +458,7 @@ bool NavigatorImpl::NavigateNewChildFrame(
 
   return NavigateToEntry(render_frame_host->frame_tree_node(), *frame_entry,
                          *entry, NavigationControllerImpl::NO_RELOAD, false,
-                         false);
+                         false, nullptr);
 }
 
 void NavigatorImpl::DidNavigate(
@@ -724,7 +725,9 @@ void NavigatorImpl::RequestTransferURL(
     const Referrer& referrer,
     ui::PageTransition page_transition,
     const GlobalRequestID& transferred_global_request_id,
-    bool should_replace_current_entry) {
+    bool should_replace_current_entry,
+    const std::string& method,
+    const scoped_refptr<ResourceRequestBody>& post_body) {
   // This call only makes sense for subframes if OOPIFs are possible.
   DCHECK(!render_frame_host->GetParent() ||
          SiteIsolationPolicy::AreCrossProcessFramesPossible());

[clamy, 2016/05/26 15:35:23]

Add a DCHECK that post_body is null for non POST navigations.

[Łukasz Anforowicz, 2016/05/26 17:14:02]

Done.

I've also added the DCHECK in CommonNavigationParams.

My only worry is that sometimes POST might have an empty body.  In such cases we
should be using non-null, empty ResourceRequestBody, but I am not 100% sure if
with the current codebase we won't ever treat null and empty interchangably. 
Still - let's try adding the DCHECK - this should put us in a better place in
the long term.

[clamy, 2016/05/30 16:40:43]

Yes this is why I suggested a check that the body is null in non-POST
navigations, and not a check that the body is not null in POST navigations. I'm
not sure that we will always have one in POSTs, and ti complexifies testing
code.

If we want to keep, it should also be written without if else clauses (you
shoudln't be using if/else when writing only dchecks as it's not clear that the
compiler will properly compile out the if/else when compiling for release).

[Łukasz Anforowicz, 2016/05/31 16:25:20]

Done - kept only testing for absence of body if method != POST.
Thanks for raising this.  I misremembered the DCHECK_IMPLIES thread on
chromium-dev [1] - I thought that it was said that we can safely use "if"
statement, but it is more subtle than that (i.e. the compiler might not always
be able to tell that there were no side effects).  Being lazy (i.e. not wanting
to learn how to get disassembly and learn how to find the relevant pieces of
disassembly) I just got rid of the "if" statement.

Done - |if (a) DCHECK(b)| has been rewritten as |DCHECK(!a || b)|.

[1]
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/1Jpudx4lMMA/dj_1O...

@@ -788,12 +791,10 @@ void NavigatorImpl::RequestTransferURL(
               is_renderer_initiated, std::string(),
               controller_->GetBrowserContext()));
     }
-    // TODO(creis): Handle POST submissions.  See https://crbug.com/582211 and
-    // https://crbug.com/101395.
     entry->AddOrUpdateFrameEntry(
         node, -1, -1, nullptr,
         static_cast<SiteInstanceImpl*>(source_site_instance), dest_url,
-        referrer_to_use, PageState(), "GET", -1);
+        referrer_to_use, PageState(), method, -1);
   } else {
     // Main frame case.
     entry = NavigationEntryImpl::FromNavigationEntry(
@@ -823,14 +824,13 @@ void NavigatorImpl::RequestTransferURL(
   // further in https://crbug.com/536906.
   scoped_refptr<FrameNavigationEntry> frame_entry(entry->GetFrameEntry(node));
   if (!frame_entry) {
-    // TODO(creis): Handle POST submissions here, as above.
     frame_entry = new FrameNavigationEntry(
         node->unique_name(), -1, -1, nullptr,
         static_cast<SiteInstanceImpl*>(source_site_instance), dest_url,
-        referrer_to_use, "GET", -1);
+        referrer_to_use, method, -1);
   }
   NavigateToEntry(node, *frame_entry, *entry.get(),
-                  NavigationController::NO_RELOAD, false, false);
+                  NavigationController::NO_RELOAD, false, false, post_body);
 }
 
 // PlzNavigate