Thanks for the report!  I think there may have been some relatively recent changes to titles that may have caused a regression here.  I'll try to look into it.

Here's the bisect info I got, but it looks suspicious to me:

You are probably looking for a change made after 354846 (known good), but no later than 354862 (first known bad).
CHANGELOG URL:
  https://chromium.googlesource.com/chromium/src/+log/ddb0b49f3f9fa19f64cc3bacd1bdd3070571b383..e635e4d73c9478029fbf28d045288993fda69a50

The only one in there that looks related is r354857 (where Nasko reverted the CL that disabled swapped out), but that can't be right.  Among other things, we've relanded that CL.  I'll try again later when I have some more time.

[+avi, japhet]

I looked at this in a debugger and it's pretty concerning.  Twitter apparently does a replaceState in an unload listener.  RenderFrameImpl::sendDidCommitProvisionalLoad matches the replaceState's commit with the back/forward navigation, so it assumes the back/forward has completed and uses that nav entry ID / page ID.  Then the real back/forward navigation completes with the same nav entry ID / page ID.

This has some ugly consequences.  In the browser process, we overwrite the URL and other parameters of the test page's NavigationEntry with Twitter's URL, etc.  Then a title update comes in for Twitter, and we put that on the NavigationEntry.  When the test page actually commits, we overwrite all the parameters again, except there's no title this time and we leave the stale Twitter title in place (explaining the symptoms of this bug).

I'm not sure whether this can cause anything worse to happen, but it's definitely not doing the right thing.  We should be treating the replaceState commit as part of the Twitter NavigationEntry, not the test page's NavigationEntry.  Interestingly, I haven't been able to hit the bug in a simpler repro case yet, so I might be missing a step.

Nate or Avi, can you think of a way we can identify this case in the renderer process, to avoid using (or clearing) the NavigationState for the back/forward when the replaceState in the unload handler commits?  (We may need to be careful to also distinguish between this and a fragment-navigation during unload, which appears to interrupt and cancel the back navigation.)

What's odd is that we have a test for a replaceState while there is a pending entry. Are things different when you do it in an unload listener?

I'm looking into this.

> What's odd is that we have a test for a replaceState while there is a pending entry.

