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

content/renderer/history_controller.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 /*
  * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  *     (http://www.torchmobile.com/)
  *
@@ skipping 169 matching lines @@
 }
 
 void HistoryController::UpdateForCommit(RenderFrameImpl* frame,
                                         const WebHistoryItem& item,
                                         WebHistoryCommitType commit_type,
                                         bool navigation_within_page) {
   switch (commit_type) {
     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);
       }
       break;
     case blink::WebStandardCommit:
       CreateNewBackForwardItem(frame, item, navigation_within_page);
       break;
     case blink::WebInitialCommitInChildFrame:
       UpdateForInitialLoadInChildFrame(frame, item);
@@ skipping 47 matching lines @@
     bool clone_children_of_target) {
   if (!current_entry_) {
     current_entry_.reset(new HistoryEntry(new_item));
   } else {
     current_entry_.reset(current_entry_->CloneAndReplace(
         new_item, clone_children_of_target, target_frame, render_view_));
   }
 }
 
 }  // namespace content