NavigationController: Fix several broken class invariants.

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 261 matching lines @@
 }
 
 void NavigationControllerImpl::Restore(
     int selected_navigation,
     RestoreType type,
     std::vector<std::unique_ptr<NavigationEntry>>* entries) {
   // Verify that this controller is unused and that the input is valid.
   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());
   for (auto& entry : *entries)
     entries_.push_back(
         NavigationEntryImpl::FromNavigationEntry(std::move(entry)));
 
   // At this point, the |entries| is full of empty scoped_ptrs, so it can be
   // cleared out safely.
   entries->clear();
@@ skipping 148 matching lines @@
 NavigationEntryImpl*
 NavigationControllerImpl::GetEntryWithUniqueID(int nav_entry_id) const {
   int index = GetEntryIndexWithUniqueID(nav_entry_id);
   return (index != -1) ? entries_[index].get() : nullptr;
 }
 
 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).
   SetPendingEntry(std::move(entry));
   NavigateToPendingEntry(ReloadType::NONE);
 }
 
 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),
       Details<NavigationEntry>(pending_entry_));
 }
 
 NavigationEntryImpl* NavigationControllerImpl::GetActiveEntry() const {
   if (transient_entry_index_ != -1)
     return entries_[transient_entry_index_].get();
   if (pending_entry_)
@@ skipping 52 matching lines @@
   const std::string& mime_type = delegate_->GetContentsMimeType();
   bool is_viewable_mime_type =
       mime_util::IsSupportedNonImageMimeType(mime_type) &&
       !media::IsSupportedMediaMimeType(mime_type);
   NavigationEntry* visible_entry = GetVisibleEntry();
   return visible_entry && !visible_entry->IsViewSourceMode() &&
       is_viewable_mime_type && !delegate_->GetInterstitialPage();
 }
 
 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());
 }
 
 NavigationEntryImpl* NavigationControllerImpl::GetEntryAtIndex(
     int index) const {
   if (index < 0 || index >= GetEntryCount())
     return nullptr;
 
   return entries_[index].get();
 }
@@ skipping 56 matching lines @@
       return;
     }
     if (index > transient_entry_index_) {
       // Removing the transient is goint to shift all entries by 1.
       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);
 }
 
 void NavigationControllerImpl::GoToOffset(int offset) {
   // Note: This is actually reached in unit tests.
   if (!CanGoToOffset(offset))
     return;
 
   GoToIndex(GetIndexForOffset(offset));
 }
@@ skipping 1086 matching lines @@
   DiscardNonCommittedEntriesInternal();
 
   // If there was a transient entry, invalidate everything so the new active
   // entry state is shown.
   if (transient) {
     delegate_->NotifyNavigationStateChanged(INVALIDATE_TYPE_ALL);
   }
 }
 
 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_;
 }
 
 void NavigationControllerImpl::InsertOrReplaceEntry(
     std::unique_ptr<NavigationEntryImpl> entry,
     bool replace) {
   DCHECK(!ui::PageTransitionCoreTypeIs(entry->GetTransitionType(),
                                        ui::PAGE_TRANSITION_AUTO_SUBFRAME));
 
   // If the pending_entry_index_ is -1, the navigation was to a new page, and we
@@ skipping 41 matching lines @@
 void NavigationControllerImpl::PruneOldestEntryIfFull() {
   if (entries_.size() >= max_entry_count()) {
     DCHECK_EQ(max_entry_count(), entries_.size());
     DCHECK_GT(last_committed_entry_index_, 0);
     RemoveEntryAtIndex(0);
     NotifyPrunedEntries(this, true, 1);
   }
 }
 
 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
   // a session history navigation to the last committed page, RenderViewHost
   // will force the throbber to start, but WebKit will essentially ignore the
   // navigation, and won't send a message to stop the throbber. To prevent this
   // from happening, we drop the navigation here and stop the slow-to-commit
   // 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
     // to what was showing before.
     if (delegate_->GetInterstitialPage())
       delegate_->GetInterstitialPage()->DontProceed();
 
     DiscardNonCommittedEntries();
     return;
   }
 
   // If an interstitial page is showing, the previous renderer is blocked and
   // cannot make new requests.  Unblock (and disable) it to allow this
   // navigation to succeed.  The interstitial will stay visible until the
   // resulting DidNavigate.
   if (delegate_->GetInterstitialPage()) {
     static_cast<InterstitialPageImpl*>(delegate_->GetInterstitialPage())
         ->CancelForNavigation();
   }
 
   // The last navigation is the last pending navigation which hasn't been
   // committed yet, or the last committed navigation.
   NavigationEntryImpl* last_navigation =
       last_pending_entry_ ? last_pending_entry_ : GetLastCommittedEntry();
 
   // Convert Enter-in-omnibox to a reload. This is what Blink does in
   // 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
       // a link, etc would be treated as reloads.
       ShouldTreatNavigationAsReload(pending_entry_) &&
       // Skip entries with SSL errors.
       !last_navigation->ssl_error() &&
       // Ignore interstitial pages
       last_transient_entry_index_ == -1 &&
       pending_entry_->frame_tree_node_id() == -1 &&
