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..0a9ae90ead25d4b4d79646540df8826797e2b8a6 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);

[Charlie Reis, 2016/05/31 21:08:36]

Sanity check: Will we need to change this when we later add support for
back/forward POST resubmissions on transfers?  Or are those not a problem here
because the POST data is hidden away in the PageState?  (Just wondering if we
should leave a TODO.)

[Łukasz Anforowicz, 2016/05/31 22:40:55]

I don't really know :-(  AFAIK, we don't need to send the POST body to the
renderer to history and reload navigations, because it will be taken from
renderer's in-memory state (i.e. see in RenderFrameImpl::NavigateInternal how
|request| is initialized in |is_reload| and/or |is_history_navigation| cases). 
So - I think PageState should cover this, and we will continue passing
|nullptr|, but I don't really know...

[clamy, 2016/06/01 13:22:37]

In the current architecture, the POST body should not be needed for simple
reloads and history since we create the WebURLRequest for them in
RenderFrameImpl using the PageState that is provided by the browser (and I can
confirm we initialize the WebHTTPBody based on the PageState).

A possible issue would be transfers in the middle of a history/reload (if they
are possible). In that case, I imagine the transfer case would take care of
that, but I'm not sure.

[Charlie Reis, 2016/06/01 23:46:32]

Yeah, this answers my question.  The PageState should be sufficient in most
cases.
Yeah, transfers during history navigations are possible, and it's kind of a
scary case that we should add some tests for (separately).  I thought it might
explain the NavigateInternal crashes in https://crbug.com/568703#c9, but it
turned out to not be an issue there.

I think you're right that the existing transfer logic for POSTs should kick in
there, so I'm guessing this line is fine.

 }
 
 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,12 @@ 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) {
+  // |method != "POST"| should imply absence of |post_body|.
+  DCHECK(method == "POST" || !post_body);

[Charlie Reis, 2016/05/31 21:08:36]

Should we be concerned if this fails in practice?

[Łukasz Anforowicz, 2016/05/31 22:40:55]

Yes, we should be concerned, but I don't know how much :-)

If this condition fails, it means that we are unnecessarily sending a POST body
to a renderer (and potentially to a renderer that wouldn't have seen this POST
body otherwise).

[Charlie Reis, 2016/06/01 23:46:32]

Doesn't sound like a crashable offense, but maybe we should put in a
NOTREACHED() block that resets post_body instead of just DCHECKing?  Or is that
tricky?

[Łukasz Anforowicz, 2016/06/02 22:07:04]

Done.  To reset the parameter we need to make it non-const, but this seems fine
/ is the only trickyness here.

I've also did a similar change in the other places where this CL adds DCHECKs
about post body:
- NavigatorImpl::RequestTransferURL
- CommonNavigationParams constructor

+
   // This call only makes sense for subframes if OOPIFs are possible.
   DCHECK(!render_frame_host->GetParent() ||
          SiteIsolationPolicy::AreCrossProcessFramesPossible());
@@ -788,12 +794,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 +827,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