Ensure that HistoryController's current entry is updated on inert commits.

Ensure that HistoryController's current entry is updated on inert commits.

BUG=480201
TEST=See bug for repro steps.

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

[Charlie Reis, 2015-04-30 06:28:05]

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

[Charlie Reis, 2015-04-30 06:28:05]

Avi, can you take a look overall?  The test follows the repro steps in comment
14 of the bug.  I added a renderer kill as a second line of defense, but the fix
in HistoryController is all that's necessary.

Nate, can you give a sanity check for the HistoryController change you
suggested?

[Avi (use Gerrit), 2015-04-30 15:25:28]

This lgtm, with a question about more defenses.

https://codereview.chromium.org/1112823005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1112823005/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1311: // Make sure that
a subframe commit isn't changing the main frame URL.
Wouldn't this check also apply for DidNavManualSubframe?

[Charlie Reis, 2015-04-30 16:28:29]

Thanks!

https://codereview.chromium.org/1112823005/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/1112823005/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1311: // Make sure that
a subframe commit isn't changing the main frame URL.
On 2015/04/30 15:25:28, Avi wrote:
> Wouldn't this check also apply for DidNavManualSubframe?

I like that idea, but it doesn't look like there's a good way to enforce it in
the NewSubframe case.  There's no existing entry in that case, and we just clone
the last committed entry.  That means the check would never fail if we compared
the URLs of the last committed and new entries.  params.url doesn't work either,
since that's the subframe URL.  And in the OOPIF case, the subframe RFH doesn't
even know the main frame's URL, so we wouldn't be able to report it from the
renderer.

I think I'm ok leaving the defense out of that one for the time being.

[Nate Chapin, 2015-04-30 18:24:10]

LGTM

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

https://codereview.chromium.org/1112823005/diff/40001/content/renderer/histor...
content/renderer/history_controller.cc:172: } else {
Now that this if-else block includes all WebHistoryCommitType values, should it
be a switch?

[Charlie Reis, 2015-04-30 18:36:30]

Thanks!

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

https://codereview.chromium.org/1112823005/diff/40001/content/renderer/histor...
content/renderer/history_controller.cc:172: } else {
On 2015/04/30 18:24:10, Nate Chapin wrote:
> Now that this if-else block includes all WebHistoryCommitType values, should
it
> be a switch?

Done.

[Charlie Reis, 2015-04-30 18:36:38]

The CQ bit was checked by creis@chromium.org

[Charlie Reis, 2015-04-30 18:36:38]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1112823005/#ps60001
(title: "Use switch statement.")

[commit-bot: I haz the power, 2015-04-30 18:38:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1112823005/60001

[commit-bot: I haz the power, 2015-04-30 22:00:05]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-04-30 22:01:10]

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