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

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 <utility>
 
 #include "base/metrics/histogram.h"
 #include "base/time/time.h"
@@ skipping 251 matching lines @@
         error_description, was_ignored_by_handler);
   }
 }
 
 bool NavigatorImpl::NavigateToEntry(
     FrameTreeNode* frame_tree_node,
     const FrameNavigationEntry& frame_entry,
     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();
   Referrer dest_referrer = frame_entry.referrer();
   if (reload_type ==
           NavigationController::ReloadType::RELOAD_ORIGINAL_REQUEST_URL &&
       entry.GetOriginalRequestURL().is_valid() && !entry.GetHasPostData()) {
     // We may have been redirected when navigating to the current URL.
     // Use the URL the user originally intended to visit, if it's valid and if a
     // POST wasn't involved; the latter case avoids issues with sending data to
@@ skipping 97 matching lines @@
         is_transfer &&
         entry.transferred_global_request_id().child_id ==
             dest_render_frame_host->GetProcess()->GetID();
     if (!is_transfer_to_same) {
       navigation_data_.reset(new NavigationMetricsData(
           navigation_start, dest_url, entry.restore_type()));
       // Create the navigation parameters.
       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,
               frame_tree_node->has_committed_real_load(),
               controller_->GetPendingEntryIndex() == -1,
               controller_->GetIndexOfEntry(&entry),
               controller_->GetLastCommittedEntryIndex(),
               controller_->GetEntryCount()));
     } else {
       // No need to navigate again.  Just resume the deferred request.
@@ skipping 26 matching lines @@
   return true;
 }
 
 bool NavigatorImpl::NavigateToPendingEntry(
     FrameTreeNode* frame_tree_node,
     const FrameNavigationEntry& frame_entry,
     NavigationController::ReloadType reload_type,
     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(
     RenderFrameHostImpl* render_frame_host,
     const std::string& unique_name) {
   NavigationEntryImpl* entry =
       controller_->GetEntryWithUniqueID(render_frame_host->nav_entry_id());
   if (!entry)
     return false;
 
   // TODO(creis): Remove unique_name from the IPC, now that we can rely on the
   // replication state.
   DCHECK_EQ(render_frame_host->frame_tree_node()->unique_name(), unique_name);
   FrameNavigationEntry* frame_entry =
       entry->GetFrameEntry(render_frame_host->frame_tree_node());
   if (!frame_entry)
     return false;
 
   return NavigateToEntry(render_frame_host->frame_tree_node(), *frame_entry,
                          *entry, NavigationControllerImpl::NO_RELOAD, false,
-                         false);
+                         false, nullptr);
 }
 
 void NavigatorImpl::DidNavigate(
     RenderFrameHostImpl* render_frame_host,
     const FrameHostMsg_DidCommitProvisionalLoad_Params& params) {
   FrameTree* frame_tree = render_frame_host->frame_tree_node()->frame_tree();
   bool oopifs_possible = SiteIsolationPolicy::AreCrossProcessFramesPossible();
 
   bool has_embedded_credentials =
       params.url.has_username() || params.url.has_password();
@@ skipping 246 matching lines @@
 }
 
 void NavigatorImpl::RequestTransferURL(
     RenderFrameHostImpl* render_frame_host,
     const GURL& url,
     SiteInstance* source_site_instance,
     const std::vector<GURL>& redirect_chain,
     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());
 
   // Allow the delegate to cancel the transfer.
   if (!delegate_->ShouldTransferNavigation())
     return;
 
   GURL dest_url(url);
   Referrer referrer_to_use(referrer);
@@ skipping 43 matching lines @@
     } else {
       // If there's no last committed entry, create an entry for about:blank
       // with a subframe entry for our destination.
       // TODO(creis): Ensure this case can't exist in https://crbug.com/524208.
       entry = NavigationEntryImpl::FromNavigationEntry(
           controller_->CreateNavigationEntry(
               GURL(url::kAboutBlankURL), referrer_to_use, page_transition,
               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(
         controller_->CreateNavigationEntry(
             dest_url, referrer_to_use, page_transition, is_renderer_initiated,
             std::string(), controller_->GetBrowserContext()));
     entry->root_node()->frame_entry->set_source_site_instance(
         static_cast<SiteInstanceImpl*>(source_site_instance));
   }
 
   entry->SetRedirectChain(redirect_chain);
   // Don't allow an entry replacement if there is no entry to replace.
   // http://crbug.com/457149
   if (should_replace_current_entry && controller_->GetEntryCount() > 0)
     entry->set_should_replace_entry(true);
   if (controller_->GetLastCommittedEntry() &&
       controller_->GetLastCommittedEntry()->GetIsOverridingUserAgent()) {
     entry->SetIsOverridingUserAgent(true);
   }
   entry->set_transferred_global_request_id(transferred_global_request_id);
   // TODO(creis): Set user gesture and intent received timestamp on Android.
 
   // We may not have successfully added the FrameNavigationEntry to |entry|
   // above (per https://crbug.com/608402), in which case we create it from
   // scratch.  This works because we do not depend on |frame_entry| being inside
   // |entry| during NavigateToEntry.  This will go away when we shortcut this
   // 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
 void NavigatorImpl::OnBeforeUnloadACK(FrameTreeNode* frame_tree_node,
                                       bool proceed) {
   CHECK(IsBrowserSideNavigationEnabled());
   DCHECK(frame_tree_node);
 
   NavigationRequest* navigation_request = frame_tree_node->navigation_request();
 
@@ skipping 319 matching lines @@
   if (pending_entry != controller_->GetVisibleEntry() ||
       !should_preserve_entry) {
     controller_->DiscardPendingEntry(true);
 
     // Also force the UI to refresh.
     controller_->delegate()->NotifyNavigationStateChanged(INVALIDATE_TYPE_URL);
   }
 }
 
 }  // namespace content