Fix a race in proxy_service_unittest.cc that was causing flakiness on purify b...

Fix a race in proxy_service_unittest.cc that was causing flakiness on purify build-bot.

The issue is that the test helper "SyncProxyService" is deleting the underlying ProxyService from the main thread, it should instead be deleted from IO thread.

Deleting from the main thread allows for a small window where deletion of ProxyService can begin while ProxyService::ProcessRequestsQueue() is still executing.

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

[eroman, 2009-03-25 09:27:38]

http://codereview.chromium.org/42596/diff/1002/1003
File net/proxy/proxy_service_unittest.cc (left):

http://codereview.chromium.org/42596/diff/1002/1003#oldcode61
Line 61: class SyncProxyService {
Note, I moved this class further down in the file; It is now implemented using
ProxyServiceWithFutures.

ProxyServiceWithFutures had the same problem (where ProxyService was destroyed
from main thread), so I chose to fix the problem there instead.

[wtc, 2009-03-25 18:21:14]

LGTM.  I'm glad you tracked this down.

http://codereview.chromium.org/42596/diff/1002/1003
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/42596/diff/1002/1003#newcode123
Line 123: void StartReconsider(const GURL& url, net::ProxyInfo* proxy_info) {
Nit: it seems that proxy_info is used as a read-only input
parameter, so perhaps it should be a const reference?
    const net::ProxyInfo& proxy_info

http://codereview.chromium.org/42596/diff/1002/1003#newcode130
Line 130: void StartResolveOrReconsider(const GURL& url, bool is_reconsider) {
It seems that you could rename this method StartRequest,
and replace the is_reconsider boolean parameter by a
method pointer.  Probably not worth the trouble.

http://codereview.chromium.org/42596/diff/1002/1003#newcode221
Line 221: (*result) = new ResultFuture(io_thread_.message_loop(),
Nit: don't need the parentheses around *result.  Same thing
for the next method.

[eroman, 2009-03-25 20:16:03]

Thanks for the review.
Will submit and address any other comments as follow-up.

http://codereview.chromium.org/42596/diff/1002/1003
File net/proxy/proxy_service_unittest.cc (right):

http://codereview.chromium.org/42596/diff/1002/1003#newcode123
Line 123: void StartReconsider(const GURL& url, net::ProxyInfo* proxy_info) {
On 2009/03/25 18:21:14, wtc wrote:
> Nit: it seems that proxy_info is used as a read-only input
> parameter, so perhaps it should be a const reference?
>     const net::ProxyInfo& proxy_info

Done.

http://codereview.chromium.org/42596/diff/1002/1003#newcode130
Line 130: void StartResolveOrReconsider(const GURL& url, bool is_reconsider) {
On 2009/03/25 18:21:14, wtc wrote:
> It seems that you could rename this method StartRequest,
> and replace the is_reconsider boolean parameter by a
> method pointer. 

Done.

> Probably not worth the trouble.

I liked this suggestion; made it more concise and readable.

http://codereview.chromium.org/42596/diff/1002/1003#newcode221
Line 221: (*result) = new ResultFuture(io_thread_.message_loop(),
On 2009/03/25 18:21:14, wtc wrote:
> Nit: don't need the parentheses around *result.  Same thing
> for the next method.

Done.

[wtc, 2009-03-25 21:36:26]

LGTM.  No other comments.