Bumping priority of this because it will affect current metrics too (page load metrics, site engagement metrics, etc). I imagine anything looking at page transition in the browser process will face similar issues.

I think this may also turn off LoFi mode, because we'll lose the lofi bit on the navigation params as well.

Still not sure of a solution...

I can confirm, and this actually leads to a similar session history corruption bug that r407239 was meant to prevent.  Repro steps:

1) Start testserver.py on localhost.
2) Visit http://127.0.0.1:8080/
3) Visit http://127.0.0.1:8080/foo
4) Start a navigation to http://127.0.0.1:8080/slow?30
5) Before it commits, go back.

As csharrison@ notes, step 5 will abort the navigation from step 4, but the didStopLoading call will happen after we have replaced the pending_navigation_params_ with the ones for the back navigation.  We'll clear those, giving us no nav_entry_id for commit.

As a result, the back navigation commits in-place, replacing /foo instead of going back one entry.  The forward button will not be enabled.

There isn't really enough context in didStopLoading to know whether the pending params are stale or not, so we'll need to find another way to clear them.

Since this regression is arguably worse than the original bug, I'll revert r407239 and we can look for an alternate fix.  Sorry for the hassle!

No worries! Thanks for taking this one. I am mainly concerned that the only tests for this was in code I had not yet landed. Let's make sure we add tests when we add a fix for the original bug.

If you can separate out the tests from the change you are writing, feel free to land them. More tests are always good.

Charlie, this is now fixed with your revert?

I can confirm at least the tests that were failing in my local branch are now passing.

I can try to throw up a minimized test.

Yes, this is fixed as of the revert in r408285.

csharrison@: Thanks!  Feel free to land the test when you're ready, or I can include it with the updated fix for issue 626838 when I get back to it.

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

commit c0cf770bd57739b34ae035f0fb75bbb9e81f4026
Author: csharrison <csharrison@chromium.org>
Date: Thu Jul 28 19:13:09 2016

Add regression test for an aborting navigation losing its nav_entry_id

BUG= 631617 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/c0cf770bd57739b34ae035f0fb75bbb9e81f4026/content/browser/frame_host/navigation_controller_impl_browsertest.cc