Fix HistoryEntry corruption when commit isn't for provisional entry.

content/renderer/history_controller.cc

Index: content/renderer/history_controller.cc
diff --git a/content/renderer/history_controller.cc b/content/renderer/history_controller.cc
index e459d9a3f7c1722f4ec68424c162ea9068c5cbf0..0a6cd634f3546944f4ee4dc7a8bdad8bc6a80168 100644
--- a/content/renderer/history_controller.cc
+++ b/content/renderer/history_controller.cc
@@ -187,7 +187,14 @@ void HistoryController::UpdateForCommit(RenderFrameImpl* frame,
     case blink::WebBackForwardCommit:
       if (!provisional_entry_)
         return;
-      current_entry_.reset(provisional_entry_.release());
+      // Commit the provisional entry, but only if this back/forward item
+      // matches it.  Otherwise it could be a commit from an earlier attempt to
+      // go back/forward, and we should leave the provisional entry in place.
+      if (HistoryEntry::HistoryNode* node =

[Nate Chapin, 2016/04/01 17:33:51]

In most cases, we'll end up looking up the node twice. Once before and once
after making the provisional entry the current entry. Can we merge those, and
early-exit if the node is null?

[Charlie Reis, 2016/04/01 19:05:35]

We can't early exit if the node is null.  When the provisional entry doesn't
match, we still want to look up the node from the current entry so we can set
the item there.  This way the new subframe ends up in the current PageState.

While I might be able to rearrange things and add more checks to avoid a second
lookup in the common case, I think that would make the code hard to follow. 
It's basically just a hash_map lookup (thus inexpensive), and this code will be
replaced soon (next month or so) by switching to the new browser-based flow
anyway.

Thoughts?

+              provisional_entry_->GetHistoryNodeForFrame(frame)) {
+        if (node->item().itemSequenceNumber() == item.itemSequenceNumber())

[Charlie Reis, 2016/04/01 17:28:05]

Is it safe to assume that a back/forward navigation to an item will match the
ISN of the corresponding item in the provisional HistoryEntry?  Seems reasonable
to me and passes all the tests, but I wanted to get your thoughts.

(As a sanity check, I tried skipping the commit of the provisional entry
completely and thankfully that does cause some tests to fail, so we do have some
coverage of it.)

+          current_entry_.reset(provisional_entry_.release());
+      }
       if (HistoryEntry::HistoryNode* node =
               current_entry_->GetHistoryNodeForFrame(frame)) {
         node->set_item(item);