[go: up one dir, main page]

Don't enable "replace current entry" if there is no entry to replace.

BUG=457149
TEST=included; as in bug

Review URL: https://codereview.chromium.org/914893002

Cr-Commit-Position: refs/heads/master@{#316031}
(cherry picked from commit b6cc1564475f04f6598ac6a228c33ff3530d4450)

(This cherry-pick omits the test as it does not merge cleanly.)

Review URL: https://codereview.chromium.org/920343004

Cr-Commit-Position: refs/branch-heads/2272@{#325}
Cr-Branched-From: 827a380cfdb31aa54c8d56e63ce2c3fd8c3ba4d4-refs/heads/master@{#310958}
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index e501407..23cd703 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -727,7 +727,9 @@
       static_cast<SiteInstanceImpl*>(params.source_site_instance.get()));
   if (params.redirect_chain.size() > 0)
     entry->SetRedirectChain(params.redirect_chain);
-  if (params.should_replace_current_entry)
+  // 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);
@@ -1553,6 +1555,7 @@
   DiscardNonCommittedEntriesInternal();
 
   int current_size = static_cast<int>(entries_.size());
+  DCHECK(current_size > 0 || !replace);
 
   if (current_size > 0) {
     // Prune any entries which are in front of the current entry.
diff --git a/content/browser/frame_host/navigation_controller_impl_unittest.cc b/content/browser/frame_host/navigation_controller_impl_unittest.cc
index 7820fca..e628d5f 100644
--- a/content/browser/frame_host/navigation_controller_impl_unittest.cc
+++ b/content/browser/frame_host/navigation_controller_impl_unittest.cc
@@ -1328,6 +1328,15 @@
 // commit.
 TEST_F(NavigationControllerTest, ResetEntryValuesAfterCommit) {
   NavigationControllerImpl& controller = controller_impl();
+
+  // The value of "should replace entry" will be tested, but it's an error to
+  // specify it when there are no entries. Create a simple entry to be replaced.
+  const GURL url0("http://foo0");
+  controller.LoadURL(
+      url0, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
+  main_test_rfh()->SendNavigate(0, url0);
+
+  // Set up the pending entry.
   const GURL url1("http://foo1");
   controller.LoadURL(
       url1, Referrer(), ui::PAGE_TRANSITION_TYPED, std::string());
@@ -1340,8 +1349,6 @@
   scoped_refptr<base::RefCountedBytes> post_data =
       base::RefCountedBytes::TakeVector(&post_data_vector);
   GlobalRequestID transfer_id(3, 4);
-  std::vector<GURL> redirects;
-  redirects.push_back(GURL("http://foo2"));
 
   // Set non-persisted values on the pending entry.
   NavigationEntryImpl* pending_entry =
@@ -1357,7 +1364,8 @@
   EXPECT_TRUE(pending_entry->should_replace_entry());
   EXPECT_TRUE(pending_entry->should_clear_history_list());
 
-  main_test_rfh()->SendNavigate(0, url1);
+  // Fake a commit response.
+  main_test_rfh()->SendNavigate(1, url1);
 
   // Certain values that are only used for pending entries get reset after
   // commit.
diff --git a/content/renderer/render_frame_impl.cc b/content/renderer/render_frame_impl.cc
index 8cfe73e..dcda817 100644
--- a/content/renderer/render_frame_impl.cc
+++ b/content/renderer/render_frame_impl.cc
@@ -2432,6 +2432,8 @@
     // UpdateSessionHistory and update page_id_ even in this case, so that
     // the current entry gets a state update and so that we don't send a
     // state update to the wrong entry when we swap back in.
+    DCHECK(render_view_->history_list_length_ > 0 ||
+           !navigation_state->should_replace_current_entry());
     if (GetLoadingUrl() != GURL(kSwappedOutURL) &&
         !navigation_state->should_replace_current_entry()) {
       // Advance our offset in session history, applying the length limit.
@@ -4077,7 +4079,9 @@
     DocumentState* document_state = DocumentState::FromDataSource(ds);
     NavigationState* navigation_state = document_state->navigation_state();
     if (navigation_state->is_content_initiated()) {
-      params.should_replace_current_entry = ds->replacesCurrentHistoryItem();
+      params.should_replace_current_entry =
+          ds->replacesCurrentHistoryItem() &&
+          render_view_->history_list_length_;
     } else {
       // This is necessary to preserve the should_replace_current_entry value on
       // cross-process redirects, in the event it was set by a previous process.
diff --git a/content/test/data/navigation_controller/simple_page_1.html b/content/test/data/navigation_controller/simple_page_1.html
new file mode 100644
index 0000000..4b1d267
--- /dev/null
+++ b/content/test/data/navigation_controller/simple_page_1.html
@@ -0,0 +1,7 @@
+<html>
+<head>
+</head>
+<body>
+<p>Simple page 1.
+</body>
+</html>