OOPIF: Fix subframe back/forward after recreating FTNs (try #2).

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,
+        entry->AddOrUpdateFrameEntry(node, -1, -1, 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_));
@@ skipping 220 matching lines @@
 
     // Valid subframe navigation.
     return NAVIGATION_TYPE_NEW_SUBFRAME;
   }
 
   // Cross-process location.replace navigations should be classified as New with
   // replacement rather than ExistingPage, since it is not safe to reuse the
   // NavigationEntry.
   // TODO(creis): Have the renderer classify location.replace as
   // did_create_new_entry for all cases and eliminate this special case.  This
-  // requires updating several test expectations.  See https://crbug.com/317872.
+  // requires updating several test expectations.  See https://crbug.com/596707.
   if (!rfh->GetParent() && GetLastCommittedEntry() &&
       GetLastCommittedEntry()->site_instance() != rfh->GetSiteInstance() &&
       params.should_replace_current_entry) {
     return NAVIGATION_TYPE_NEW_PAGE;
   }
 
   // We only clear the session history when navigating to a new page.
   DCHECK(!params.history_list_was_cleared);
 
   if (rfh->GetParent()) {
@@ skipping 192 matching lines @@
   entry->SetReferrer(params.referrer);
   if (entry->update_virtual_url_with_url())
     UpdateVirtualURLToURL(entry, params.url);
 
   // Update the post parameters.
   FrameNavigationEntry* frame_entry =
       entry->GetFrameEntry(rfh->frame_tree_node());
   frame_entry->set_method(params.method);
   frame_entry->set_post_id(params.post_id);
 
+  // Update the ISN and DSN in case this was a location.replace, which can cause
+  // them to change.
+  // TODO(creis): Classify location.replace as NEW_PAGE instead of EXISTING_PAGE
+  // in https://crbug.com/596707.
+  frame_entry->set_item_sequence_number(params.item_sequence_number);
+  frame_entry->set_document_sequence_number(params.document_sequence_number);
+
   // The redirected to page should not inherit the favicon from the previous
   // page.
   if (ui::PageTransitionIsRedirect(params.transition))
     entry->GetFavicon() = FaviconStatus();
 
   // 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());
   entry->set_site_instance(
@@ skipping 58 matching lines @@
 
   // Manual subframe navigations just get the current entry cloned so the user
   // can go back or forward to it. The actual subframe information will be
   // stored in the page state for each of those entries. This happens out of
   // band with the actual navigations.
   DCHECK(GetLastCommittedEntry()) << "ClassifyNavigation should guarantee "
                                   << "that a last committed entry exists.";
 
   std::unique_ptr<NavigationEntryImpl> new_entry;
   if (SiteIsolationPolicy::UseSubframeNavigationEntries()) {
-    // Make sure new_entry takes ownership of frame_entry in a scoped_refptr.
-    FrameNavigationEntry* frame_entry = new FrameNavigationEntry(
-        rfh->frame_tree_node()->frame_tree_node_id(), params.frame_unique_name,
-        params.item_sequence_number, params.document_sequence_number,
-        rfh->GetSiteInstance(), params.url, params.referrer, params.method,
-        params.post_id);
+    // Make sure we don't leak frame_entry if new_entry doesn't take ownership.
+    scoped_refptr<FrameNavigationEntry> frame_entry(new FrameNavigationEntry(

[Charlie Reis, 2016/04/27 19:12:57]

This was a bug in my https://codereview.chromium.org/1303333006, where we would
leak this FNE if it didn't get used.

The ASAN bot caught the leak in the SitePerProcessBrowserTest.DynamicWindowName
test, which now exercises this path (due to https://crbug.com/607205).

I'll fix https://crbug.com/607205 in a followup CL, to avoid cramming too much
into this CL.  It affects default Chrome anyway.

[alexmos, 2016/04/28 01:05:03]

Acknowledged.

+        params.frame_unique_name, params.item_sequence_number,
+        params.document_sequence_number, rfh->GetSiteInstance(), params.url,
+        params.referrer, params.method, params.post_id));
     new_entry = GetLastCommittedEntry()->CloneAndReplace(rfh->frame_tree_node(),
-                                                         frame_entry);
+                                                         frame_entry.get());
 
-    // TODO(creis): Make sure the last committed entry always has the subframe
-    // entry to replace, and CHECK(frame_entry->HasOneRef).  For now, we might
-    // not find the entry to replace, and new_entry will be deleted when it goes
-    // out of scope.  See https://crbug.com/522193.
+    // TODO(creis): Update this to add the frame_entry if we can't find the one
+    // to replace, which can happen due to a unique name change.  See
+    // https://crbug.com/607205.  For now, frame_entry will be deleted when it
+    // goes out of scope if it doesn't get used.
   } else {
     new_entry = GetLastCommittedEntry()->Clone();
   }
 
   new_entry->SetPageID(params.page_id);
   InsertOrReplaceEntry(std::move(new_entry), replace_entry);
 }
 
 bool NavigationControllerImpl::RendererDidNavigateAutoSubframe(
     RenderFrameHostImpl* rfh,
@@ skipping 31 matching lines @@
       DiscardNonCommittedEntriesInternal();
       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.frame_unique_name,
-        params.item_sequence_number, params.document_sequence_number,
-        rfh->GetSiteInstance(), params.url, params.referrer, params.page_state,
-        params.method, params.post_id);
+        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);
 
     // 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