Invalidate the URL systematically when DiscardNonCommittedEntries()

The NavigationController was not invalidating the URL when a pending
entry was removed.

To fix this, be more systematic, more stupid. Always invalidate the URL
when DiscardNonCommittedEntries() is called.

Bug: 998284.
Change-Id: I01f1d16bcb25fa827bf68a52db4de531429a8564
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1781434
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Tao Bai <michaelbai@chromium.org>
Reviewed-by: Tommy Li <tommycli@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#697145}

android_webview/javatests/src/org/chromium/android_webview/test/PopupWindowTest.java

@@ -244,7 +244,7 @@ public class PopupWindowTest {
         // additional onPageFinished() callback. Also, this eventually affects commit stage of the
         // navigation which creates additional navigationStateChanged() and one additional
         // onPageFinished() callback.
-        onPageFinishedHelper.waitForCallback(onPageFinishedCount, 3);
+        onPageFinishedHelper.waitForCallback(onPageFinishedCount, 4);
         // This is the URL that gets shown to the user because parent changed DOM of the popup
         // window.
         List<String> urlList = onPageFinishedHelper.getUrlList();
@@ -253,8 +253,8 @@ public class PopupWindowTest {
         // that we wanted. The loaded page does not have the changed DOM. This is slightly different
         // from the original workflow in b/19325392 as there is no good hook to stop navigation and
         // trigger DidAccessInitialDocument at the same time.
-        Assert.assertTrue(urlList.get(onPageFinishedCount + 1).endsWith(popupPath));
         Assert.assertTrue(urlList.get(onPageFinishedCount + 2).endsWith(popupPath));
+        Assert.assertTrue(urlList.get(onPageFinishedCount + 3).endsWith(popupPath));
     }
 
     @Test

content/browser/frame_host/navigation_controller_impl.cc

@@ -528,7 +528,10 @@ NavigationControllerImpl::NavigationControllerImpl(
 }
 
 NavigationControllerImpl::~NavigationControllerImpl() {
-  DiscardNonCommittedEntriesInternal();
+  // The NavigationControllerImpl might be called inside its delegate
+  // destructor. Calling it is not valid anymore.
+  delegate_ = nullptr;
+  DiscardNonCommittedEntries();
 }
 
 WebContents* NavigationControllerImpl::GetWebContents() {
@@ -592,7 +595,7 @@ void NavigationControllerImpl::Reload(ReloadType reload_type,
     // should also update the current_index.
     current_index = pending_entry_index_;
   } else {
-    DiscardNonCommittedEntriesInternal();
+    DiscardNonCommittedEntries();
     current_index = GetCurrentEntryIndex();
     if (current_index != -1) {
       entry = GetEntryAtIndex(current_index);
@@ -618,7 +621,7 @@ void NavigationControllerImpl::Reload(ReloadType reload_type,
     delegate_->ActivateAndShowRepostFormWarningDialog();
   } else {
     if (!IsInitialNavigation())
-      DiscardNonCommittedEntriesInternal();
+      DiscardNonCommittedEntries();
 
     pending_entry_ = entry;
     pending_entry_index_ = current_index;
@@ -662,7 +665,7 @@ NavigationControllerImpl::GetEntryWithUniqueID(int nav_entry_id) const {
 
 void NavigationControllerImpl::SetPendingEntry(
     std::unique_ptr<NavigationEntryImpl> entry) {
-  DiscardNonCommittedEntriesInternal();
+  DiscardNonCommittedEntries();
   pending_entry_ = entry.release();
   DCHECK_EQ(-1, pending_entry_index_);
   NotificationService::current()->Notify(
@@ -1088,10 +1091,8 @@ bool NavigationControllerImpl::RendererDidNavigate(
       // discard it and make sure it is removed from the URL bar.  After that,
       // there is nothing we can do with this navigation, so we just return to
       // the caller that nothing has happened.
-      if (pending_entry_) {
+      if (pending_entry_)
         DiscardNonCommittedEntries();
-        delegate_->NotifyNavigationStateChanged(INVALIDATE_TYPE_URL);
-      }
       return false;
     default:
       NOTREACHED();
@@ -1112,7 +1113,7 @@ bool NavigationControllerImpl::RendererDidNavigate(
   // TODO(pbos): Consider a CHECK here that verifies that the pending entry has
   // been cleared instead of protecting against it.
   if (!keep_pending_entry)
-    DiscardNonCommittedEntriesInternal();
+    DiscardNonCommittedEntries();
 
   // All committed entries should have nonempty content state so WebKit doesn't
   // get confused when we go back to them (see the function for details).
@@ -1470,7 +1471,7 @@ void NavigationControllerImpl::RendererDidNavigateToNewPage(
   // navigation. Now we know that the renderer has updated its state accordingly
   // and it is safe to also clear the browser side history.
   if (params.history_list_was_cleared) {
-    DiscardNonCommittedEntriesInternal();
+    DiscardNonCommittedEntries();
     entries_.clear();
     last_committed_entry_index_ = -1;
   }
@@ -1661,7 +1662,7 @@ void NavigationControllerImpl::RendererDidNavigateToExistingPage(
   // Note that we need to use the "internal" version since we don't want to
   // actually change any other state, just kill the pointer.
   if (!keep_pending_entry)
-    DiscardNonCommittedEntriesInternal();
+    DiscardNonCommittedEntries();
 
   // If a transient entry was removed, the indices might have changed, so we
   // have to query the entry index again.
@@ -1820,7 +1821,7 @@ bool NavigationControllerImpl::RendererDidNavigateAutoSubframe(
       // We only need to discard the pending entry in this history navigation
       // case.  For newly created subframes, there was no pending entry.
       last_committed_entry_index_ = entry_index;
-      DiscardNonCommittedEntriesInternal();
+      DiscardNonCommittedEntries();
 
       // History navigations should send a commit notification.
       send_commit_notification = true;
@@ -2399,17 +2400,6 @@ void NavigationControllerImpl::RemoveEntryAtIndexInternal(int index) {
     last_committed_entry_index_--;
 }
 
-void NavigationControllerImpl::DiscardNonCommittedEntries() {
-  bool transient = transient_entry_index_ != -1;
-  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() {
   // If there is no pending_entry_, there should be no pending_entry_index_.
   DCHECK(pending_entry_ || pending_entry_index_ == -1);
@@ -2444,7 +2434,7 @@ void NavigationControllerImpl::InsertOrReplaceEntry(
   if (pending_entry_ && pending_entry_index_ == -1)
     entry->set_unique_id(pending_entry_->GetUniqueID());
 
-  DiscardNonCommittedEntriesInternal();
+  DiscardNonCommittedEntries();
 
   int current_size = static_cast<int>(entries_.size());
 
@@ -3379,9 +3369,11 @@ void NavigationControllerImpl::FinishRestore(int selected_index,
   last_committed_entry_index_ = selected_index;
 }
 
-void NavigationControllerImpl::DiscardNonCommittedEntriesInternal() {
+void NavigationControllerImpl::DiscardNonCommittedEntries() {
   DiscardPendingEntry(false);
   DiscardTransientEntry();
+  if (delegate_)
+    delegate_->NotifyNavigationStateChanged(INVALIDATE_TYPE_ALL);
 }
 
 void NavigationControllerImpl::DiscardTransientEntry() {

content/browser/frame_host/navigation_controller_impl.h

@@ -444,9 +444,6 @@ class CONTENT_EXPORT NavigationControllerImpl : public NavigationController {
   // Removes the entry at |index|, as long as it is not the current entry.
   void RemoveEntryAtIndexInternal(int index);
 
-  // Discards both the pending and transient entries.
-  void DiscardNonCommittedEntriesInternal();
-
   // Discards only the transient entry.
   void DiscardTransientEntry();
 

content/browser/frame_host/navigation_controller_impl_unittest.cc

@@ -1072,7 +1072,7 @@ TEST_F(NavigationControllerTest, LoadURL_IgnorePreemptsPending) {
   EXPECT_EQ(-1, controller.GetPendingEntryIndex());
   EXPECT_TRUE(controller.GetPendingEntry());
   EXPECT_EQ(-1, controller.GetLastCommittedEntryIndex());
-  EXPECT_EQ(1, delegate->navigation_state_change_count());
+  EXPECT_EQ(2, 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.
@@ -1083,7 +1083,7 @@ TEST_F(NavigationControllerTest, LoadURL_IgnorePreemptsPending) {
   EXPECT_EQ(-1, controller.GetPendingEntryIndex());
   EXPECT_FALSE(controller.GetPendingEntry());
   EXPECT_EQ(-1, controller.GetLastCommittedEntryIndex());
-  EXPECT_EQ(2, delegate->navigation_state_change_count());
+  EXPECT_EQ(3, delegate->navigation_state_change_count());
 
   contents()->SetDelegate(nullptr);
 }
@@ -1110,7 +1110,7 @@ TEST_F(NavigationControllerTest, LoadURL_AbortDoesntCancelPending) {
   EXPECT_EQ(-1, controller.GetPendingEntryIndex());
   EXPECT_TRUE(controller.GetPendingEntry());
   EXPECT_EQ(-1, controller.GetLastCommittedEntryIndex());
-  EXPECT_EQ(1, delegate->navigation_state_change_count());
+  EXPECT_EQ(2, 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.
@@ -1122,7 +1122,7 @@ TEST_F(NavigationControllerTest, LoadURL_AbortDoesntCancelPending) {
   EXPECT_EQ(-1, controller.GetPendingEntryIndex());
   EXPECT_TRUE(controller.GetPendingEntry());
   EXPECT_EQ(-1, controller.GetLastCommittedEntryIndex());
-  EXPECT_EQ(1, delegate->navigation_state_change_count());
+  EXPECT_EQ(2, delegate->navigation_state_change_count());
   NavigationEntry* pending_entry = controller.GetPendingEntry();
 
   // Ensure that a reload keeps the same pending entry.
@@ -1163,9 +1163,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 twice: once for the loading state
-  // change, and once for the url change.
-  EXPECT_EQ(2, delegate->navigation_state_change_count());
+  // 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 visible entry should be the last committed URL, not the pending one.
   EXPECT_EQ(kExistingURL, controller.GetVisibleEntry()->GetURL());
@@ -1189,7 +1189,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(4, delegate->navigation_state_change_count());
+  EXPECT_EQ(5, delegate->navigation_state_change_count());
 
   // The visible entry should be the last committed URL, not the pending one,
   // so that no spoof is possible.

content/browser/navigation_browsertest.cc

@@ -2435,24 +2435,24 @@ IN_PROC_BROWSER_TEST_P(NavigationBaseBrowserTest,
   // URL here.
   {
     EXPECT_EQ(url_b, shell()->web_contents()->GetVisibleURL());
-    EXPECT_EQ(url_c, embedder_url_tracker.url());
+    EXPECT_EQ(url_b, embedder_url_tracker.url());
   }
   response_A2.WaitForRequest();
   {
     EXPECT_EQ(url_b, shell()->web_contents()->GetVisibleURL());
-    EXPECT_EQ(url_c, embedder_url_tracker.url());
+    EXPECT_EQ(url_b, embedder_url_tracker.url());
   }
 
   // 6. Start history same-document navigation, cancelling 5.
   EXPECT_TRUE(ExecJs(shell()->web_contents(), "history.forward()"));
   {
     EXPECT_EQ(url_b, shell()->web_contents()->GetVisibleURL());
-    EXPECT_EQ(url_c, embedder_url_tracker.url());
+    EXPECT_EQ(url_b, embedder_url_tracker.url());
   }
   EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
   {
     EXPECT_EQ(url_b, shell()->web_contents()->GetVisibleURL());
-    EXPECT_EQ(url_c, embedder_url_tracker.url());
+    EXPECT_EQ(url_b, embedder_url_tracker.url());
   }
 
   // TODO(https://crbug.com/998284): The URL tracked by the embedder should have