Check the return value when canceling prerenders.

Check the return value when canceling prerenders.

It's possible for an IO thread cancel to race with the prerender being used on
the UI thread. If the UI thread wins, the prerender was used and we should not
cancel network requests, etc.

TEST=none
BUG=293540
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225322

[mmenke, 2013-09-17 17:49:47]

LGTM, great catch!

[davidben, 2013-09-18 16:11:15]

Oh, forgot to add someone from OWNERS. +jochen.

[plz use chromium.org account, 2013-09-19 13:44:37]

you say this was covered by existing tests, yet you didn't update any test?

[mmenke, 2013-09-23 16:17:45]

On 2013/09/19 13:44:37, plz use chromium.org account wrote:
> you say this was covered by existing tests, yet you didn't update any test?

To test it with an integration, you'd have to swap in a prerender, but have a
request that's caught between when the swap starts on the IO thread, and when
the message is posted to the IOThread (Which happens pretty much instantly -
just one message posted), and then make sure the request is not cancelled
(Latter part shouldn't be too hard).  You'd basically need a hook in
PrerenderTracker between posting the message to the IO thread, and updating
state protected by a lock.  That hook would have to trigger the start of a
request that would then be passed through the delegate, so we'd need to
implement some new way of delaying a request before it reaches the RDH, and
before the information about a redirect reaches the RDH - this strikes me as
something that'd be extremely hacky, and the test fixture would be likely to
become flaky.

Alternatively, we'd need a test fixture for the
ChromeResourceDispatcherHostDelegate.  At this level, I think the test would be
sufficiently abstract as to have fairly low value (Mocking out the
PrerenderTracker, or where it gets information from, as well as the signals from
the RDH.

[davidben, 2013-09-23 17:18:31]

On 2013/09/19 13:44:37, plz use chromium.org account wrote:
> you say this was covered by existing tests, yet you didn't update any test?

Oops, sorry, missed this. Yeah, I'd just meant that the tests would catch if I
seriously broke something. Would TEST=none be more appropriate here? This would
only appear in a pretty tight race between firing requests and using a
prerender. I don't know if we have good ways of forcing these sorts of things to
happen in a test.

[jochen (gone - plz use gerrit), 2013-09-23 20:27:14]

Usually, the TEST= line should tell you how to verify that the problem the CL
addresses was fixed, so QA can make sure the problem is indeed fixed e.g. after
merging the patch to a branch.

I agree that reproducing a race reliably in a browser test is sometimes
difficult, however, I disagree that something closer to a unit test would be
worthless. At the very least, it makes sure that the desired behavior doesn't
regress if somebody refactors resource dispatcher host delegate. And they're
less flaky.

Anyway, the code change lgtm. Please consider updating the TEST= line (or add a
test :) )

[commit-bot: I haz the power, 2013-09-24 18:40:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/23460046/1

[commit-bot: I haz the power, 2013-09-24 20:07:22]

Retried try job too often on linux_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-09-25 15:05:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/23460046/1

[commit-bot: I haz the power, 2013-09-25 21:25:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/davidben@chromium.org/23460046/1

[commit-bot: I haz the power, 2013-09-26 04:28:32]

Change committed as 225322