clamy@ or arthursonzogni@: Can you point us to the logic that determines whether an ongoing navigation gets canceled when another renderer-initiated navigation occurs?  Here, the Gmail same-document commit seems to clear the pending NavigationEntry but doesn't cancel the slow ongoing navigation.

Would be good if we could keep the pending entry in sync with the ongoing navigation (i.e., make sure both are either preserved or canceled).

The navigation from
* https://www.google.com/maps
* https://www.google.com/maps/@XX.8770388,XX.3290716,XXz
is a same document navigation. Only the URL is updated, but the document stays the same.

Navigating to www.konami.jp is a different document navigation.

The logic about (canceling ongoing navigation) or (ignoring the new one) applies only for different document navigations:
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigator_impl.cc?type=cs&q=%22The+renderer-initiated+navigation+request+is+ignored+iff+a)%22&g=0&l=608

In this example, from a navigation point of view, it works as intended. The renderer initiated navigation should not cancel the ongoing browser initiated navigation. Otherwise, malicious website could prevent users from navigating away.

> Would be good if we could keep the pending entry in sync with the ongoing navigation (i.e., make sure both are either preserved or canceled).
I didn't reply to this. I need to think more about this. What would be the difference for the user? A different URL displayed?

Do you know where the URL update happens? I don't think it should affect the current display state if there's a pending navigation.

I don't know where the URL update happens.
I guess it takes its input from the NavigationEntryImpl.
maybe when NavigationControllerDelegate::NotifyNavigationEntryCommitted is called.

The URL update usually happens in response to WebContentsImpl::NotifyNavigationStateChanged - https://cs.chromium.org/chromium/src/content/browser/web_contents/web_contents_impl.cc?rcl=c7a8a7c4f62192d78412a0260db02ab19cdd247c&l=1597. The delegate is usually the chrome/ui/browser.cc code which updates the omnibox based on the state in WebContents and more specifically by calling GetVisibleURL on it or GetVisibleEntry from the WebContent's NavigationController.

Notes from exploration with creis@, as best I can still remember (please fill in if you think I missed something). It's also very likely we've glossed over details while exploring, so any notes where appropriate would help. creis@: Some of these are also questions I thought of while jotting down this list.

Functionally:

* Same-document navigation can't cancel a pending navigation, even if it's a user gesture.
* User-gesture navigation to other document cancels pending browser navigation as expected (::OnBeginNavigation).
* It's unclear whether clicking things (user-gesture) under "Contents" in https://en.wikipedia.org/wiki/Greeting "should" ideally cancel the pending navigation or not.
* This inconsistency between two links that the user clicks on seems weird in the age of single-page websites. /cooking.html->/baking.html may act similar to same-document navigation from /#cooking to /#baking backed by XHR fetching new page content.
* Should the user know to distinguish same-document navigation when clicking on a link?

Code notes:

* Same-document navigation shouldn't hit ::OnBeginNavigation.
* NavigationControllerImpl::RendererDidNavigate has the following segment:

  // We should not have a pending entry anymore.  Clear it again in case any
  // error cases above forgot to do so.
  DiscardNonCommittedEntriesInternal();

  This seems to make most navigation types result in unconditionally discarding the navigation entry. Is this code ever correct or just an additional safeguard?
* _PAGE refers to navigation entry and not the document.
* Even though ::RendererDidNavigateToNewPage doesn't explicitly call NavigationControllerImpl::DiscardNonCommittedEntriesInternal(), unless params.history_list_was_cleared, ::InsertOrReplaceEntry at the end of it does call DiscardNonCommittedEntriesInternal() unconditionally.
* ::InsertOrReplaceEntry always takes the unique ID from the pending entry if it's a navigation to a new page. This function seems to assume that the inserted entry is a copy of pending_entry_?
* Should InsertOrReplaceEntry() ever replace the pending entry, or should cancelling the entry be an outside responsibility?
* Can the pending entry always be kept on same-page navigations where !PendingEntryMatchesHandle()?

Thanks for capturing that, pbos!  I'll also mention that we think this bug might be related to what's being seen in issue 914124.

There's indeed several things to consider:

1) Should same-document navigations get a similar rule about canceling ongoing navigations (i.e., cancel the ongoing navigation iff there's a user gesture)?  That would be more consistent with cross-document navigations, though it may involve a bit more work.

2) Can we change how pending entries are discarded at commit time, so that pending entries corresponding to ongoing (non-canceled) navigations are left in place?  This probably requires a pretty full look at the commit logic in NavigationController to see how to do it safely, since leaving a pending entry around incorrectly can cause a URL spoof.  I think this approach is probably the right way to fix this bug, though.

3) I'm guessing that you're right about the bug in InsertOrReplace entry, and we should probably only be copying over the unique ID if the pending entry matches the commit's NavigationHandle.  We'd have to look at the CL that introduced it to make sure we don't regress something that way.

4) Yes, we should probably rename RendererDidNavigate*Page to *Entry (similarly for the classification values), since "page" is misleading there.