Handle correctly start and commit of non-latest navigations.

Notable changes:
 - didStartProvisionalNavigation: is no op if passed navigation is not the latest
    navigation (this way didStartProvisionalNavigation: does not mess up with
    pending item)
 - didCommitNavigation: does not commit pending entry if if passed navigation
    is not the latest navigation. Instead it changes last committed item to one
    that matches with URL, which may not be correct, but it's just a minor
    temporary inconsistency which does not have any security implications
    (last committed URL is still correct).

BUG=711465

Review-Url: https://codereview.chromium.org/2821173002
Cr-Commit-Position: refs/heads/master@{#465669}
(cherry picked from commit 79d9b86)

Review-Url: https://codereview.chromium.org/2832853002 .
Cr-Commit-Position: refs/branch-heads/3071@{nwjs#84}
Cr-Branched-From: a106f0a-refs/heads/master@{#464641}

ios/chrome/browser/web/BUILD.gn

@@ -293,6 +293,7 @@ source_set("eg_tests") {
     "//components/content_settings/core/browser",
     "//components/content_settings/core/common",
     "//components/strings",
+    "//components/version_info:version_info",
     "//ios/chrome/app/strings",
     "//ios/chrome/browser",
     "//ios/chrome/browser/browser_state",

ios/chrome/browser/web/visible_url_egtest.mm

@@ -7,6 +7,11 @@
 #include "base/memory/ptr_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/strings/sys_string_conversions.h"
+#include "base/strings/utf_string_conversions.h"
+#include "components/version_info/version_info.h"
+#include "ios/chrome/browser/chrome_url_constants.h"
+#import "ios/chrome/browser/ui/commands/generic_chrome_command.h"
+#include "ios/chrome/browser/ui/commands/ios_command_ids.h"
 #include "ios/chrome/browser/ui/ui_util.h"
 #import "ios/chrome/test/app/chrome_test_util.h"
 #import "ios/chrome/test/earl_grey/chrome_earl_grey.h"
@@ -498,6 +503,38 @@ - (void)testDoubleBackNavigation {
       assertWithMatcher:grey_notNil()];
 }
 
+// Tests that visible URL is always the same as last committed URL if user
+// issues 2 go forward commands to WebUI page (crbug.com/711465).
+- (void)testDoubleForwardNavigationToWebUIPage {
+  // Create 3rd entry in the history, to be able to go back twice.
+  GURL URL(kChromeUIVersionURL);
+  [ChromeEarlGrey loadURL:GURL(kChromeUIVersionURL)];
+
+  // Tap the back button twice in the toolbar and wait for URL 1 to load.
+  [[EarlGrey selectElementWithMatcher:chrome_test_util::BackButton()]
+      performAction:grey_tap()];
+  [[EarlGrey selectElementWithMatcher:chrome_test_util::BackButton()]
+      performAction:grey_tap()];
+  [[EarlGrey selectElementWithMatcher:WebViewContainingText(kTestPage1)]
+      assertWithMatcher:grey_notNil()];
+
+  // Quickly (using chrome command) navigate forward twice and wait for
+  // kChromeUIVersionURL to load.
+  base::scoped_nsobject<GenericChromeCommand> forwardCommand(
+      [[GenericChromeCommand alloc] initWithTag:IDC_FORWARD]);
+  chrome_test_util::RunCommandWithActiveViewController(forwardCommand);
+  chrome_test_util::RunCommandWithActiveViewController(forwardCommand);
+
+  const std::string version = version_info::GetVersionNumber();
+  [[EarlGrey selectElementWithMatcher:WebViewContainingText(version)]
+      assertWithMatcher:grey_notNil()];
+
+  // Make sure that kChromeUIVersionURL URL is displayed in the omnibox.
+  std::string expectedText = base::UTF16ToUTF8(web::GetDisplayTitleForUrl(URL));
+  [[EarlGrey selectElementWithMatcher:OmniboxText(expectedText)]
+      assertWithMatcher:grey_notNil()];
+}
+
 // Tests that visible URL is always the same as last committed URL if page calls
 // window.history.back() twice.
 - (void)testDoubleBackJSNavigation {

ios/web/navigation/crw_session_controller.h

@@ -134,7 +134,8 @@ struct Referrer;
 - (void)copyStateFromSessionControllerAndPrune:(CRWSessionController*)source;
 
 // Sets |lastCommittedItemIndex| to the |index| if it's in the entries bounds.
-- (void)goToItemAtIndex:(NSInteger)index;
+// Discards pending and transient entries if |discard| is YES.
+- (void)goToItemAtIndex:(NSInteger)index discardNonCommittedItems:(BOOL)discard;
 
 // Removes the item at |index| after discarding any noncomitted entries.
 // |index| must not be the index of the last committed item, or a noncomitted

ios/web/navigation/crw_session_controller.mm

@@ -541,21 +541,26 @@ - (void)copyStateFromSessionControllerAndPrune:(CRWSessionController*)source {
             self.items.size());
 }
 
-- (void)goToItemAtIndex:(NSInteger)index {
+- (void)goToItemAtIndex:(NSInteger)index
+    discardNonCommittedItems:(BOOL)discard {
   if (index < 0 || static_cast<NSUInteger>(index) >= self.items.size())
     return;
 
-  if (index < _lastCommittedItemIndex) {
-    // Going back.
-    [self discardNonCommittedItems];
-  } else if (_lastCommittedItemIndex < index) {
-    // Going forward.
-    [self discardTransientItem];
-  } else {
-    // |delta| is 0, no need to change the last committed item index.
+  if (index == _lastCommittedItemIndex) {
+    // |delta| is 0, no need to change current navigation index.
     return;
   }
 
+  if (discard) {
+    if (index < _lastCommittedItemIndex) {
+      // Going back.
+      [self discardNonCommittedItems];
+    } else if (_lastCommittedItemIndex < index) {
+      // Going forward.
+      [self discardTransientItem];
+    }
+  }
+
   _previousItemIndex = _lastCommittedItemIndex;
   _lastCommittedItemIndex = index;
 }

ios/web/navigation/crw_session_controller_unittest.mm

@@ -372,7 +372,7 @@ void SetUp() override {
   [session_controller_ commitPendingItem];
 
   // Go back to the first item.
-  [session_controller_ goToItemAtIndex:0];
+  [session_controller_ goToItemAtIndex:0 discardNonCommittedItems:NO];
 
   // Create and commit a new pending item.
   [session_controller_
@@ -416,7 +416,7 @@ void SetUp() override {
   ASSERT_EQ(3U, [session_controller_ items].size());
 
   // Go to the middle, and commit first pending item index.
-  [session_controller_ goToItemAtIndex:1];
+  [session_controller_ goToItemAtIndex:1 discardNonCommittedItems:NO];
   [session_controller_ setPendingItemIndex:0];
   ASSERT_EQ(0, [session_controller_ pendingItemIndex]);
   web::NavigationItem* pending_item = [session_controller_ pendingItem];
@@ -824,16 +824,16 @@ void SetUp() override {
 
   EXPECT_EQ(session_controller_.get().previousItemIndex, 1);
 
-  [session_controller_ goToItemAtIndex:1];
+  [session_controller_ goToItemAtIndex:1 discardNonCommittedItems:NO];
   EXPECT_EQ(session_controller_.get().previousItemIndex, 2);
 
-  [session_controller_ goToItemAtIndex:0];
+  [session_controller_ goToItemAtIndex:0 discardNonCommittedItems:NO];
   EXPECT_EQ(session_controller_.get().previousItemIndex, 1);
 
-  [session_controller_ goToItemAtIndex:1];
+  [session_controller_ goToItemAtIndex:1 discardNonCommittedItems:NO];
   EXPECT_EQ(session_controller_.get().previousItemIndex, 0);
 
-  [session_controller_ goToItemAtIndex:2];
+  [session_controller_ goToItemAtIndex:2 discardNonCommittedItems:NO];
   EXPECT_EQ(session_controller_.get().previousItemIndex, 1);
 }
 
@@ -999,11 +999,11 @@ void SetUp() override {
   EXPECT_TRUE([session_controller_ forwardItems].empty());
   EXPECT_EQ("http://www.example.com/redirect", backItems[0]->GetURL().spec());
 
-  [session_controller_ goToItemAtIndex:1];
+  [session_controller_ goToItemAtIndex:1 discardNonCommittedItems:NO];
   EXPECT_EQ(1U, [session_controller_ backwardItems].size());
   EXPECT_EQ(1U, [session_controller_ forwardItems].size());
 
-  [session_controller_ goToItemAtIndex:0];
+  [session_controller_ goToItemAtIndex:0 discardNonCommittedItems:NO];
   web::NavigationItemList forwardItems = [session_controller_ forwardItems];
   EXPECT_EQ(0U, [session_controller_ backwardItems].size());
   EXPECT_EQ(2U, forwardItems.size());
@@ -1052,8 +1052,20 @@ void SetUp() override {
   EXPECT_TRUE([session_controller_ pendingItem]);
   EXPECT_TRUE([session_controller_ transientItem]);
 
+  // Going back and forth without discaring transient and pending items.
+  [session_controller_ goToItemAtIndex:1 discardNonCommittedItems:NO];
+  EXPECT_EQ(1, session_controller_.get().lastCommittedItemIndex);
+  EXPECT_EQ(3, session_controller_.get().previousItemIndex);
+  EXPECT_TRUE(session_controller_.get().pendingItem);
+  EXPECT_TRUE(session_controller_.get().transientItem);
+  [session_controller_ goToItemAtIndex:3 discardNonCommittedItems:NO];
+  EXPECT_EQ(3, session_controller_.get().lastCommittedItemIndex);
+  EXPECT_EQ(1, session_controller_.get().previousItemIndex);
+  EXPECT_TRUE(session_controller_.get().pendingItem);
+  EXPECT_TRUE(session_controller_.get().transientItem);
+
   // Going back should discard transient and pending items.
-  [session_controller_ goToItemAtIndex:1];
+  [session_controller_ goToItemAtIndex:1 discardNonCommittedItems:YES];
   EXPECT_EQ(1, session_controller_.get().lastCommittedItemIndex);
   EXPECT_EQ(3, session_controller_.get().previousItemIndex);
   EXPECT_FALSE(session_controller_.get().pendingItem);
@@ -1062,21 +1074,22 @@ void SetUp() override {
   // Going forward should discard transient item.
   [session_controller_ addTransientItemWithURL:GURL("http://www.example.com")];
   EXPECT_TRUE(session_controller_.get().transientItem);
-  [session_controller_ goToItemAtIndex:2];
+  [session_controller_ goToItemAtIndex:2 discardNonCommittedItems:YES];
   EXPECT_EQ(2, session_controller_.get().lastCommittedItemIndex);
   EXPECT_EQ(1, session_controller_.get().previousItemIndex);
   EXPECT_FALSE(session_controller_.get().transientItem);
 
   // Out of bounds navigations should be no-op.
-  [session_controller_ goToItemAtIndex:-1];
+  [session_controller_ goToItemAtIndex:-1 discardNonCommittedItems:NO];
   EXPECT_EQ(2, session_controller_.get().lastCommittedItemIndex);
   EXPECT_EQ(1, session_controller_.get().previousItemIndex);
-  [session_controller_ goToItemAtIndex:NSIntegerMax];
+  [session_controller_ goToItemAtIndex:NSIntegerMax
+              discardNonCommittedItems:NO];
   EXPECT_EQ(2, session_controller_.get().lastCommittedItemIndex);
   EXPECT_EQ(1, session_controller_.get().previousItemIndex);
 
   // Going to current index should not change the previous index.
-  [session_controller_ goToItemAtIndex:2];
+  [session_controller_ goToItemAtIndex:2 discardNonCommittedItems:NO];
   EXPECT_EQ(2, session_controller_.get().lastCommittedItemIndex);
   EXPECT_EQ(1, session_controller_.get().previousItemIndex);
 }

ios/web/navigation/navigation_manager_impl_unittest.mm

@@ -176,15 +176,15 @@ TestNavigationManagerDelegate navigation_manager_delegate() {
   EXPECT_TRUE(navigation_manager()->CanGoBack());
   EXPECT_TRUE(navigation_manager()->CanGoToOffset(-1));
 
-  [session_controller() goToItemAtIndex:1];
+  [session_controller() goToItemAtIndex:1 discardNonCommittedItems:NO];
   EXPECT_TRUE(navigation_manager()->CanGoBack());
   EXPECT_TRUE(navigation_manager()->CanGoToOffset(-1));
 
-  [session_controller() goToItemAtIndex:0];
+  [session_controller() goToItemAtIndex:0 discardNonCommittedItems:NO];
   EXPECT_FALSE(navigation_manager()->CanGoBack());
   EXPECT_FALSE(navigation_manager()->CanGoToOffset(-1));
 
-  [session_controller() goToItemAtIndex:1];
+  [session_controller() goToItemAtIndex:1 discardNonCommittedItems:NO];
   EXPECT_TRUE(navigation_manager()->CanGoBack());
   EXPECT_TRUE(navigation_manager()->CanGoToOffset(-1));
 }
@@ -230,19 +230,19 @@ TestNavigationManagerDelegate navigation_manager_delegate() {
   EXPECT_FALSE(navigation_manager()->CanGoForward());
   EXPECT_FALSE(navigation_manager()->CanGoToOffset(1));
 
-  [session_controller() goToItemAtIndex:1];
+  [session_controller() goToItemAtIndex:1 discardNonCommittedItems:NO];
   EXPECT_TRUE(navigation_manager()->CanGoForward());
   EXPECT_TRUE(navigation_manager()->CanGoToOffset(1));
 
-  [session_controller() goToItemAtIndex:0];
+  [session_controller() goToItemAtIndex:0 discardNonCommittedItems:NO];
   EXPECT_TRUE(navigation_manager()->CanGoForward());
   EXPECT_TRUE(navigation_manager()->CanGoToOffset(1));
 
-  [session_controller() goToItemAtIndex:1];
+  [session_controller() goToItemAtIndex:1 discardNonCommittedItems:NO];
   EXPECT_TRUE(navigation_manager()->CanGoForward());
   EXPECT_TRUE(navigation_manager()->CanGoToOffset(1));
 
-  [session_controller() goToItemAtIndex:2];
+  [session_controller() goToItemAtIndex:2 discardNonCommittedItems:NO];
   EXPECT_FALSE(navigation_manager()->CanGoForward());
   EXPECT_FALSE(navigation_manager()->CanGoToOffset(1));
 }
@@ -282,7 +282,7 @@ TestNavigationManagerDelegate navigation_manager_delegate() {
   ASSERT_EQ(4, navigation_manager()->GetLastCommittedItemIndex());
 
   // Go to entry at index 1 and test API from that state.
-  [session_controller() goToItemAtIndex:1];
+  [session_controller() goToItemAtIndex:1 discardNonCommittedItems:NO];
   ASSERT_EQ(1, navigation_manager()->GetLastCommittedItemIndex());
   ASSERT_EQ(-1, navigation_manager()->GetPendingItemIndex());
   EXPECT_FALSE(navigation_manager()->CanGoToOffset(-1));
@@ -306,7 +306,7 @@ TestNavigationManagerDelegate navigation_manager_delegate() {
   EXPECT_EQ(1000000002, navigation_manager()->GetIndexForOffset(1000000000));
 
   // Go to entry at index 2 and test API from that state.
-  [session_controller() goToItemAtIndex:2];
+  [session_controller() goToItemAtIndex:2 discardNonCommittedItems:NO];
   ASSERT_EQ(2, navigation_manager()->GetLastCommittedItemIndex());
   ASSERT_EQ(-1, navigation_manager()->GetPendingItemIndex());
   EXPECT_TRUE(navigation_manager()->CanGoToOffset(-1));
@@ -328,7 +328,7 @@ TestNavigationManagerDelegate navigation_manager_delegate() {
   EXPECT_EQ(1000000003, navigation_manager()->GetIndexForOffset(1000000000));
 
   // Go to entry at index 4 and test API from that state.
-  [session_controller() goToItemAtIndex:4];
+  [session_controller() goToItemAtIndex:4 discardNonCommittedItems:NO];
   ASSERT_EQ(4, navigation_manager()->GetLastCommittedItemIndex());
   ASSERT_EQ(-1, navigation_manager()->GetPendingItemIndex());
   EXPECT_TRUE(navigation_manager()->CanGoToOffset(-1));
@@ -424,7 +424,7 @@ TestNavigationManagerDelegate navigation_manager_delegate() {
   EXPECT_EQ(1000000003, navigation_manager()->GetIndexForOffset(1000000000));
 
   // Set pending index to 4 and committed entry to 1 and test.
-  [session_controller() goToItemAtIndex:1];
+  [session_controller() goToItemAtIndex:1 discardNonCommittedItems:NO];
   [session_controller() setPendingItemIndex:4];
   ASSERT_EQ(1, navigation_manager()->GetLastCommittedItemIndex());
   ASSERT_EQ(4, navigation_manager()->GetPendingItemIndex());
@@ -447,7 +447,7 @@ TestNavigationManagerDelegate navigation_manager_delegate() {
   EXPECT_EQ(1000000004, navigation_manager()->GetIndexForOffset(1000000000));
 
   // Test with existing transient entry in the end of the stack.
-  [session_controller() goToItemAtIndex:4];
+  [session_controller() goToItemAtIndex:4 discardNonCommittedItems:NO];
   [session_controller() setPendingItemIndex:-1];
   [session_controller() addTransientItemWithURL:GURL("http://www.url.com")];
   ASSERT_EQ(5, navigation_manager()->GetItemCount());
@@ -506,7 +506,7 @@ TestNavigationManagerDelegate navigation_manager_delegate() {
 
   // Now go forward to that middle transient item (pending index is 1,
   // current index is 0).
-  [session_controller() goToItemAtIndex:0];
+  [session_controller() goToItemAtIndex:0 discardNonCommittedItems:NO];
   [session_controller() setPendingItemIndex:1];
   ASSERT_EQ(3, navigation_manager()->GetItemCount());
   ASSERT_EQ(0, navigation_manager()->GetLastCommittedItemIndex());
@@ -1159,7 +1159,7 @@ TestNavigationManagerDelegate navigation_manager_delegate() {
       web::NavigationManager::UserAgentOverrideOption::INHERIT);
   [session_controller() commitPendingItem];
 
-  [session_controller() goToItemAtIndex:1];
+  [session_controller() goToItemAtIndex:1 discardNonCommittedItems:NO];
   EXPECT_EQ(1, navigation_manager()->GetLastCommittedItemIndex());
 
   navigation_manager()->Reload(web::ReloadType::NORMAL,
@@ -1287,7 +1287,7 @@ TestNavigationManagerDelegate navigation_manager_delegate() {
       web::NavigationManager::UserAgentOverrideOption::INHERIT);
   [session_controller() commitPendingItem];
 
-  [session_controller() goToItemAtIndex:1];
+  [session_controller() goToItemAtIndex:1 discardNonCommittedItems:NO];
   EXPECT_EQ(1, navigation_manager()->GetLastCommittedItemIndex());
 
   navigation_manager()->Reload(web::ReloadType::ORIGINAL_REQUEST_URL,