Move redirect_chain from NavigationEntry to FrameNavigationEntry.

content/browser/frame_host/navigation_controller_impl.cc

Index: content/browser/frame_host/navigation_controller_impl.cc
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index c0ddb1b926e0af03b8758c235c8e8a4026164be5..32a76dd9ac485029e2c19e3a62d60efcb9554c6a 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -704,7 +704,8 @@ void NavigationControllerImpl::LoadURLWithParams(const LoadURLParams& params) {
         entry->AddOrUpdateFrameEntry(
             node, -1, -1, nullptr,
             static_cast<SiteInstanceImpl*>(params.source_site_instance.get()),
-            params.url, params.referrer, PageState(), "GET", -1);
+            params.redirect_chain, params.url, params.referrer, PageState(),
+            "GET", -1);
       }
     }
   }
@@ -716,12 +717,12 @@ void NavigationControllerImpl::LoadURLWithParams(const LoadURLParams& params) {
         params.is_renderer_initiated, params.extra_headers, browser_context_));
     entry->set_source_site_instance(
         static_cast<SiteInstanceImpl*>(params.source_site_instance.get()));
+    if (params.redirect_chain.size() > 0)
+      entry->SetMainFrameRedirectChain(params.redirect_chain);

[arthursonzogni, 2016/09/27 12:56:20]

I don't know the reasons why the main frame redirection chain is optionally
updated. Do we have to do the same thing a few lines above for subframes too?

[Charlie Reis, 2016/09/28 04:13:17]

Let's remove the size check.  Originally, calling SetRedirectChain with a zero
size chain would have been no-op, but that's less clear as this method has grown
to support subframes.

[arthursonzogni, 2016/09/28 16:36:14]

Done.

   }
 
   // Set the FTN ID (only used in non-site-per-process, for tests).
   entry->set_frame_tree_node_id(frame_tree_node_id);
-  if (params.redirect_chain.size() > 0)
-    entry->SetRedirectChain(params.redirect_chain);
   // Don't allow an entry replacement if there is no entry to replace.
   // http://crbug.com/457149
   if (params.should_replace_current_entry && entries_.size() > 0)
@@ -870,15 +871,17 @@ bool NavigationControllerImpl::RendererDidNavigate(
   FrameNavigationEntry* frame_entry =
       active_entry->GetFrameEntry(rfh->frame_tree_node());
   if (SiteIsolationPolicy::UseSubframeNavigationEntries()) {
-    // Update the frame-specific PageState.
+    // Update the frame-specific PageState and RedirectChain
     // We may not find a frame_entry in some cases; ignore the PageState if so.
     // TODO(creis): Remove the "if" once https://crbug.com/522193 is fixed.
-    if (frame_entry)
+    if (frame_entry) {
       frame_entry->SetPageState(params.page_state);
+      frame_entry->set_redirect_chain(params.redirects);
+    }
   } else {
     active_entry->SetPageState(params.page_state);
+    active_entry->SetMainFrameRedirectChain(params.redirects);
   }
-  active_entry->SetRedirectChain(params.redirects);
 
   // Use histogram to track memory impact of redirect chain because it's now
   // not cleared for committed entries.
@@ -1210,8 +1213,8 @@ void NavigationControllerImpl::RendererDidNavigateToExistingPage(
   entry->AddOrUpdateFrameEntry(
       rfh->frame_tree_node(), params.item_sequence_number,
       params.document_sequence_number, rfh->GetSiteInstance(), nullptr,
-      params.url, params.referrer, params.page_state, params.method,
-      params.post_id);
+      params.redirects, params.url, params.referrer, params.page_state,
+      params.method, params.post_id);

[arthursonzogni, 2016/09/27 12:56:20]

I want to warn that previously, params.redirects was not used to update the
NavigationEntry. I would like to check that it is not wrong to update the
FrameNavigationEntry.

The same thing happens also in the next two modifications of this file.

[Charlie Reis, 2016/09/28 04:13:17]

This case and the SamePage one were covered by the
active_entry->SetRedirectChain(params.redirects) call in RendererDidNavigate
(around 881).  Looks like we'll assign it redundantly, but maybe that's ok.

The AutoSubframe case didn't used to track redirects for subframes (since it
returned early), so it seems fine to add it now.

I think you discovered a bug in the NewSubframe case.  That does go through the
SetRedirectChain call in RendererDidNavigate, meaning that subframe redirects
used to end up on the main frame.  (I confirmed in a debugger.)  Glad we're
fixing it!

Repro steps, if you're curious:
0) Start testserver.py.
1) Visit http://csreis.github.io/tests/cross-site-iframe.html.
2) In the DevTools console, enter: 
navFrame("http://127.0.0.1:8080/server-redirect?https://csreis.github.io");

At this point, NavigationEntry::GetRedirectChain will incorrectly show the
redirect chain for the subframe.  Your change should fix that.

[arthursonzogni, 2016/09/28 16:36:14]

I'm glad that it fix a new bug! Thanks for the repro steps.

 
   // The redirected to page should not inherit the favicon from the previous
   // page.
@@ -1268,8 +1271,8 @@ void NavigationControllerImpl::RendererDidNavigateToSamePage(
   existing_entry->AddOrUpdateFrameEntry(
       rfh->frame_tree_node(), params.item_sequence_number,
       params.document_sequence_number, rfh->GetSiteInstance(), nullptr,
-      params.url, params.referrer, params.page_state, params.method,
-      params.post_id);
+      params.redirects, params.url, params.referrer, params.page_state,
+      params.method, params.post_id);
 
   DiscardNonCommittedEntries();
 }
@@ -1367,8 +1370,8 @@ bool NavigationControllerImpl::RendererDidNavigateAutoSubframe(
     last_committed->AddOrUpdateFrameEntry(
         rfh->frame_tree_node(), params.item_sequence_number,
         params.document_sequence_number, rfh->GetSiteInstance(), nullptr,
-        params.url, params.referrer, params.page_state, params.method,
-        params.post_id);
+        params.redirects, params.url, params.referrer, params.page_state,
+        params.method, params.post_id);
   }
 
   return send_commit_notification;