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

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

BUG=597322
TEST=See bug for repro steps.

Committed: https://crrev.com/bb3548ef2fcdb58f9bc638bb5a3c379320fdd0e0
Cr-Commit-Position: refs/heads/master@{#384659}

[Charlie Reis, 2016-04-01 17:28:04]

creis@chromium.org changed reviewers:
+ japhet@chromium.org

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

Nate, can you review this fix?  I have the test in a separate CL
(https://codereview.chromium.org/1848813005/) because it depends on a new test
utility class that hasn't quite landed yet.  (That will also make the fix easier
to merge.)

https://codereview.chromium.org/1848103004/diff/20001/content/renderer/histor...
File content/renderer/history_controller.cc (right):

https://codereview.chromium.org/1848103004/diff/20001/content/renderer/histor...
content/renderer/history_controller.cc:195: if
(node->item().itemSequenceNumber() == item.itemSequenceNumber())
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.)

[Nate Chapin, 2016-04-01 17:33:52]

LGTM with a nit

https://codereview.chromium.org/1848103004/diff/20001/content/renderer/histor...
File content/renderer/history_controller.cc (right):

https://codereview.chromium.org/1848103004/diff/20001/content/renderer/histor...
content/renderer/history_controller.cc:193: if (HistoryEntry::HistoryNode* node
=
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]

https://codereview.chromium.org/1848103004/diff/20001/content/renderer/histor...
File content/renderer/history_controller.cc (right):

https://codereview.chromium.org/1848103004/diff/20001/content/renderer/histor...
content/renderer/history_controller.cc:193: if (HistoryEntry::HistoryNode* node
=
On 2016/04/01 17:33:51, Nate Chapin wrote:
> 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?

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?

[Nate Chapin, 2016-04-01 19:12:57]

On 2016/04/01 19:05:35, Charlie Reis (slow til 4-8) wrote:
>
https://codereview.chromium.org/1848103004/diff/20001/content/renderer/histor...
> File content/renderer/history_controller.cc (right):
> 
>
https://codereview.chromium.org/1848103004/diff/20001/content/renderer/histor...
> content/renderer/history_controller.cc:193: if (HistoryEntry::HistoryNode*
node
> =
> On 2016/04/01 17:33:51, Nate Chapin wrote:
> > 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?
> 
> 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?

Doh, I misread the code. LGTM as-is.

[Charlie Reis, 2016-04-01 19:13:55]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2016-04-01 19:14:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1848103004/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1848103004/20001

[commit-bot: I haz the power, 2016-04-01 19:21:19]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-01 19:22:42]

Description was changed from

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

BUG=597322
TEST=See bug for repro steps.
==========

to

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

BUG=597322
TEST=See bug for repro steps.

Committed: https://crrev.com/bb3548ef2fcdb58f9bc638bb5a3c379320fdd0e0
Cr-Commit-Position: refs/heads/master@{#384659}
==========

[commit-bot: I haz the power, 2016-04-01 19:22:43]

Patchset 2 (id:??) landed as
https://crrev.com/bb3548ef2fcdb58f9bc638bb5a3c379320fdd0e0
Cr-Commit-Position: refs/heads/master@{#384659}

[Charlie Reis, 2016-04-04 16:29:29]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/1859593003/ by creis@chromium.org.

The reason for reverting is: This led to crashes in
HistoryController::UpdateForCommit if there is no current entry and the
provisional entry doesn't match the commit.  See https://crbug.com/600238.  I'll
land an updated fix..