Ooooooh shoot. In those tests, we forgot to check to see if the pending entry is smashed. It is... :(

I'm uploading a fix for the tests at https://codereview.chromium.org/1867413002 to see how bad the damage is.

Sigh.

ClassifyNavigation knows full well that the replaceState call isn't the pending entry. It classifies the navigation as "existing page".

However, at the end of RendererDidNavigateToExistingPage we have:

  // The entry we found in the list might be pending if the user hit
  // back/forward/reload. This load should commit it (since it's already in the
  // list, we can just discard the pending pointer).  We should also discard the
  // pending entry if it corresponds to a different navigation, since that one
  // is now likely canceled.  If it is not canceled, we will treat it as a new
  // navigation when it arrives, which is also ok.
  //
  // Note that we need to use the "internal" version since we don't want to
  // actually change any other state, just kill the pointer.
  DiscardNonCommittedEntriesInternal();

It explicitly blows away any pending entry because... um... why again?

https://codereview.chromium.org/6709056 from five years ago deliberately always nuked the pending entry to fix  bug 68350  . There were two tests added, so let me go and look to see if this is still needed.

Of course, even if RendererDidNavigateToExistingPage doesn't nuke the pending entry, RendererDidNavigate will:

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

Our entire navigation system assumes that we can't commit something that isn't pending, and is super willing to blow away the pending entry if you do so much as sneeze in its direction.

Charlie, how much do you trust our tests to catch issues if we were to remove these pending-entry-killing calls?

Actually, no, that's not at all what's happening. You called out RenderFrameImpl::sendDidCommitProvisionalLoad in comment 4, which is a different failure than the one I pinned for the test failures in our existing nav tests.

Will look deeper.

Ah.

When we start RenderFrameImpl::didNavigateWithinPage, the nav_entry_id on the NavigationStateImpl is 0, which is right. By the time didNavigateWithinPage calls didCommitProvisionalLoad, it's wrong.

The mixup is within UpdateNavigationState, which is called to update the reference fragment in the case of that navigation. However, in the replaceState case, it seems to be mixing up the state with the pending one. Bad.

Still investigating.

The call to UpdateNavigationState from didNavigateWithinPage has the second parameter as |true|, so UpdateNavigationState takes the pending navigation state and overwrites the current one with it.

Re the comment with the reference to the hash navigation, this appears to have been put in to fix a problem, yet causes a larger problem. What's the history here?

Let's go back in time.

3 weeks ago, boliu found that UpdateNavigationState screwed something up in the in-page case, so he was the one who introduced the boolean that neutered part of UpdateNavigationState when called from didNavigateWithinPage in https://codereview.chromium.org/1802383004 .

5 months ago, csharrison switched didNavigateWithinPage to call UpdateNavigationState instead of didCreateDataSource. https://codereview.chromium.org/1409883008 . It's probably still broken there? I need to take a lunch break, so there's one of two possibilities.

1. The switch from didCreateDataSource to UpdateNavigationState caused the problem. Unlikely, since didCreateDataSource did even more than UpdateNavigationState. But I'll check.
2. This was broken before then, and I'll keep searching.

Re the comment "If this was a reference fragment navigation that we initiated" and the call to didCreateDataSource/UpdateNavigationState, that originates with Darin in 2009:

https://codereview.chromium.org/115575

Quote:
> We have to take care to handle reference fragment
> navigations since those do not result in a new WebDataSource 
> being created.  So, in DidChangeLocationWithinPage, we
> update the ExtraData associated with the WebDataSource
> returned by WebFrame::GetDataSource.

and the code for that:
  // If this was a reference fragment navigation that we initiated, then we
  // could end up having a non-null pending navigation state.  We just need to
  // update the ExtraData on the datasource so that others who read the
  // ExtraData will get the new NavigationState.  Similarly, if we did not
  // initiate this navigation, then we need to take care to clear any pre-
  // existing navigation state.
  frame->GetDataSource()->SetExtraData(pending_navigation_state_.release());

which is the problem we have now; when we navigate within a page, we blindly smash the data source on the frame with the pending one.

Joy. We might need to restrict this to just the hash navigation case.

Is there a test to make sure we don't screw this up? I see no tests in the original CL.

As an experiment I commented out the UpdateNavigationState call in RenderFrameImpl::didNavigateWithinPage, and that indeed is the issue. So we need to look at that.

As a further experiment I am tossing that change to the trybots in https://codereview.chromium.org/1884763002 . I'm curious what fails (if anything).

Just making Avi the owner since he's been investigating it.

Charlie noted that bug 96454 was similar; make sure to return to it after addressing this one to see if it's actually related.

After investigation, bug 96454 looks to be different enough from this bug to remain independent.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/68a109ab91b41f9ccfad871e640cd9fc3325e278

commit 68a109ab91b41f9ccfad871e640cd9fc3325e278
Author: avi <avi@chromium.org>
Date: Sun May 15 01:46:32 2016

Only use pending navigation params for browser-initiated navigations.

BUG= 597239 
TEST=as in bug
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/1976573002
Cr-Commit-Position: refs/heads/master@{#393760}

[modify] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/chrome/renderer/content_settings_observer_browsertest.cc
[modify] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/chrome/renderer/safe_browsing/phishing_classifier_delegate_browsertest.cc
[modify] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/content/browser/frame_host/navigation_controller_impl_browsertest.cc
[modify] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/content/public/test/render_view_test.cc
[modify] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/content/public/test/render_view_test.h
[modify] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/content/renderer/render_frame_impl.cc
[modify] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/content/renderer/render_frame_impl.h
[modify] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/content/renderer/render_frame_impl_browsertest.cc
[add] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/content/test/data/navigation_controller/beforeunload_replacestate_1.html
[add] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/content/test/data/navigation_controller/beforeunload_replacestate_2.html
[modify] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/third_party/WebKit/Source/core/frame/History.cpp
[modify] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/third_party/WebKit/Source/core/loader/FrameLoader.cpp
[modify] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/third_party/WebKit/Source/core/loader/FrameLoader.h
[modify] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/third_party/WebKit/Source/core/loader/FrameLoaderClient.h
[modify] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/third_party/WebKit/Source/web/FrameLoaderClientImpl.cpp
[modify] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/third_party/WebKit/Source/web/FrameLoaderClientImpl.h
[modify] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[modify] https://crrev.com/68a109ab91b41f9ccfad871e640cd9fc3325e278/third_party/WebKit/public/web/WebFrameClient.h