Add a MockAsyncProxyResolverFactory and update some tests to use it.

net/proxy/mock_proxy_resolver.cc

Index: net/proxy/mock_proxy_resolver.cc
diff --git a/net/proxy/mock_proxy_resolver.cc b/net/proxy/mock_proxy_resolver.cc
index 2ff09007eb116822b819dfa78806771987a5b7bd..95591b30522a40d82da6272d9da8200712e44b66 100644
--- a/net/proxy/mock_proxy_resolver.cc
+++ b/net/proxy/mock_proxy_resolver.cc
@@ -112,6 +112,90 @@ void MockAsyncProxyResolverBase::RemovePendingSetPacScriptRequest(
 MockAsyncProxyResolverBase::MockAsyncProxyResolverBase(bool expects_pac_bytes)
     : ProxyResolver(expects_pac_bytes) {}
 
+MockAsyncProxyResolverFactory::Request::Request(
+    MockAsyncProxyResolverFactory* factory,
+    const scoped_refptr<ProxyResolverScriptData>& script_data,
+    scoped_ptr<ProxyResolver>* resolver,
+    const CompletionCallback& callback)
+    : factory_(factory),
+      script_data_(script_data),
+      resolver_(resolver),
+      callback_(callback) {
+}
+
+MockAsyncProxyResolverFactory::Request::~Request() {
+}
+
+void MockAsyncProxyResolverFactory::Request::CompleteNow(
+    int rv,
+    scoped_ptr<ProxyResolver> resolver) {
+  CompletionCallback callback = callback_;

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

How about instead:

scoped_refptr<Request> keep_alive(this);

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

[Sam McNally, 2015/04/23 03:04:43]

Done.

+  *resolver_ = resolver.Pass();
+
+  // Will delete |this|.
+  factory_->RemovePendingRequest(this);
+
+  callback.Run(rv);
+}
+
+void MockAsyncProxyResolverFactory::Request::CompleteNowWithForwarder(
+    int rv,
+    ProxyResolver* resolver) {
+  DCHECK(resolver);
+  CompleteNow(rv, make_scoped_ptr(new ForwardingProxyResolver(resolver)));
+}
+
+class MockAsyncProxyResolverFactory::Job
+    : public ProxyResolverFactory::Request {
+ public:
+  Job(MockAsyncProxyResolverFactory* factory,
+      const scoped_refptr<MockAsyncProxyResolverFactory::Request>& request)
+      : factory_(factory), request_(request) {}
+  ~Job() override {
+    if (std::find(factory_->pending_requests_.begin(),

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

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.

[Sam McNally, 2015/04/23 03:04:43]

Done.

+                  factory_->pending_requests_.end(),
+                  request_.get()) == factory_->pending_requests_.end()) {
+      return;
+    }
+    factory_->cancelled_requests_.push_back(request_);
+    factory_->RemovePendingRequest(request_.get());
+  }
+
+ private:
+  MockAsyncProxyResolverFactory* factory_;
+  scoped_refptr<MockAsyncProxyResolverFactory::Request> request_;
+};
+
+MockAsyncProxyResolverFactory::MockAsyncProxyResolverFactory(
+    bool resolvers_expect_pac_bytes)
+    : ProxyResolverFactory(resolvers_expect_pac_bytes) {
+}
+
+int MockAsyncProxyResolverFactory::CreateProxyResolver(
+    const scoped_refptr<ProxyResolverScriptData>& pac_script,
+    scoped_ptr<ProxyResolver>* resolver,
+    const net::CompletionCallback& callback,
+    scoped_ptr<ProxyResolverFactory::Request>* request_handle) {
+  scoped_refptr<Request> request =
+      new Request(this, pac_script, resolver, callback);
+  pending_requests_.push_back(request);
+
+  request_handle->reset(new Job(this, request));

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

see comment above. |this| is redundant with |request|

[Sam McNally, 2015/04/23 03:04:43]

Done.

+
+  // Test code completes the request by calling request->CompleteNow().
+  return ERR_IO_PENDING;
+}
+
+void MockAsyncProxyResolverFactory::RemovePendingRequest(Request* request) {
+  RequestsList::iterator it =
+      std::find(pending_requests_.begin(), pending_requests_.end(), request);
+  DCHECK(it != pending_requests_.end());
+  pending_requests_.erase(it);
+}
+
+MockAsyncProxyResolverFactory::~MockAsyncProxyResolverFactory() {

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

See earlier comment about cancelling outstanding requests

[Sam McNally, 2015/04/23 03:04:43]

Done.

+}
+
 ForwardingProxyResolver::ForwardingProxyResolver(ProxyResolver* impl)
     : ProxyResolver(impl->expects_pac_bytes()), impl_(impl) {
 }
@@ -143,14 +227,16 @@ int ForwardingProxyResolver::SetPacScript(
 }
 
 ForwardingProxyResolverFactory::ForwardingProxyResolverFactory(
-    ProxyResolver* resolver)
-    : LegacyProxyResolverFactory(resolver->expects_pac_bytes()),
-      resolver_(resolver) {
+    ProxyResolverFactory* impl)
+    : ProxyResolverFactory(impl->expects_pac_bytes()), impl_(impl) {
 }
 
-scoped_ptr<ProxyResolver>
-ForwardingProxyResolverFactory::CreateProxyResolver() {
-  return make_scoped_ptr(new ForwardingProxyResolver(resolver_));
+int ForwardingProxyResolverFactory::CreateProxyResolver(
+    const scoped_refptr<ProxyResolverScriptData>& pac_script,
+    scoped_ptr<ProxyResolver>* resolver,
+    const net::CompletionCallback& callback,
+    scoped_ptr<Request>* request) {
+  return impl_->CreateProxyResolver(pac_script, resolver, callback, request);
 }
 
 }  // namespace net