Add a MockAsyncProxyResolverFactory and update some tests to use it.

Add a MockAsyncProxyResolverFactory and update some tests to use it.

This CL:
- adds a mock implementation of ProxyResolverFactory;
- changes the ProxyService tests to use MockAsyncProxyResolverFactory
  instead of using a LegacyProxyResolverFactor; and
- removes ForwardingProxyResolverFactory and updates the tests that
  used it.

BUG=467403
Committed: https://crrev.com/e64b23614bff59704dbd24202bfc4c266d2d7fa3
Cr-Commit-Position: refs/heads/master@{#327431}

[Sam McNally, 2015-04-22 00:55:51]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2015-04-22 01:43:45]

Patchset #1 (id:20001) has been deleted

[Sam McNally, 2015-04-22 03:09:56]

Patchset #1 (id:40001) has been deleted

[Sam McNally, 2015-04-22 05:24:27]

sammc@chromium.org changed reviewers:
+ eroman@chromium.org

[eroman, 2015-04-22 16:30:38]

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/mock_proxy_re...
File net/proxy/mock_proxy_resolver.cc (right):

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/mock_proxy_re...
net/proxy/mock_proxy_resolver.cc:132: CompletionCallback callback = callback_;
How about instead:

scoped_refptr<Request> keep_alive(this);

