Fix crash in InterceptNavigationDelegateImpl#maybeUpdateNavigationHistory

Note that this is a speculative fix as I was unable to repro, but see
the bug for context - I'm pretty certain this will fix the crash.

This introduces NavigationController::RemoveForwardEntries which will
attempt to remove any forward navigation entries (without crashing :)).

Bug: 963783
Change-Id: Ic375e0e4fd89d4ded3a5229bf82ccb932e10ab5f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1775070
Commit-Queue: Michael Thiessen <mthiesse@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699046}

chrome/android/java/src/org/chromium/chrome/browser/tab/InterceptNavigationDelegateImpl.java

@@ -239,14 +239,7 @@ public class InterceptNavigationDelegateImpl implements InterceptNavigationDeleg
     public void maybeUpdateNavigationHistory() {
         WebContents webContents = mTab.getWebContents();
         if (mClearAllForwardHistoryRequired && webContents != null) {
-            NavigationController navigationController =
-                    webContents.getNavigationController();
-            int lastCommittedEntryIndex = getLastCommittedEntryIndex();
-            while (navigationController.canGoForward()) {
-                boolean ret = navigationController.removeEntryAtIndex(
-                        lastCommittedEntryIndex + 1);
-                assert ret;
-            }
+            webContents.getNavigationController().pruneForwardEntries();
         } else if (mShouldClearRedirectHistoryForTabClobbering
                 && webContents != null) {
             // http://crbug/479056: Even if we clobber the current tab, we want to remove

content/browser/frame_host/navigation_controller_android.cc

@@ -425,6 +425,12 @@ jboolean NavigationControllerAndroid::RemoveEntryAtIndex(
   return navigation_controller_->RemoveEntryAtIndex(index);
 }
 
+void NavigationControllerAndroid::PruneForwardEntries(
+    JNIEnv* env,
+    const JavaParamRef<jobject>& obj) {
+  return navigation_controller_->PruneForwardEntries();
+}
+
 ScopedJavaLocalRef<jstring> NavigationControllerAndroid::GetEntryExtraData(
     JNIEnv* env,
     const JavaParamRef<jobject>& obj,

content/browser/frame_host/navigation_controller_android.h

@@ -119,6 +119,8 @@ class CONTENT_EXPORT NavigationControllerAndroid {
   jboolean RemoveEntryAtIndex(JNIEnv* env,
                               const base::android::JavaParamRef<jobject>& obj,
                               jint index);
+  void PruneForwardEntries(JNIEnv* env,
+                           const base::android::JavaParamRef<jobject>& obj);
   base::android::ScopedJavaLocalRef<jstring> GetEntryExtraData(
       JNIEnv* env,
       const base::android::JavaParamRef<jobject>& obj,

content/browser/frame_host/navigation_controller_impl.cc

@@ -887,6 +887,17 @@ bool NavigationControllerImpl::RemoveEntryAtIndex(int index) {
   return true;
 }
 
+void NavigationControllerImpl::PruneForwardEntries() {
+  DiscardNonCommittedEntries();
+  int remove_start_index = last_committed_entry_index_ + 1;
+  int num_removed = int(entries_.size()) - remove_start_index;
+  if (num_removed <= 0)
+    return;
+  entries_.erase(entries_.begin() + remove_start_index, entries_.end());
+  NotifyPrunedEntries(this, remove_start_index /* start index */,
+                      num_removed /* count */);
+}
+
 void NavigationControllerImpl::UpdateVirtualURLToURL(
     NavigationEntryImpl* entry, const GURL& new_url) {
   GURL new_virtual_url(new_url);
@@ -2431,10 +2442,8 @@ void NavigationControllerImpl::InsertOrReplaceEntry(
 
   DiscardNonCommittedEntries();
 
-  int current_size = static_cast<int>(entries_.size());
-
   // When replacing, don't prune the forward history.
-  if (replace && current_size > 0) {
+  if (replace && entries_.size() > 0) {
     CopyReplacedNavigationEntryDataIfPreviouslyEmpty(
         entries_[last_committed_entry_index_].get(), entry.get());
     entries_[last_committed_entry_index_] = std::move(entry);
@@ -2444,20 +2453,7 @@ void NavigationControllerImpl::InsertOrReplaceEntry(
   // We shouldn't see replace == true when there's no committed entries.
   DCHECK(!replace);
 
-  if (current_size > 0) {
-    // Prune any entries which are in front of the current entry.
-    int num_pruned = 0;
-    while (last_committed_entry_index_ < (current_size - 1)) {
-      num_pruned++;
-      entries_.pop_back();
-      current_size--;
-    }
-    if (num_pruned > 0) {  // Only notify if we did prune something.
-      NotifyPrunedEntries(this,
-                          last_committed_entry_index_ + 1 /* start index */,
-                          num_pruned /* count */);
-    }
-  }
+  PruneForwardEntries();
 
   PruneOldestSkippableEntryIfFull();
 
@@ -3364,6 +3360,9 @@ void NavigationControllerImpl::FinishRestore(int selected_index,
 }
 
 void NavigationControllerImpl::DiscardNonCommittedEntries() {
+  // Avoid sending a notification if there is nothing to discard.
+  if (!pending_entry_ && transient_entry_index_ == -1)
+    return;
   DiscardPendingEntry(false);
   DiscardTransientEntry();
   if (delegate_)

content/browser/frame_host/navigation_controller_impl.h

@@ -80,6 +80,7 @@ class CONTENT_EXPORT NavigationControllerImpl : public NavigationController {
   void GoToIndex(int index) override;
   void GoToOffset(int offset) override;
   bool RemoveEntryAtIndex(int index) override;
+  void PruneForwardEntries() override;
   const SessionStorageNamespaceMap& GetSessionStorageNamespaceMap() override;
   SessionStorageNamespace* GetDefaultSessionStorageNamespace() override;
   bool NeedsReload() override;

content/browser/frame_host/navigation_controller_impl_unittest.cc

@@ -1071,7 +1071,7 @@ TEST_F(NavigationControllerTest, LoadURL_IgnorePreemptsPending) {
   EXPECT_EQ(-1, controller.GetPendingEntryIndex());
   EXPECT_TRUE(controller.GetPendingEntry());
   EXPECT_EQ(-1, controller.GetLastCommittedEntryIndex());
-  EXPECT_EQ(2, delegate->navigation_state_change_count());
+  EXPECT_EQ(1, delegate->navigation_state_change_count());
 
   // Certain rare cases can make a direct DidCommitProvisionalLoad call without
   // going to the browser. Renderer reload of an about:blank is such a case.
@@ -1082,7 +1082,7 @@ TEST_F(NavigationControllerTest, LoadURL_IgnorePreemptsPending) {
   EXPECT_EQ(-1, controller.GetPendingEntryIndex());
   EXPECT_FALSE(controller.GetPendingEntry());
   EXPECT_EQ(-1, controller.GetLastCommittedEntryIndex());
-  EXPECT_EQ(3, delegate->navigation_state_change_count());
+  EXPECT_EQ(2, delegate->navigation_state_change_count());
 
   contents()->SetDelegate(nullptr);
 }
@@ -1109,7 +1109,7 @@ TEST_F(NavigationControllerTest, LoadURL_AbortDoesntCancelPending) {
   EXPECT_EQ(-1, controller.GetPendingEntryIndex());
   EXPECT_TRUE(controller.GetPendingEntry());
   EXPECT_EQ(-1, controller.GetLastCommittedEntryIndex());
-  EXPECT_EQ(2, delegate->navigation_state_change_count());
+  EXPECT_EQ(1, delegate->navigation_state_change_count());
 
   // It may abort before committing, if it's a download or due to a stop or
   // a new navigation from the user.
@@ -1121,7 +1121,7 @@ TEST_F(NavigationControllerTest, LoadURL_AbortDoesntCancelPending) {
   EXPECT_EQ(-1, controller.GetPendingEntryIndex());
   EXPECT_TRUE(controller.GetPendingEntry());
   EXPECT_EQ(-1, controller.GetLastCommittedEntryIndex());
-  EXPECT_EQ(2, delegate->navigation_state_change_count());
+  EXPECT_EQ(1, delegate->navigation_state_change_count());
   NavigationEntry* pending_entry = controller.GetPendingEntry();
 
   // Ensure that a reload keeps the same pending entry.
@@ -1162,9 +1162,9 @@ TEST_F(NavigationControllerTest, LoadURL_RedirectAbortDoesntShowPendingURL) {
   EXPECT_EQ(-1, controller.GetPendingEntryIndex());
   EXPECT_TRUE(controller.GetPendingEntry());
   EXPECT_EQ(0, controller.GetLastCommittedEntryIndex());
-  // The delegate should have been notified at least twice: once for the loading
-  // state change, and once for the url change.
-  EXPECT_EQ(3, delegate->navigation_state_change_count());
+  // The delegate should have been notified twice: once for the loading state
+  // change, and once for the url change.
+  EXPECT_EQ(2, delegate->navigation_state_change_count());
 
   // The visible entry should be the last committed URL, not the pending one.
   EXPECT_EQ(kExistingURL, controller.GetVisibleEntry()->GetURL());
@@ -1188,7 +1188,7 @@ TEST_F(NavigationControllerTest, LoadURL_RedirectAbortDoesntShowPendingURL) {
   EXPECT_EQ(0, controller.GetLastCommittedEntryIndex());
   // The delegate should have been notified twice: once for the loading state
   // change, and once for the url change.
-  EXPECT_EQ(5, delegate->navigation_state_change_count());
+  EXPECT_EQ(4, delegate->navigation_state_change_count());
 
   // The visible entry should be the last committed URL, not the pending one,
   // so that no spoof is possible.
@@ -4838,4 +4838,106 @@ TEST_F(NavigationControllerTest, NoURLRewriteForSubframes) {
   BrowserURLHandlerImpl::GetInstance()->SetFixupHandlerForTesting(nullptr);
 }
 
+// Tests that calling RemoveForwareEntries() clears all forward entries
+// including non-committed entries.
+TEST_F(NavigationControllerTest, PruneForwardEntries) {
+  NavigationControllerImpl& controller = controller_impl();
+  const GURL url_0("http://foo/0");
+  const GURL url_1("http://foo/1");
+  const GURL url_2("http://foo/2");
+  const GURL url_3("http://foo/3");
+  const GURL url_transient("http://foo/transient");
+
+  NavigateAndCommit(url_0);
+  NavigateAndCommit(url_1);
+  NavigateAndCommit(url_2);
+  NavigateAndCommit(url_3);
+
+  // Set a WebContentsDelegate to listen for state changes.
+  std::unique_ptr<TestWebContentsDelegate> delegate(
+      new TestWebContentsDelegate());
+  EXPECT_FALSE(contents()->GetDelegate());
+  contents()->SetDelegate(delegate.get());
+
+  controller.GoBack();
+
+  // Ensure that non-committed entries are removed even if there are no forward
+  // entries.
+  EXPECT_EQ(4, controller.GetEntryCount());
+  EXPECT_EQ(2, controller.GetPendingEntryIndex());
+  EXPECT_EQ(2, controller.GetCurrentEntryIndex());
+  EXPECT_EQ(3, controller.GetLastCommittedEntryIndex());
+  int state_change_count = delegate->navigation_state_change_count();
+  controller.PruneForwardEntries();
+  EXPECT_EQ(4, controller.GetEntryCount());
+  EXPECT_EQ(-1, controller.GetPendingEntryIndex());
+  EXPECT_EQ(3, controller.GetCurrentEntryIndex());
+  EXPECT_EQ(3, controller.GetLastCommittedEntryIndex());
+  EXPECT_EQ(0U, navigation_list_pruned_counter_);
+  EXPECT_EQ(state_change_count + 1, delegate->navigation_state_change_count());
+
+  controller.GoBack();
+  contents()->CommitPendingNavigation();
+  controller.GoBack();
+  contents()->CommitPendingNavigation();
+  controller.GoBack();
+  contents()->CommitPendingNavigation();
+  controller.GoForward();
+
+  EXPECT_EQ(1, controller.GetPendingEntryIndex());
+  EXPECT_EQ(0, controller.GetLastCommittedEntryIndex());
+  // Insert a transient entry before the pending one.
+  std::unique_ptr<NavigationEntry> transient_entry(new NavigationEntryImpl);
+  transient_entry->SetURL(url_transient);
+  controller.SetTransientEntry(std::move(transient_entry));
+
+  state_change_count = delegate->navigation_state_change_count();
+  controller.PruneForwardEntries();
+
+  EXPECT_EQ(1, controller.GetEntryCount());
+  EXPECT_FALSE(controller.CanGoForward());
+  EXPECT_FALSE(controller.CanGoBack());
+  EXPECT_EQ(0, controller.GetLastCommittedEntryIndex());
+  EXPECT_EQ(0, controller.GetCurrentEntryIndex());
+  EXPECT_EQ(-1, controller.GetPendingEntryIndex());
+  EXPECT_EQ(nullptr, controller.GetPendingEntry());
+  EXPECT_EQ(nullptr, controller.GetTransientEntry());
+  EXPECT_EQ(url_0, controller.GetVisibleEntry()->GetURL());
+  EXPECT_EQ(1U, navigation_list_pruned_counter_);
+  EXPECT_EQ(1, last_navigation_entry_pruned_details_.index);
+  EXPECT_EQ(3, last_navigation_entry_pruned_details_.count);
+  EXPECT_EQ(state_change_count + 1, delegate->navigation_state_change_count());
+}
+
+// Make sure that cloning a WebContentsImpl and clearing forward entries
+// before the first commit doesn't clear all entries.
+TEST_F(NavigationControllerTest, PruneForwardEntriesAfterClone) {
+  NavigationControllerImpl& controller = controller_impl();
+  const GURL url1("http://foo1");
+  const GURL url2("http://foo2");
+
+  NavigateAndCommit(url1);
+  NavigateAndCommit(url2);
+
+  std::unique_ptr<WebContents> clone(controller.GetWebContents()->Clone());
+  clone->GetController().LoadIfNecessary();
+
+  // Set a WebContentsDelegate to listen for state changes after the clone call
+  // to only count state changes from the PruneForwardEntries call.
+  std::unique_ptr<TestWebContentsDelegate> delegate(
+      new TestWebContentsDelegate());
+  EXPECT_FALSE(clone->GetDelegate());
+  clone->SetDelegate(delegate.get());
+
+  EXPECT_EQ(1, clone->GetController().GetPendingEntryIndex());
+
+  clone->GetController().PruneForwardEntries();
+
+  ASSERT_EQ(2, clone->GetController().GetEntryCount());
+  EXPECT_EQ(-1, clone->GetController().GetPendingEntryIndex());
+  EXPECT_EQ(url2, clone->GetController().GetVisibleEntry()->GetURL());
+  EXPECT_EQ(0U, navigation_list_pruned_counter_);
+  EXPECT_EQ(1, delegate->navigation_state_change_count());
+}
+
 }  // namespace content

content/public/android/java/src/org/chromium/content/browser/framehost/NavigationControllerImpl.java

@@ -271,6 +271,13 @@ import org.chromium.content_public.common.ResourceRequestBody;
         return false;
     }
 
+    @Override
+    public void pruneForwardEntries() {
+        if (mNativeNavigationControllerAndroid == 0) return;
+        NavigationControllerImplJni.get().pruneForwardEntries(
+                mNativeNavigationControllerAndroid, NavigationControllerImpl.this);
+    }
+
     @Override
     public String getEntryExtraData(int index, String key) {
         if (mNativeNavigationControllerAndroid == 0) return null;
@@ -362,6 +369,8 @@ import org.chromium.content_public.common.ResourceRequestBody;
                 long nativeNavigationControllerAndroid, NavigationControllerImpl caller);
         boolean removeEntryAtIndex(
                 long nativeNavigationControllerAndroid, NavigationControllerImpl caller, int index);
+        void pruneForwardEntries(
+                long nativeNavigationControllerAndroid, NavigationControllerImpl caller);
         String getEntryExtraData(long nativeNavigationControllerAndroid,
                 NavigationControllerImpl caller, int index, String key);
         void setEntryExtraData(long nativeNavigationControllerAndroid,

content/public/android/java/src/org/chromium/content_public/browser/NavigationController.java

@@ -172,6 +172,12 @@ public interface NavigationController {
      */
     public boolean removeEntryAtIndex(int index);
 
+    /**
+     * Discards any transient or pending entries, then discards all entries after the current entry
+     * index.
+     */
+    void pruneForwardEntries();
+
     /**
      * Gets extra data on the {@link NavigationEntry} at {@code index}.
      * @param index The index of the navigation entry.

content/public/browser/navigation_controller.h

@@ -414,6 +414,10 @@ class NavigationController {
   // Otherwise this call discards any transient or pending entries.
   virtual bool RemoveEntryAtIndex(int index) = 0;
 
+  // Discards any transient or pending entries, then discards all entries after
+  // the current entry index.
+  virtual void PruneForwardEntries() = 0;
+
   // Random --------------------------------------------------------------------
 
   // Session storage depends on dom_storage that depends on blink::WebString.

content/public/test/android/javatests/src/org/chromium/content_public/browser/test/mock/MockNavigationController.java

@@ -120,6 +120,9 @@ public class MockNavigationController implements NavigationController {
         return false;
     }
 
+    @Override
+    public void pruneForwardEntries() {}
+
     @Override
     public String getEntryExtraData(int index, String key) {
         return null;