NavigationController: Fix several broken class invariants.

content/browser/frame_host/navigation_controller_impl.cc

Index: content/browser/frame_host/navigation_controller_impl.cc
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index 768f595fac70a59363483b33f06821983b03fc60..6b2f51d48da88cb79de218d3549b81507eb40d8e 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -279,6 +279,7 @@ void NavigationControllerImpl::Restore(
   DCHECK(GetEntryCount() == 0 && !GetPendingEntry());
   DCHECK(selected_navigation >= 0 &&
          selected_navigation < static_cast<int>(entries->size()));
+  DCHECK_EQ(pending_entry_index_, -1);

[Charlie Reis, 2017/04/24 03:58:13]

nit: Swap order (expected, actual).

Same in all the cases below.

[arthursonzogni, 2017/04/24 16:28:17]

Done.

 
   needs_reload_ = true;
   entries_.reserve(entries->size());
@@ -447,10 +448,14 @@ void NavigationControllerImpl::LoadEntry(
     std::unique_ptr<NavigationEntryImpl> entry) {
   // Remember the last pending entry for which we haven't received a response
   // yet. This will be deleted in the NavigateToPendingEntry() function.
+  DCHECK_EQ(last_pending_entry_, nullptr);
+  DCHECK_EQ(last_pending_entry_index_, -1);
   last_pending_entry_ = pending_entry_;
   last_pending_entry_index_ = pending_entry_index_;
   last_transient_entry_index_ = transient_entry_index_;
+
   pending_entry_ = nullptr;
+  pending_entry_index_ = -1;
   // When navigating to a new page, we don't know for sure if we will actually
   // end up leaving the current page.  The new page load could for example
   // result in a download or a 'no content' response (e.g., a mailto: URL).
@@ -462,6 +467,7 @@ void NavigationControllerImpl::SetPendingEntry(
     std::unique_ptr<NavigationEntryImpl> entry) {
   DiscardNonCommittedEntriesInternal();
   pending_entry_ = entry.release();
+  DCHECK_EQ(pending_entry_index_, -1);
   NotificationService::current()->Notify(
       NOTIFICATION_NAV_ENTRY_PENDING,
       Source<NavigationController>(this),
@@ -534,11 +540,14 @@ bool NavigationControllerImpl::CanViewSource() const {
 }
 
 int NavigationControllerImpl::GetLastCommittedEntryIndex() const {
+  // The last committed entry index must always be less than the number of
+  // entries.  It should be -1 iff there are no entries.
+  DCHECK_LT(last_committed_entry_index_, GetEntryCount());

[Charlie Reis, 2017/04/24 03:58:13]

Can we also include the second DCHECK from my CL (below), or did that not work?
DCHECK_EQ(last_committed_entry_index_ == -1, GetEntryCount() == 0);

That's what the second sentence is about, and it doesn't strictly follow from
the DCHECK_LT.  Currently, it could be -1 when there are entries.  If that can
happen in some cases (e.g., duplicating a tab?), then we should rephrase or
remove the second sentence in the comment.

[arthursonzogni, 2017/04/24 16:28:17]

We can't because:
https://codereview.chromium.org/2810173002/#msg40
```
FYI: I found that the assertion:
DCHECK_EQ(last_committed_entry_index_ == -1, GetEntryCount() == 0);
is not true when a transient entry is added before that any entry are committed.
There is a test for this: WebContentsImplTest.ShowInterstitialThenNavigate
```

Then I don't know if it is possible to add transient entry without having
committed any entry outside of tests.

I will update the comment in my next patch.

   return last_committed_entry_index_;
 }
 
 int NavigationControllerImpl::GetEntryCount() const {
-  DCHECK(entries_.size() <= max_entry_count());
+  DCHECK_LE(entries_.size(), max_entry_count());
   return static_cast<int>(entries_.size());
 }
 
@@ -615,11 +624,12 @@ void NavigationControllerImpl::GoToIndex(int index) {
 
   DiscardNonCommittedEntries();
 
+  DCHECK_EQ(pending_entry_, nullptr);
+  DCHECK_EQ(pending_entry_index_, -1);
+  pending_entry_ = entries_[index].get();
   pending_entry_index_ = index;
-  entries_[pending_entry_index_]->SetTransitionType(
-      ui::PageTransitionFromInt(
-          entries_[pending_entry_index_]->GetTransitionType() |
-          ui::PAGE_TRANSITION_FORWARD_BACK));
+  pending_entry_->SetTransitionType(ui::PageTransitionFromInt(
+      pending_entry_->GetTransitionType() | ui::PAGE_TRANSITION_FORWARD_BACK));
   NavigateToPendingEntry(ReloadType::NONE);
 }
 
@@ -1726,10 +1736,22 @@ void NavigationControllerImpl::DiscardNonCommittedEntries() {
 }
 
 NavigationEntryImpl* NavigationControllerImpl::GetPendingEntry() const {
+  // If there is no pending_entry_, there should be no pending_entry_index_.
+  DCHECK(pending_entry_ || pending_entry_index_ == -1);
+
+  // If there is a pending_entry_index_, then pending_entry_ must be the entry
+  // at that index.
+  DCHECK(pending_entry_index_ == -1 ||
+         pending_entry_ == GetEntryAtIndex(pending_entry_index_));
+
   return pending_entry_;
 }
 
 int NavigationControllerImpl::GetPendingEntryIndex() const {
+  // The pending entry index must always be less than the number of entries.
+  // If there are no entries, it must be exactly -1.
+  DCHECK_LT(pending_entry_index_, GetEntryCount());
+  DCHECK(GetEntryCount() != 0 || pending_entry_index_ == -1);
   return pending_entry_index_;
 }
 
@@ -1791,6 +1813,7 @@ void NavigationControllerImpl::PruneOldestEntryIfFull() {
 }
 
 void NavigationControllerImpl::NavigateToPendingEntry(ReloadType reload_type) {
+  DCHECK(pending_entry_);
   needs_reload_ = false;
 
   // If we were navigating to a slow-to-commit page, and the user performs
@@ -1801,9 +1824,8 @@ void NavigationControllerImpl::NavigateToPendingEntry(ReloadType reload_type) {
   // page from loading (which would normally happen during the navigation).
   if (pending_entry_index_ != -1 &&
       pending_entry_index_ == last_committed_entry_index_ &&
-      (entries_[pending_entry_index_]->restore_type() == RestoreType::NONE) &&
-      (entries_[pending_entry_index_]->GetTransitionType() &
-       ui::PAGE_TRANSITION_FORWARD_BACK)) {
+      pending_entry_->restore_type() == RestoreType::NONE &&
+      pending_entry_->GetTransitionType() & ui::PAGE_TRANSITION_FORWARD_BACK) {
     delegate_->Stop();
 
     // If an interstitial page is showing, we want to close it to get back
@@ -1833,9 +1855,10 @@ void NavigationControllerImpl::NavigateToPendingEntry(ReloadType reload_type) {
   // FrameLoader, but we want to handle it here so that if the navigation is
   // redirected or handled purely on the browser side in PlzNavigate we have the
   // same behaviour as Blink would. Note that we don't want to convert to a
-  // reload for history navigations, so this must be above the retrieval of the
-  // pending_entry_ below when pending_entry_index_ is used.
-  if (reload_type == ReloadType::NONE && last_navigation && pending_entry_ &&
+  // reload for history navigations.
+  if (reload_type == ReloadType::NONE && last_navigation &&
+      !(pending_entry_->GetTransitionType() &
+        ui::PAGE_TRANSITION_FORWARD_BACK) &&

[Charlie Reis, 2017/04/24 03:58:13]

Let's avoid using PageTransition for any meaningful navigation decisions, since
I don't trust it in all cases.

Can we check for pending_entry_ && pending_entry_index_ == -1 instead?  I think
that equates to a non-session-history pending navigation now.

[arthursonzogni, 2017/04/24 16:28:17]

Okay, please note that pending_entry_ is already non-null since this CL. I have
also added a DCHECK(pending_entry_) above.

[Charlie Reis, 2017/04/24 17:34:55]

Acknowledged.

       // Please refer to the ShouldTreatNavigationAsReload() function for info
       // on which navigations are treated as reloads. In general navigating to
       // the last committed or pending entry via the address bar, clicking on
@@ -1870,12 +1893,6 @@ void NavigationControllerImpl::NavigateToPendingEntry(ReloadType reload_type) {
   last_pending_entry_ = nullptr;
   last_pending_entry_index_ = -1;
 
-  // For session history navigations only the pending_entry_index_ is set.
-  if (!pending_entry_) {
-    CHECK_NE(pending_entry_index_, -1);
-    pending_entry_ = entries_[pending_entry_index_].get();
-  }
-
   // Any renderer-side debug URLs or javascript: URLs should be ignored if the
   // renderer process is not live, unless it is the initial navigation of the
   // tab.
@@ -2044,6 +2061,7 @@ void NavigationControllerImpl::LoadIfNecessary() {
   if (pending_entry_) {
     NavigateToPendingEntry(ReloadType::NONE);
   } else if (last_committed_entry_index_ != -1) {
+    pending_entry_ = entries_[last_committed_entry_index_].get();
     pending_entry_index_ = last_committed_entry_index_;
     NavigateToPendingEntry(ReloadType::NONE);
   } else {
@@ -2091,10 +2109,12 @@ void NavigationControllerImpl::DiscardPendingEntry(bool was_failure) {
     failed_pending_entry_id_ = 0;
   }
 
-  if (pending_entry_index_ == -1)
-    delete pending_entry_;
-  pending_entry_ = NULL;
-  pending_entry_index_ = -1;
+  if (pending_entry_) {
+    if (pending_entry_index_ == -1)
+      delete pending_entry_;
+    pending_entry_index_ = -1;
+    pending_entry_ = nullptr;
+  }
 }
 
 void NavigationControllerImpl::SetPendingNavigationSSLError(bool error) {
@@ -2108,6 +2128,8 @@ void NavigationControllerImpl::DiscardTransientEntry() {
   entries_.erase(entries_.begin() + transient_entry_index_);
   if (last_committed_entry_index_ > transient_entry_index_)
     last_committed_entry_index_--;
+  if (pending_entry_index_ > transient_entry_index_)

[Charlie Reis, 2017/04/24 03:58:13]

(Same question as below.)

+    pending_entry_index_--;
   transient_entry_index_ = -1;
 }
 
@@ -2135,6 +2157,8 @@ void NavigationControllerImpl::SetTransientEntry(
   DiscardTransientEntry();
   entries_.insert(entries_.begin() + index,
                   NavigationEntryImpl::FromNavigationEntry(std::move(entry)));
+  if (pending_entry_index_ >= index)

[Charlie Reis, 2017/04/24 03:58:13]

Thanks for pointing me to the CL showing the failure.  However, there's still
something I don't yet understand.  The rest of this CL seems like it isn't
really changing behavior, other than an internal implementation detail that
pending_entry_index_ and pending_entry_ always agree now.  But this looks like a
definite behavior change, and one that affects test outcomes.

Does this mean we had a bug before, and your new DCHECKs caught it?  That is,
was the GetPendingEntry() call from FocusLocationBarByDefault returning the
wrong entry before?

I just want to understand this a bit more before we land it, to make sure we
aren't regressing anything with the behavior change.

[arthursonzogni, 2017/04/24 16:28:17]

Maybe we didn't catch it before, because pending_entry_index_ was wrong only for
a short time between the call to SetTransientEntry and and
DiscardTransientEntry. Two bugs that compensate each other :-)

There was no problem with FocusLocationBarByDefault despite the bug since it
only use GetPendingEntry(), and GetPendingEntry() doesn't use
pending_entry_index_ at all.

Outside of the NavigationController, GetPendingEntryIndex() is mainly used to
check if it isn't equals to -1. Which is not a problem.

There is 2 functions that are using GetPendingEntryIndex() for something else:
- SessionService::BuildCommandsForTabs
- TabContentsSyncedTabDelegate::GetPossiblyPendingEntryAtIndex

TabContentsSyncedTabDelegate::GetPossiblyPendingEntryAtIndex is weird because it
could be replaced by GetEntryAtIndex if we assume that the class invariant is
true.

I don't really know how the transient entry are used.

[Charlie Reis, 2017/04/24 17:34:55]

More specifically, I think this means we might have had a bug any time
GetPendingEntryIndex() is called while an interstitial is showing (and when it's
larger than the interstitial's transient index)?  Seems like it might have been
off-by-one that whole time, until the interstitial is dismissed.
Yes, I think I agree-- pending_entry_ doesn't seem like it would be affected.
Yes, maybe this off-by-one bug is the reason people went through such strange
contortions to get the pending entry in places like
GetPossiblyPendingEntryAtIndex.  I agree we should be able to simplify it now,
thanks to this fix.

To be sure, can you add a unit test to navigation_controller_impl_unittest.cc
showing that GetPendingEntryIndex() was wrong when adding a transient entry on a
forward navigation, and that your CL fixes it?  I would expect that test to fail
even without all the DCHECKs, and that will improve our test coverage in this
area.

(That said, I can't see it explaining the code or the crash in
WebContentsState::GetContentsStateAsByteBuffer.)
They're used for interstitial pages, and represent entries that will go away as
soon as you leave the interstitial.  They're certainly tricky to reason about,
though.

+    pending_entry_index_++;
   transient_entry_index_ = index;
   delegate_->NotifyNavigationStateChanged(INVALIDATE_TYPE_ALL);
 }
@@ -2154,6 +2178,8 @@ void NavigationControllerImpl::InsertEntriesFrom(
                       source.entries_[i]->Clone());
     }
   }
+  DCHECK(pending_entry_index_ == -1 ||
+         pending_entry_ == GetEntryAtIndex(pending_entry_index_));
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(