Then the code can rely on |this| being alive up until the function returns.

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/mock_proxy_re...
net/proxy/mock_proxy_resolver.cc:155: if
(std::find(factory_->pending_requests_.begin(),
The assumption here is that the ProxyResolverFactory::Request does not outlive
the ProxyResolverFactory. 

This is currently underspecified in the documentation for the interface. Please
update the documentation to explicitly say what is expected of implementations.

I think that deleting the ProxyResolverFactory should be a supported mode, and
should cause the requests to be implicitly cancelled.

In this case, we could achieve it by:
  (1) Remove the |factory_| dependency from Job
  (2) When the MockAsyncProxyResolverFactory is deleted, walk the
pending_requests_ and call some method like "FactoryDestroyed()" on it, which
nulls the Request's |factory_| pointer (preventing a use-after-free)
  (3) Here we can do something like
          if (request_->factory_) {
              request_->factory_->cancelled_requests.push ...
              request_->factory_->....
          }
That can be accomplished here by walking the pending_requests_ and marking them
cancelled. Then in here it would be sufficient to check for something like: 
request_->pending(), and avoid accessing factory_ if the request isn't alive.

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/mock_proxy_re...
net/proxy/mock_proxy_resolver.cc:183: request_handle->reset(new Job(this,
request));
see comment above. |this| is redundant with |request|

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/mock_proxy_re...
net/proxy/mock_proxy_resolver.cc:196:
MockAsyncProxyResolverFactory::~MockAsyncProxyResolverFactory() {
See earlier comment about cancelling outstanding requests

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/mock_proxy_re...
File net/proxy/mock_proxy_resolver.h (right):

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/mock_proxy_re...
net/proxy/mock_proxy_resolver.h:139: const ProxyResolverScriptData*
script_data() const {
can you return a "const scoped_refptr<ProxyResolverScriptData>&" instead? Is
that better?

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/mock_proxy_re...
net/proxy/mock_proxy_resolver.h:187: class ForwardingProxyResolverFactory :
public ProxyResolverFactory {
Let's delete this. I don't think there is much value in forwarding a factory to
itself (see the comments in proxy_service_unittest.cc)

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/proxy_service...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:246: make_scoped_ptr(new
ForwardingProxyResolverFactory(&factory)), NULL);
Would it be clearer to just get rid of ForwardingProxyResolverFactory?

The pattern would change to:

MockAsyncProxyResolverFactory* factory = new
MockAsyncProxyResolverFactory(false);
ProxyService service(..., make_scoped_ptr(factory), ...);

[Sam McNally, 2015-04-23 02:38:35]

Patchset #2 (id:80001) has been deleted

[Sam McNally, 2015-04-23 03:04:33]

Patchset #3 (id:120001) has been deleted

[Sam McNally, 2015-04-23 03:04:44]

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/mock_proxy_re...
File net/proxy/mock_proxy_resolver.cc (right):

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/mock_proxy_re...
net/proxy/mock_proxy_resolver.cc:132: CompletionCallback callback = callback_;
On 2015/04/22 16:30:38, eroman wrote:
> How about instead:
> 
> scoped_refptr<Request> keep_alive(this);
> 
> Then the code can rely on |this| being alive up until the function returns.

Done.

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/mock_proxy_re...
net/proxy/mock_proxy_resolver.cc:155: if
(std::find(factory_->pending_requests_.begin(),
On 2015/04/22 16:30:38, eroman wrote:
> The assumption here is that the ProxyResolverFactory::Request does not outlive
> the ProxyResolverFactory. 
> 
> This is currently underspecified in the documentation for the interface.
Please
> update the documentation to explicitly say what is expected of
implementations.
> 
> I think that deleting the ProxyResolverFactory should be a supported mode, and
> should cause the requests to be implicitly cancelled.
> 
> In this case, we could achieve it by:
>   (1) Remove the |factory_| dependency from Job
>   (2) When the MockAsyncProxyResolverFactory is deleted, walk the
> pending_requests_ and call some method like "FactoryDestroyed()" on it, which
> nulls the Request's |factory_| pointer (preventing a use-after-free)
>   (3) Here we can do something like
>           if (request_->factory_) {
>               request_->factory_->cancelled_requests.push ...
>               request_->factory_->....
>           }
> That can be accomplished here by walking the pending_requests_ and marking
them
> cancelled. Then in here it would be sufficient to check for something like: 
> request_->pending(), and avoid accessing factory_ if the request isn't alive.
> 

Done.

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/mock_proxy_re...
net/proxy/mock_proxy_resolver.cc:183: request_handle->reset(new Job(this,
request));
On 2015/04/22 16:30:38, eroman wrote:
> see comment above. |this| is redundant with |request|

Done.

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/mock_proxy_re...
net/proxy/mock_proxy_resolver.cc:196:
MockAsyncProxyResolverFactory::~MockAsyncProxyResolverFactory() {
On 2015/04/22 16:30:38, eroman wrote:
> See earlier comment about cancelling outstanding requests

Done.

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/mock_proxy_re...
File net/proxy/mock_proxy_resolver.h (right):

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/mock_proxy_re...
net/proxy/mock_proxy_resolver.h:139: const ProxyResolverScriptData*
script_data() const {
On 2015/04/22 16:30:38, eroman wrote:
> can you return a "const scoped_refptr<ProxyResolverScriptData>&" instead? Is
> that better?

Done.

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/mock_proxy_re...
net/proxy/mock_proxy_resolver.h:187: class ForwardingProxyResolverFactory :
public ProxyResolverFactory {
On 2015/04/22 16:30:38, eroman wrote:
> Let's delete this. I don't think there is much value in forwarding a factory
to
> itself (see the comments in proxy_service_unittest.cc)

Done.

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/proxy_service...
File net/proxy/proxy_service_unittest.cc (right):

https://codereview.chromium.org/1102463002/diff/60001/net/proxy/proxy_service...
net/proxy/proxy_service_unittest.cc:246: make_scoped_ptr(new
ForwardingProxyResolverFactory(&factory)), NULL);
On 2015/04/22 16:30:38, eroman wrote:
> Would it be clearer to just get rid of ForwardingProxyResolverFactory?
> 
> The pattern would change to:
> 
> MockAsyncProxyResolverFactory* factory = new
> MockAsyncProxyResolverFactory(false);
> ProxyService service(..., make_scoped_ptr(factory), ...);

Done.

[eroman, 2015-04-23 03:17:21]

LGTM.

Judging from the try results there is another modification needed for
content/browser/resolve_proxy_msg_helper_unittest.cc. Assuming that is a
mechanical change no need to get another lgtm from me.

[eroman, 2015-04-23 03:17:54]

One more thing: the CL description still talks about
forwardingproxyresolverfactory and should be updated

[Sam McNally, 2015-04-23 04:06:17]

sammc@chromium.org changed reviewers:
+ avi@chromium.org

[Sam McNally, 2015-04-23 04:06:18]

avi@chromium.org: Please review changes in
content/browser/resolve_proxy_msg_helper_unittest.cc

[Sam McNally, 2015-04-28 23:37:22]

Avi: ping.

[Avi (use Gerrit), 2015-04-29 00:38:11]

LGTM

Feel free to ping me earlier if something seems to have fallen off my plate.

[Sam McNally, 2015-04-29 00:39:50]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2015-04-29 00:39:50]

The patchset sent to the CQ was uploaded after l-g-t-m from eroman@chromium.org
Link to the patchset: https://codereview.chromium.org/1102463002/#ps200001
(title: "rebase")

[commit-bot: I haz the power, 2015-04-29 00:40:34]

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

[commit-bot: I haz the power, 2015-04-29 03:50:38]

Committed patchset #6 (id:200001)

[commit-bot: I haz the power, 2015-04-29 03:51:38]

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