Guard against duplicate adds in PrerenderLinkManager.

Guard against duplicate adds in PrerenderLinkManager.

It seems WebKit may be sending duplicate add requests. Guard against that to prevent crashes until the cause for that can be found.

R=mmenke@chromium.org
BUG=173012
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=182530

[gavinp, 2013-02-14 16:32:41]

mmenke: PTAL.

[mmenke, 2013-02-14 16:38:36]

https://codereview.chromium.org/12253035/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/12253035/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_unittest.cc:319: size_t
CountFromPrerenderLinkManager() {
This function name is wrong, since it only counts running prerenders.

https://codereview.chromium.org/12253035/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_unittest.cc:320: return
prerender_link_manager_->CountRunningPrerenders();
Is this right?  What if we create a second prerender, but don't start running
it?  That would also be bad, no?

[gavinp, 2013-02-14 16:43:51]

Good catch! Take a look at the new upload.

https://codereview.chromium.org/12253035/diff/1/chrome/browser/prerender/prer...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/12253035/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_unittest.cc:319: size_t
CountFromPrerenderLinkManager() {
On 2013/02/14 16:38:36, mmenke wrote:
> This function name is wrong, since it only counts running prerenders.

Done.

https://codereview.chromium.org/12253035/diff/1/chrome/browser/prerender/prer...
chrome/browser/prerender/prerender_unittest.cc:320: return
prerender_link_manager_->CountRunningPrerenders();
On 2013/02/14 16:38:36, mmenke wrote:
> Is this right?  What if we create a second prerender, but don't start running
> it?  That would also be bad, no?

Done.

[mmenke, 2013-02-14 16:48:11]

LGTM.

Suppose setting the concurrency to 2 (Which I missed) and the empty checks were
probably sufficient, but think this is a better test.

https://codereview.chromium.org/12253035/diff/5001/chrome/browser/prerender/p...
File chrome/browser/prerender/prerender_unittest.cc (right):

https://codereview.chromium.org/12253035/diff/5001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_unittest.cc:315: bool
IsEmptyPrerenderLinkManager() {
While you're here, this could be const, too.  Since this is a release blocker
being merged, feel free not to worry about it in this CL, just thought I'd
mention it.

https://codereview.chromium.org/12253035/diff/5001/chrome/browser/prerender/p...
chrome/browser/prerender/prerender_unittest.cc:319: size_t
CountAllLinkPrerenders() {
nit:  const?

[gavinp, 2013-02-14 16:50:40]

On 2013/02/14 16:48:11, mmenke wrote:
> LGTM.
> 
> Suppose setting the concurrency to 2 (Which I missed) and the empty checks
were
> probably sufficient, but think this is a better test.
> 
>
https://codereview.chromium.org/12253035/diff/5001/chrome/browser/prerender/p...
> File chrome/browser/prerender/prerender_unittest.cc (right):
> 
>
https://codereview.chromium.org/12253035/diff/5001/chrome/browser/prerender/p...
> chrome/browser/prerender/prerender_unittest.cc:315: bool
> IsEmptyPrerenderLinkManager() {
> While you're here, this could be const, too.  Since this is a release blocker
> being merged, feel free not to worry about it in this CL, just thought I'd
> mention it.
> 
>
https://codereview.chromium.org/12253035/diff/5001/chrome/browser/prerender/p...
> chrome/browser/prerender/prerender_unittest.cc:319: size_t
> CountAllLinkPrerenders() {
> nit:  const?

Both are good nits. I'm feeling free to ignore them as you said, for now.

Thanks!

[commit-bot: I haz the power, 2013-02-14 16:51:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gavinp@chromium.org/12253035/5001

[commit-bot: I haz the power, 2013-02-14 20:47:48]

Change committed as 182530