[ios] Prevent session storage from saving invalid last committed.

It's possible for sessions to be serialized while navigating to a
SlimNav restore URL, leading to a brief moment where the wrong
last committed index is serialized. This leads to a DCHECK on restore.

Bug: 1003049
Change-Id: I6181609a2e91370c2e3eecdb6e63e24801513d68
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1797244
Commit-Queue: Justin Cohen <justincohen@chromium.org>
Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Reviewed-by: Ali Juma <ajuma@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Auto-Submit: Justin Cohen <justincohen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#696051}

ios/web/navigation/session_storage_builder.mm

@@ -34,18 +34,24 @@ CRWSessionStorage* SessionStorageBuilder::BuildStorage(
   session_storage.hasOpener = web_state->HasOpener();
   session_storage.lastCommittedItemIndex =
       navigation_manager->GetLastCommittedItemIndex();
+  if (session_storage.lastCommittedItemIndex == -1) {
+    // This can happen when a session is saved during restoration. Instead,
+    // default to GetItemCount() - 1.
+    session_storage.lastCommittedItemIndex =
+        navigation_manager->GetItemCount() - 1;
+  }
   session_storage.previousItemIndex =
       navigation_manager->GetPreviousItemIndex();
   NSMutableArray* item_storages = [[NSMutableArray alloc] init];
   NavigationItemStorageBuilder item_storage_builder;
+  size_t originalIndex = session_storage.lastCommittedItemIndex;
   for (size_t index = 0;
        index < static_cast<size_t>(navigation_manager->GetItemCount());
        ++index) {
     web::NavigationItemImpl* item =
         navigation_manager->GetNavigationItemImplAtIndex(index);
     if (item->ShouldSkipSerialization()) {
-      if (index <= static_cast<size_t>(
-                       navigation_manager->GetLastCommittedItemIndex())) {
+      if (index <= originalIndex) {
         session_storage.lastCommittedItemIndex--;
       }
       continue;

ios/web/web_state/web_state_impl_unittest.mm

@@ -58,6 +58,8 @@ using testing::Assign;
 using testing::AtMost;
 using testing::DoAll;
 using testing::Return;
+using base::test::ios::WaitUntilConditionOrTimeout;
+using base::test::ios::kWaitForPageLoadTimeout;
 
 namespace web {
 namespace {
@@ -883,6 +885,8 @@ TEST_P(WebStateImplTest, UncommittedRestoreSession) {
   EXPECT_EQ(@(1), user_data_value);
 }
 
+// Test that lastCommittedItemIndex is end-of-list when there's no defined
+// index, such as during a restore.
 TEST_P(WebStateImplTest, NoUncommittedRestoreSession) {
   base::test::ScopedFeatureList scoped_feature_list;
   scoped_feature_list.InitAndEnableFeature(
@@ -895,6 +899,51 @@ TEST_P(WebStateImplTest, NoUncommittedRestoreSession) {
   EXPECT_EQ(GURL::EmptyGURL(), web_state_->GetVisibleURL());
 }
 
+TEST_P(WebStateImplTest, BuildStorageDuringRestore) {
+  if (GetParam() == NavigationManagerChoice::LEGACY) {
+    return;
+  }
+
+  GURL urls[3] = {GURL("https://chromium.test/1"),
+                  GURL("https://chromium.test/2"),
+                  GURL("https://chromium.test/3")};
+  std::vector<std::unique_ptr<NavigationItem>> items;
+  for (size_t index = 0; index < base::size(urls); ++index) {
+    items.push_back(NavigationItem::Create());
+    items.back()->SetURL(urls[index]);
+  }
+
+  // Force generation of child views; necessary for some tests.
+  web_state_->GetView();
+  web_state_->SetKeepRenderProcessAlive(true);
+
+  web_state_->GetNavigationManager()->Restore(0, std::move(items));
+  __block bool restore_done = false;
+  web_state_->GetNavigationManager()->AddRestoreCompletionCallback(
+      base::BindOnce(^{
+        restore_done = true;
+      }));
+  ASSERT_TRUE(WaitUntilConditionOrTimeout(kWaitForPageLoadTimeout, ^{
+    return restore_done;
+  }));
+  // Trying to grab the lastCommittedItemIndex while a restore is happening is
+  // undefined, so the last committed item defaults to end-of-list.
+  CRWSessionStorage* session_storage = web_state_->BuildSessionStorage();
+  EXPECT_EQ(2, session_storage.lastCommittedItemIndex);
+
+  // Now wait until the last committed item is fully loaded, and
+  // lastCommittedItemIndex goes back to 0.
+  EXPECT_TRUE(WaitUntilConditionOrTimeout(kWaitForPageLoadTimeout, ^{
+    EXPECT_FALSE(
+        wk_navigation_util::IsWKInternalUrl(web_state_->GetVisibleURL()));
+
+    return !web_state_->GetNavigationManager()->GetPendingItem() &&
+           !web_state_->IsLoading() && web_state_->GetLoadingProgress() == 1.0;
+  }));
+  session_storage = web_state_->BuildSessionStorage();
+  EXPECT_EQ(0, session_storage.lastCommittedItemIndex);
+}
+
 // Tests showing and clearing interstitial when NavigationManager is
 // empty.
 TEST_P(WebStateImplTest, ShowAndClearInterstitialWithNoCommittedItems) {