@@ skipping 14 matching lines @@
     reload_type = ReloadType::NORMAL;
   }
 
   if (last_pending_entry_index_ == -1 && last_pending_entry_)
     delete last_pending_entry_;
 
   last_transient_entry_index_ = -1;
   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.
   if (IsRendererDebugURL(pending_entry_->GetURL())) {
     // TODO(creis): Find the RVH for the correct frame.
     if (!delegate_->GetRenderViewHost()->IsRenderViewLive() &&
         !IsInitialNavigation()) {
       DiscardNonCommittedEntries();
       return;
     }
@@ skipping 148 matching lines @@
 void NavigationControllerImpl::LoadIfNecessary() {
   if (!needs_reload_)
     return;
 
   // Calling Reload() results in ignoring state, and not loading.
   // Explicitly use NavigateToPendingEntry so that the renderer uses the
   // cached state.
   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 {
     // If there is something to reload, the successful reload will clear the
     // |needs_reload_| flag. Otherwise, just do it here.
     needs_reload_ = false;
   }
 }
 
 void NavigationControllerImpl::NotifyEntryChanged(
@@ skipping 27 matching lines @@
   // when the tab is being destroyed for shutdown, since it won't return to
   // NavigateToEntry in that case.)  http://crbug.com/347742.
   CHECK(!in_navigate_to_pending_entry_ || delegate_->IsBeingDestroyed());
 
   if (was_failure && pending_entry_) {
     failed_pending_entry_id_ = pending_entry_->GetUniqueID();
   } else {
     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) {
   if (pending_entry_)
     pending_entry_->set_ssl_error(error);
 }
 
 void NavigationControllerImpl::DiscardTransientEntry() {
   if (transient_entry_index_ == -1)
     return;
   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;
 }
 
 int NavigationControllerImpl::GetEntryIndexWithUniqueID(
     int nav_entry_id) const {
   for (int i = static_cast<int>(entries_.size()) - 1; i >= 0; --i) {
     if (entries_[i]->GetUniqueID() == nav_entry_id)
       return i;
   }
   return -1;
 }
 
 NavigationEntryImpl* NavigationControllerImpl::GetTransientEntry() const {
   if (transient_entry_index_ == -1)
     return NULL;
   return entries_[transient_entry_index_].get();
 }
 
 void NavigationControllerImpl::SetTransientEntry(
     std::unique_ptr<NavigationEntry> entry) {
   // Discard any current transient entry, we can only have one at a time.
   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)

[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);
 }
 
 void NavigationControllerImpl::InsertEntriesFrom(
     const NavigationControllerImpl& source,
     int max_index) {
   DCHECK_LE(max_index, source.GetEntryCount());
   size_t insert_index = 0;
   for (int i = 0; i < max_index; i++) {
     // When cloning a tab, copy all entries except interstitial pages.
     if (source.entries_[i]->GetPageType() != PAGE_TYPE_INTERSTITIAL) {
       // TODO(creis): Once we start sharing FrameNavigationEntries between
       // NavigationEntries, it will not be safe to share them with another tab.
       // Must have a version of Clone that recreates them.
       entries_.insert(entries_.begin() + insert_index++,
                       source.entries_[i]->Clone());
     }
   }
+  DCHECK(pending_entry_index_ == -1 ||
+         pending_entry_ == GetEntryAtIndex(pending_entry_index_));
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content