Ensure that CopyStateFromAndPrune doesn't exceed kMaxSessionHistoryEntries.

Ensure that CopyStateFromAndPrune doesn't exceed kMaxSessionHistoryEntries.

BUG=93427
TEST=See bug, comment 112


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=124729

[Charlie Reis, 2012-03-02 02:37:12]

Currently, having more than 50 navigation entries and then committing an Instant
page can put us over the max number of entries.  That confuses the renderer and
causes it to ignore back/forwards.

Chris, can you review for impact on prerendering and instant?

Brett, can you review as a NavigationController OWNER?

https://chromiumcodereview.appspot.com/9565045/diff/3001/content/browser/tab_...
File content/browser/tab_contents/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/9565045/diff/3001/content/browser/tab_...
content/browser/tab_contents/navigation_controller_impl.cc:415:
DCHECK(entries_.size() <= max_entry_count());
This would have caught the bug in the first place, and it should catch any other
cases that we end up exceeding the max entry count.

[brettw, 2012-03-02 05:38:12]

Looking at this briefly, it's not clear to me why this only prunes the first
entry. Like what if it's 3 entries over the max limit? Or are we assuming that
we're only ever one over because we're adding one at a time? If so, we should
probably make the more clear, maybe with an assertion in the prune function?

It would be more clear to me if we called it the "oldest" entry rather than the
"first" one.

[cbentzel, 2012-03-02 16:47:36]

http://codereview.chromium.org/9565045/diff/3001/content/browser/tab_contents...
File content/browser/tab_contents/navigation_controller_impl.cc (right):

http://codereview.chromium.org/9565045/diff/3001/content/browser/tab_contents...
content/browser/tab_contents/navigation_controller_impl.cc:1083: if
(GetEntryCount() > 0)
You should DCHECK that GetEntryCount is only 0 or 1 here. I am pretty confident
that PruneAllButActive guarantees this, and you rely on it here.

Another option to be less fragile is to do the insertion of entries followed by
a prune loop which cuts out the first K entries when there are max + K entries
in the history.

[Charlie Reis, 2012-03-02 17:46:14]

On 2012/03/02 05:38:12, brettw wrote:
> Looking at this briefly, it's not clear to me why this only prunes the first
> entry. Like what if it's 3 entries over the max limit? Or are we assuming that
> we're only ever one over because we're adding one at a time? If so, we should
> probably make the more clear, maybe with an assertion in the prune function?

As Chris mentioned, PruneAllButActive leaves us with at most one entry.  I've
documented it and added a DCHECK.

> It would be more clear to me if we called it the "oldest" entry rather than
the
> "first" one.

Done.

[Charlie Reis, 2012-03-02 17:48:23]

https://chromiumcodereview.appspot.com/9565045/diff/3001/content/browser/tab_...
File content/browser/tab_contents/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/9565045/diff/3001/content/browser/tab_...
content/browser/tab_contents/navigation_controller_impl.cc:1083: if
(GetEntryCount() > 0)
On 2012/03/02 16:47:36, cbentzel wrote:
> You should DCHECK that GetEntryCount is only 0 or 1 here. I am pretty
confident
> that PruneAllButActive guarantees this, and you rely on it here.

Done.

> Another option to be less fragile is to do the insertion of entries followed
by
> a prune loop which cuts out the first K entries when there are max + K entries
> in the history.

I thought about that, but it makes it much harder to reuse code--
InsertOrReplace deletes in advance, and RemoveEntryAtIndex calls GetEntryCount
(which now has an arguably useful DCHECK to ensure we never go over the limit).

I'd prefer to stick with the existing approach that prunes in advance and never
exceeds the limit.

[cbentzel, 2012-03-02 17:53:32]

LGTM

[brettw, 2012-03-02 18:05:06]

lgtm

[commit-bot: I haz the power, 2012-03-02 18:07:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/creis@chromium.org/9565045/8002

[commit-bot: I haz the power, 2012-03-02 20:57:28]

Change committed as 124729