Move redirect_chain from NavigationEntry to FrameNavigationEntry.

content/browser/frame_host/navigation_controller_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.
 
 /*
  * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  *     (http://www.torchmobile.com/)
  *
@@ skipping 686 matching lines @@
       frame_tree_node_id = node->frame_tree_node_id();
 
       // In --site-per-process, create an identical NavigationEntry with a
       // new FrameNavigationEntry for the target subframe.
       if (SiteIsolationPolicy::UseSubframeNavigationEntries()) {
         entry = GetLastCommittedEntry()->Clone();
         entry->SetPageID(-1);
         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);
       }
     }
   }
 
   // Otherwise, create a pending entry for the main frame.
   if (!entry) {
     entry = NavigationEntryImpl::FromNavigationEntry(CreateNavigationEntry(
         params.url, params.referrer, params.transition_type,
         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)
     entry->set_should_replace_entry(true);
   entry->set_should_clear_history_list(params.should_clear_history_list);
   entry->SetIsOverridingUserAgent(override);
   entry->set_transferred_global_request_id(
       params.transferred_global_request_id);
 
 #if defined(OS_ANDROID)
@@ skipping 128 matching lines @@
     base::debug::DumpWithoutCrashing();
     NOTREACHED() << "Shouldn't see an empty PageState at commit.";
   }
   NavigationEntryImpl* active_entry = GetLastCommittedEntry();
   active_entry->SetTimestamp(timestamp);
   active_entry->SetHttpStatusCode(params.http_status_code);
 
   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.
   size_t redirect_chain_size = 0;
   for (size_t i = 0; i < params.redirects.size(); ++i) {
     redirect_chain_size += params.redirects[i].spec().length();
   }
   UMA_HISTOGRAM_COUNTS("Navigation.RedirectChainSize", redirect_chain_size);
 
   // Once it is committed, we no longer need to track several pieces of state on
@@ skipping 311 matching lines @@
   // The site instance will normally be the same except during session restore,
   // when no site instance will be assigned.
   DCHECK(entry->site_instance() == nullptr ||
          entry->site_instance() == rfh->GetSiteInstance());
 
   // Update the existing FrameNavigationEntry to ensure all of its members
   // reflect the parameters coming from the renderer process.
   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.
   if (ui::PageTransitionIsRedirect(params.transition) && !is_in_page)
     entry->GetFavicon() = FaviconStatus();
 
   // The entry we found in the list might be pending if the user hit
   // back/forward/reload. This load should commit it (since it's already in the
   // list, we can just discard the pending pointer).  We should also discard the
   // pending entry if it corresponds to a different navigation, since that one
@@ skipping 36 matching lines @@
   // If a user presses enter in the omnibox and the server redirects, the URL
   // might change (but it's still considered a SAME_PAGE navigation). So we must
   // update the SSL status.
   existing_entry->GetSSL() = rfh->navigation_handle()->ssl_status();
 
   // Update the existing FrameNavigationEntry to ensure all of its members
   // reflect the parameters coming from the renderer process.
   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();
 }
 
 void NavigationControllerImpl::RendererDidNavigateNewSubframe(
     RenderFrameHostImpl* rfh,
     const FrameHostMsg_DidCommitProvisionalLoad_Params& params,
     bool is_in_page,
     bool replace_entry) {
   DCHECK(ui::PageTransitionCoreTypeIs(params.transition,
@@ skipping 77 matching lines @@
     }
   }
 
   if (SiteIsolationPolicy::UseSubframeNavigationEntries()) {
     // This may be a "new auto" case where we add a new FrameNavigationEntry, or
     // it may be a "history auto" case where we update an existing one.
     NavigationEntryImpl* last_committed = GetLastCommittedEntry();
     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;
 }
 
 int NavigationControllerImpl::GetIndexOfEntry(
     const NavigationEntryImpl* entry) const {
   for (size_t i = 0; i < entries_.size(); ++i) {
     if (entries_[i].get() == entry)
       return i;
@@ skipping 722 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content