Fix crash after navigation deletion

When tabs are serialized by tab_restore_service, only a limited number
of entries is written to disk. On restore, these entries are created
with their previous index, which is not consistent with actual index.
This CL fixes the indices and adds checks to more easily identify
other cases with inconsistent indices.

Bug: 820910, 821214
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I1fe8f3809cb51a0b5853ccd9294690bc08d71778
Reviewed-on: https://chromium-review.googlesource.com/960026Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#543038}

chrome/browser/sessions/persistent_tab_restore_service_unittest.cc

@@ -303,6 +303,33 @@ TEST_F(PersistentTabRestoreServiceTest, Restore) {
             tab->timestamp.ToInternalValue());
 }
 
+// Tests restoring a tab with more than gMaxPersistNavigationCount entries.
+TEST_F(PersistentTabRestoreServiceTest, RestoreManyNavigations) {
+  AddThreeNavigations();
+  AddThreeNavigations();
+  AddThreeNavigations();
+
+  // Have the service record the tab.
+  service_->CreateHistoricalTab(live_tab(), -1);
+
+  // Recreate the service and have it load the tabs.
+  RecreateService();
+
+  // One entry should be created.
+  ASSERT_EQ(1U, service_->entries().size());
+
+  // And verify the entry.
+  Entry* entry = service_->entries().front().get();
+  ASSERT_EQ(sessions::TabRestoreService::TAB, entry->type);
+  Tab* tab = static_cast<Tab*>(entry);
+  // Only gMaxPersistNavigationCount + 1 (current navigation) are persisted.
+  ASSERT_EQ(7U, tab->navigations.size());
+  // Check that they are created with correct indices.
+  EXPECT_EQ(0, tab->navigations[0].index());
+  EXPECT_EQ(6, tab->navigations[6].index());
+  EXPECT_EQ(6, tab->current_navigation_index);
+}
+
 // Tests restoring a single pinned tab.
 TEST_F(PersistentTabRestoreServiceTest, RestorePinnedAndApp) {
   AddThreeNavigations();

components/sessions/core/persistent_tab_restore_service.cc

@@ -914,6 +914,10 @@ void PersistentTabRestoreService::Delegate::CreateEntriesFromCommands(
                                                &tab_id)) {
           return;
         }
+        // When navigations are serialized, only gMaxPersistNavigationCount
+        // navigations are written. This leads to inconsistent indices.
+        current_tab->navigations.back().set_index(
+            current_tab->navigations.size() - 1);
         break;
       }
 

components/sessions/core/tab_restore_service_helper.cc

@@ -144,6 +144,10 @@ void TabRestoreServiceHelper::ClearEntries() {
 
 bool TabRestoreServiceHelper::DeleteFromTab(const DeletionPredicate& predicate,
                                             Tab* tab) {
+  // TODO(dullweber): Change to DCHECK() when this is tested to be true.
+  CHECK(ValidateTab(*tab));
+  CHECK_EQ(tab->current_navigation_index,
+           tab->navigations[tab->current_navigation_index].index());
   std::vector<SerializedNavigationEntry> new_navigations;
   int deleted_navigations = 0;
   for (auto& navigation : tab->navigations) {
@@ -161,7 +165,8 @@ bool TabRestoreServiceHelper::DeleteFromTab(const DeletionPredicate& predicate,
     }
   }
   tab->navigations = std::move(new_navigations);
-  DCHECK(tab->navigations.empty() || ValidateTab(*tab));
+  // TODO(dullweber): Change to DCHECK() when this is tested to be true.
+  CHECK(tab->navigations.empty() || ValidateTab(*tab));
   return tab->navigations.empty();
 }