This doesn't seem as a desirable behavior. Can we write a browser test to reproduce those steps? It will be useful both as a regression test and as a way to diagnose/fix the problem.

Should be straightforward to write a test with TestNavigationManager. I'll see if I can throw one up tomorrow, though I'm worried the fix will not be so easy.

I think the relevant code is here:
https://cs.chromium.org/chromium/src/content/browser/frame_host/navigator_impl.cc?rcl=1471375518&l=190

I'm starting to look at this, and can confirm that it repros. I'll try to debug a bit more in the next few days.

In both cases where the browser and renderer are attributed as initiating the navigation, the reload is triggered off of a InputHostMsg_HandleInputEvent_ACK message which eventually triggers a call to content::NavigationControllerImpl::ReloadInternal.

However, in the render-initiated navigation that happens on every other reload, we receive a FrameHostMsg_DidStartProvisionalLoad which causes Chrome to create a NavigationEntryImpl that has the is_render_initiated bit set to true.

We receive the same FrameHostMsg_DidStartProvisionalLoad in both cases, but only create the NavigationEntryImpl every other case, because we evaluate a condition in NavigatorImpl::DidStartMainFrameNavigation differently every other reload. That code is:

  // If there is no browser-initiated pending entry for this navigation and it
  // is not for the error URL, create a pending entry using the current
  // SiteInstance, and ensure the address bar updates accordingly.  We don't
  // know the referrer or extra headers at this point, but the referrer will
  // be set properly upon commit.
  NavigationEntryImpl* pending_entry = controller_->GetPendingEntry();
  bool has_browser_initiated_pending_entry =
      pending_entry && !pending_entry->is_renderer_initiated();

  // If there is a transient entry, creating a new pending entry will result
  // in deleting it, which leads to inconsistent state.
  bool has_transient_entry = !!controller_->GetTransientEntry();

  if (!has_browser_initiated_pending_entry && !has_transient_entry) {
    std::unique_ptr<NavigationEntryImpl> entry =
        NavigationEntryImpl::FromNavigationEntry(
            controller_->CreateNavigationEntry(
                url, content::Referrer(), ui::PAGE_TRANSITION_LINK,
                true /* is_renderer_initiated */, std::string(),
                controller_->GetBrowserContext()));
...

The 'has_browser_initiated_pending_entry' local alternates between true and false on every other reload, causing us to create an entry for a render-initiated navigation in those cases.

has_browser_initiated_pending_entry alternates its value between true and false every other reaload because the pending_entry is null after the second reload, which seems like a bug.

Normally, the pending_entry is set up in NavigationControllerImpl::ReloadInternal. However, shortly after it is set up, the render process sends a FrameHostMsg_DidFailProvisionalLoadWithError message, presumably for the previously failed reload, which causes the pending entry to get nulled out. The FrameHostMsg_DidFailProvisionalLoadWithError is causing us to null out the new pending entry incorrectly.

Given that NavigationEntryImpls have unique IDs, it seems that we may wish to send that ID to the render process and have it echo that ID in the 
FrameHostMsg_DidFailProvisionalLoadWithError and only null out the pending entry if the id matches?

So the proposed fix is to include a navigation id in FrameHostMsg_DidFailProvisionalLoadWithError_Params, and only clear the pending entry if the nav id in that message matches the id of the current pending entry.

I see that the navigation id is already included in some IPC messages, including content::RequestNavigationParams, which is sent in FrameMsg_Navigate, FrameMsg_CommitNavigation, and FrameMsg_FailedNavigation, and content::FrameNavigateParams, which is sent in FrameHostMsg_DidCommitProvisionalLoad. It seems reasonable to me to include this id in FrameHostMsg_DidFailProvisionalLoadWithError, to mirror the existing inclusion in FrameHostMsg_DidCommitProvisionalLoad.

nasko, clamy, how does this sound to you?

I'm including creis since this looks like some of the issues we tried to avoid with the NavigationHandle.

It appears that my proposed fix won't quite work, as the same navigation entry seems to be reused on each reload, so it has the same unique id, and thus there's no way to know whether the FrameHostMsg_DidFailProvisionalLoadWithError is associated with a previous reload attempt, or the current reload attempt.

void NavigationControllerImpl::ReloadInternal(bool check_for_repost,
                                              ReloadType reload_type) {
...
  NavigationEntryImpl* entry = NULL;
  int current_index = -1;

  // If we are reloading the initial navigation, just use the current
  // pending entry.  Otherwise look up the current entry.
  if (IsInitialNavigation() && pending_entry_) {
    entry = pending_entry_;
    ...
  }
...
      pending_entry_ = entry;

Is there some other state that would allow us to distinguish between reloads, to know whether the arriving FrameHostMsg_DidFailProvisionalLoadWithError is for the current reload vs a previously initiated reload?

Alternatively, perhaps we could modify DiscardPendingEntryIfNeeded to somehow know that we're just reloading the current entry & thus shouldn't discard the pending entry?

There is also the related  issue 513742  which was fixed using a similar approach.

I'll go ahead and take this for now, as I'm hoping to fix it as part of my work to improve accuracy of user-initiated page load abort tracking.

creis, do you have thoughts on how we might address this?

It seems that the bug is that we receive a FrameHostMsg_DidFailProvisionalLoadWithError that causes us to clear out the current pending nav entry when we should not do so, since the FrameHostMsg_DidFailProvisionalLoadWithError is for a previous navigation that has already been replaced on the browser side.

Is there some way to know that the FrameHostMsg_DidFailProvisionalLoadWithError is for an old nav, so we could ignore it to avoid clearing out the pending nav entry?

Is there a NavigationHandle equivalent to NavigationEntry::GetUniqueID that allows us to figure out whether the FrameHostMsg_DidFailProvisionalLoadWithError is for the current pending navigation or not? Perhaps we could consider adding a GetUniqueID to NavigationHandle to be able to determine whether IPCs like FrameHostMsg_DidFailProvisionalLoadWithError are associated with the current nav handle, and ignore them if they are not?

This might also be related (or might be a completely different issue). IsRendererInitated() also behaves differently in browser-side-navigation.linux.browser_tests if the test case involves open link in a new tab. 

I just tested this behavior on trunk and this bug is no longer present. Woohoo! Closing.

If we don't want it to regress it may be worthwhile to add a test for this.

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=798023.