Consider transient entry in entry count DCHECK

NavigationControllerImpl::SetTransientEntry adds an entry regardless of
the current number of entries, disregarding max_entry_count(). This is
fine, as the entry will not be committed. In fact, any subsequent
navigation will remove it.

However, if you call GetEntryCount() at that point, the DCHECK for the
entry count may fail, as it does not consider a transient entry an
exception. There is no need for this.

This change modifies the upper bound in the said DCHECK to depend on
whether or not a transient entry is present and adds a test verifying
that adding a transient entry when navigation history is full works
correctly.

Note that adding a transient entry is not a good time to prune the
oldest entry instead, as pending entry may be present. Pruning is not
safe at this point.

Bug: 979536
Change-Id: Ic30e3aa73e72c87a80656c6872209935159b31ae
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1681583Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Nasko Oskov <nasko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#675608}

content/browser/frame_host/navigation_controller_impl.cc

@@ -745,7 +745,8 @@ int NavigationControllerImpl::GetLastCommittedEntryIndex() {
 }
 
 int NavigationControllerImpl::GetEntryCount() {
-  DCHECK_LE(entries_.size(), max_entry_count());
+  DCHECK_LE(entries_.size(),
+            max_entry_count() + (transient_entry_index_ == -1 ? 0 : 1));
   return static_cast<int>(entries_.size());
 }
 
@@ -3326,10 +3327,10 @@ NavigationEntryImpl* NavigationControllerImpl::GetTransientEntry() {
 void NavigationControllerImpl::SetTransientEntry(
     std::unique_ptr<NavigationEntry> entry) {
   // Discard any current transient entry, we can only have one at a time.
+  DiscardTransientEntry();
   int index = 0;
   if (last_committed_entry_index_ != -1)
     index = last_committed_entry_index_ + 1;
-  DiscardTransientEntry();
   entries_.insert(entries_.begin() + index,
                   NavigationEntryImpl::FromNavigationEntry(std::move(entry)));
   if (pending_entry_index_ >= index)

content/browser/frame_host/navigation_controller_impl_unittest.cc

@@ -2848,6 +2848,57 @@ TEST_F(NavigationControllerTest, ReloadTransient) {
   EXPECT_EQ(controller.GetEntryAtIndex(1)->GetURL(), transient_url);
 }
 
+// Ensure that adding a transient entry works when history is full.
+TEST_F(NavigationControllerTest, TransientEntryWithFullHistory) {
+  NavigationControllerImpl& controller = controller_impl();
+
+  const GURL url0("http://foo/0");
+  const GURL url1("http://foo/1");
+  const GURL url2("http://foo/2");
+  const GURL transient_url("http://foo/transient");
+
+  // Maximum count should be at least 2 or we will not be able to perform
+  // another navigation, since it would need to prune the last committed entry
+  // which is not safe.
+  controller.set_max_entry_count_for_testing(2);
+  NavigationSimulator::NavigateAndCommitFromBrowser(contents(), url0);
+  NavigationSimulator::NavigateAndCommitFromBrowser(contents(), url1);
+
+  // Add a transient entry beyond entry count limit.
+  auto transient_entry = std::make_unique<NavigationEntryImpl>();
+  transient_entry->SetURL(transient_url);
+  controller.SetTransientEntry(std::move(transient_entry));
+
+  // Check our state.
+  EXPECT_EQ(transient_url, controller.GetVisibleEntry()->GetURL());
+  EXPECT_EQ(controller.GetEntryCount(), 3);
+  EXPECT_EQ(controller.GetLastCommittedEntryIndex(), 1);
+  EXPECT_EQ(controller.GetPendingEntryIndex(), -1);
+  EXPECT_TRUE(controller.GetLastCommittedEntry());
+  EXPECT_FALSE(controller.GetPendingEntry());
+  EXPECT_TRUE(controller.CanGoBack());
+  EXPECT_FALSE(controller.CanGoForward());
+
+  // Go back, removing the transient entry.
+  controller.GoBack();
+  EXPECT_EQ(url1, controller.GetVisibleEntry()->GetURL());
+  EXPECT_EQ(controller.GetEntryCount(), 2);
+
+  // Initiate a navigation, then add a transient entry with the pending entry
+  // present.
+  auto navigation =
+      NavigationSimulator::CreateBrowserInitiated(url2, contents());
+  navigation->Start();
+  auto another_transient = std::make_unique<NavigationEntryImpl>();
+  another_transient->SetURL(transient_url);
+  controller.SetTransientEntry(std::move(another_transient));
+  EXPECT_EQ(transient_url, controller.GetVisibleEntry()->GetURL());
+  EXPECT_EQ(controller.GetEntryCount(), 3);
+  navigation->Commit();
+  EXPECT_EQ(url2, controller.GetVisibleEntry()->GetURL());
+  EXPECT_EQ(controller.GetEntryCount(), 2);
+}
+
 // Ensure that renderer initiated pending entries get replaced, so that we
 // don't show a stale virtual URL when a navigation commits.
 // See http://crbug.com/266922.