Move ownership of source SiteInstance to the 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 718 matching lines @@
       DCHECK(GetLastCommittedEntry());
 
       // Update the FTN ID to use below in case we found a named frame.
       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, params.url,
-                                     params.referrer, PageState(), "GET", -1);
+        entry->AddOrUpdateFrameEntry(
+            node, -1, -1,
+            static_cast<SiteInstanceImpl*>(params.source_site_instance.get()),
+            nullptr, 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->root_node()->frame_entry->set_source_site_instance(

[Charlie Reis, 2016/05/04 23:03:01]

Hmm.  I think I'd prefer to have a method on NavigationEntryImpl for this after
all, because it's unfortunate to expose TreeNode outside NavigationEntry.  I
kind of want to make that a private detail of NavigationEntry-- it looks like
root_node() is only used in tests these days.

[nasko, 2016/05/05 17:51:33]

Done.

+        static_cast<SiteInstanceImpl*>(params.source_site_instance.get()));

[Charlie Reis, 2016/05/04 23:03:01]

Just curious-- was there a reason not to move this parameter into
CreateNavigationEntry (and the FrameNavigationEntry constructor)?  Maybe that
had too many call sites?

[nasko, 2016/05/05 17:51:33]

It is content/public/ API. I don't think it is worthwhile adding at this point,
since I haven't seen anyone outside of content creating such a NavigationEntry.
There is usage of source_site_instance outside of content/, but it is passed
through the various params structs.

[Charlie Reis, 2016/05/05 22:20:00]

Acknowledged.

   }
+
   // Set the FTN ID (only used in non-site-per-process, for tests).
   entry->set_frame_tree_node_id(frame_tree_node_id);
-  entry->set_source_site_instance(
-      static_cast<SiteInstanceImpl*>(params.source_site_instance.get()));
   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);
@@ skipping 116 matching lines @@
   // We should not have a pending entry anymore.  Clear it again in case any
   // error cases above forgot to do so.
   DiscardNonCommittedEntriesInternal();
 
   // All committed entries should have nonempty content state so WebKit doesn't
   // get confused when we go back to them (see the function for details).
   DCHECK(params.page_state.IsValid());
   NavigationEntryImpl* active_entry = GetLastCommittedEntry();
   active_entry->SetTimestamp(timestamp);
   active_entry->SetHttpStatusCode(params.http_status_code);
+
+  // Update the frame-specific PageState.

[Charlie Reis, 2016/05/04 23:03:01]

This comment belongs inside the conditional, since the else branch does a
full-page PageState rather than a frame-specifc one.

[nasko, 2016/05/05 17:51:33]

Done.

+  FrameNavigationEntry* frame_entry =
+      active_entry->GetFrameEntry(rfh->frame_tree_node());
   if (SiteIsolationPolicy::UseSubframeNavigationEntries()) {
-    // Update the frame-specific PageState.
-    FrameNavigationEntry* frame_entry =
-        active_entry->GetFrameEntry(rfh->frame_tree_node());
     // 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)
       frame_entry->set_page_state(params.page_state);
   } else {
     active_entry->SetPageState(params.page_state);
   }
   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
   // the entry.
-  active_entry->ResetForCommit();
+  active_entry->ResetForCommit(frame_entry);
 
   // The active entry's SiteInstance should match our SiteInstance.
   // TODO(creis): This check won't pass for subframes until we create entries
   // for subframe navigations.
   if (!rfh->GetParent())
     CHECK_EQ(active_entry->site_instance(), rfh->GetSiteInstance());
 
   // Remember the bindings the renderer process has at this point, so that
   // we do not grant this entry additional bindings if we come back to it.
   active_entry->SetBindings(rfh->GetEnabledBindings());
@@ skipping 407 matching lines @@
       return true;
     }
   }
 
   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(), params.url,
-        params.referrer, params.page_state, params.method, params.post_id);
+        params.document_sequence_number, nullptr, rfh->GetSiteInstance(),
+        params.url, params.referrer, params.page_state, params.method,
+        params.post_id);
 
     // Cross-process subframe navigations may leave a pending entry around.
     // Clear it if it's actually for the subframe.
     // TODO(creis): Don't use pending entries for subframe navigations.
     // See https://crbug.com/495161.
     if (pending_entry_ &&
         pending_entry_->frame_tree_node_id() ==
             rfh->frame_tree_node()->frame_tree_node_id()) {
       DiscardPendingEntry(false);
     }
@@ skipping 699 matching lines @@
     }
   }
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content