chromeos: Simplify D-Bus proxy resolution service code.

chromeos/dbus/services/proxy_resolution_service_provider.cc

Index: chromeos/dbus/services/proxy_resolution_service_provider.cc
diff --git a/chromeos/dbus/services/proxy_resolution_service_provider.cc b/chromeos/dbus/services/proxy_resolution_service_provider.cc
index ad1eb90981ed3bc9721e86aaa0ba1c2f2bdb7182..539a81bbaa4efc52e6096e188da6a76c90ecbbb4 100644
--- a/chromeos/dbus/services/proxy_resolution_service_provider.cc
+++ b/chromeos/dbus/services/proxy_resolution_service_provider.cc
@@ -26,33 +26,72 @@ namespace chromeos {
 class ProxyResolverImpl : public ProxyResolverInterface {
  public:
   // Data being used in one proxy resolution.
-  class Request {
+  // This would ideally be bound to callbacks as a std::unique_ptr<Request> and
+  // passed around as long as it's needed instead of being ref-counted, but we
+  // don't know whether the callback will be run or not when calling
+  // net::ProxyService::ResolveProxy(): if the proxy can be resolved
+  // synchronously, it won't be, and we don't want to lose ownership of the

[satorux1, 2017/03/22 04:04:02]

I'm confused. Even if it's completed synchronously, it seems that the callback
is run:

157     if (result != net::ERR_IO_PENDING) {
158       VLOG(1) << "Network proxy resolution completed synchronously.";
159       completion_callback.Run(result);
160     }

I'm guessing I'm misunderstanding something.

[Daniel Erat, 2017/03/22 05:07:23]

sorry if the comment is unclear. i meant that net::ProxyService::ResolveProxy()
doesn't run the CompletionCallback that's passed to it if the proxy is resolved
synchronously.

it's likely that i'm misunderstanding how RepeatingCallback<> handles move-only
parameters that are bound into it, though. upon rereading the docs, it sounds
like it uses internal reference counting, so presumably it's safe for
ResolveProxyInternal to std::move the Request into the callback and then run the
callback itself if the proxy is resolved synchronously. i'll give that a try.

+  // Request object in that case.

[Daniel Erat, 2017/03/22 00:54:00]

i initially considered binding base::WeakPtr<Request> instead, but that requires
ProxyResolverImpl to continue holding onto a std::unique_ptr<Request> until
resolution is complete.

i think that a better implementation of net::ProxyService::ResolveProxy() would
either always post the callback that's passed to it, or maybe return the
callback so the caller can decide what to do with it and with any objects that
are owned by it.

+  // TODO(derat): Why is this class public? Make it private.
+  class Request : public base::RefCountedThreadSafe<Request> {
    public:
-    explicit Request(const std::string& source_url) : source_url_(source_url) {
-    }
-
-    virtual ~Request() {}
+    Request(const std::string& source_url,
+            const std::string& signal_interface,
+            const std::string& signal_name,
+            scoped_refptr<dbus::ExportedObject> exported_object,
+            scoped_refptr<net::URLRequestContextGetter> context_getter,
+            base::Callback<void(scoped_refptr<Request>)> notify_task,
+            scoped_refptr<base::SingleThreadTaskRunner> notify_thread)
+        : source_url_(source_url),
+          signal_interface_(signal_interface),
+          signal_name_(signal_name),
+          exported_object_(exported_object),
+          context_getter_(context_getter),
+          notify_task_(notify_task),

[Daniel Erat, 2017/03/22 00:54:00]

note that members like this one should be unnecessary if the later TODO about
moving the implementation into this class is resolved.

+          notify_thread_(notify_thread) {}
 
     // Callback on IO thread for when net::ProxyService::ResolveProxy
     // completes, synchronously or asynchronously.
-    void OnCompletion(scoped_refptr<base::SingleThreadTaskRunner> origin_thread,
-                      int result) {
+    void OnCompletion(int result) {
       // Generate the error message if the error message is not yet set,
       // and there was an error.
       if (error_.empty() && result != net::OK)
         error_ = net::ErrorToString(result);
-      origin_thread->PostTask(FROM_HERE, notify_task_);
+      notify_thread_->PostTask(
+          FROM_HERE, base::Bind(notify_task_, scoped_refptr<Request>(this)));
     }
 
-    std::string source_url_;  // URL being resolved.
-    net::ProxyInfo proxy_info_;  // ProxyInfo resolved for source_url_.
-    std::string error_;  // Error from proxy resolution.
-    base::Closure notify_task_;  // Task to notify of resolution result.
+    // URL being resolved.
+    const std::string source_url_;
+
+    // D-Bus interface, name, and object for emitting result signal after
+    // resolution is complete.
+    const std::string signal_interface_;
+    const std::string signal_name_;
+    const scoped_refptr<dbus::ExportedObject> exported_object_;
+
+    // Used to get network context.
+    const scoped_refptr<net::URLRequestContextGetter> context_getter_;
+
+    // Task to notify of resolution result and task runner to post it to.
+    const base::Callback<void(scoped_refptr<Request>)> notify_task_;
+    const scoped_refptr<base::SingleThreadTaskRunner> notify_thread_;
+
+    // ProxyInfo resolved for |source_url_|.
+    net::ProxyInfo proxy_info_;
+
+    // Error from proxy resolution.
+    std::string error_;
 
    private:
+    friend class base::RefCountedThreadSafe<Request>;
+    virtual ~Request() = default;
+
     DISALLOW_COPY_AND_ASSIGN(Request);
   };
 
+  // TODO(derat): Consider moving all of this class's implementation apart from
+  // ResolveProxy() into the Request class.
   explicit ProxyResolverImpl(std::unique_ptr<ProxyResolverDelegate> delegate)
       : delegate_(std::move(delegate)),
         origin_thread_(base::ThreadTaskRunnerHandle::Get()),
@@ -60,13 +99,6 @@ class ProxyResolverImpl : public ProxyResolverInterface {
 
   ~ProxyResolverImpl() override {
     DCHECK(OnOriginThread());
-
-    for (std::set<Request*>::iterator iter = all_requests_.begin();
-         iter != all_requests_.end(); ++iter) {
-      Request* request = *iter;
-      LOG(WARNING) << "Pending request for " << request->source_url_;
-      delete request;
-    }
   }
 
   // ProxyResolverInterface override.
@@ -78,96 +110,69 @@ class ProxyResolverImpl : public ProxyResolverInterface {
     DCHECK(OnOriginThread());
 
     // Create a request slot for this proxy resolution request.
-    Request* request = new Request(source_url);
-    request->notify_task_ = base::Bind(
-        &ProxyResolverImpl::NotifyProxyResolved,
-        weak_ptr_factory_.GetWeakPtr(),
-        signal_interface,
-        signal_name,
-        exported_object,
-        request);
-    all_requests_.insert(request);
-
     // GetRequestContext() must be called on UI thread.
-    scoped_refptr<net::URLRequestContextGetter> getter =
-        delegate_->GetRequestContext();
-
-    getter->GetNetworkTaskRunner()->PostTask(
+    scoped_refptr<Request> request =
+        new Request(source_url, signal_interface, signal_name, exported_object,
+                    delegate_->GetRequestContext(),
+                    base::Bind(&ProxyResolverImpl::NotifyProxyResolved,
+                               weak_ptr_factory_.GetWeakPtr()),
+                    origin_thread_);
+
+    // This would ideally call PostTaskAndReply() instead of PostTask(), but
+    // ResolveProxyInternal()'s call to net::ProxyService::ResolveProxy() can
+    // result in an asynchronous lookup, in which case the result won't be
+    // available immediately.
+    request->context_getter_->GetNetworkTaskRunner()->PostTask(
         FROM_HERE,
-        base::Bind(&ProxyResolverImpl::ResolveProxyInternal,
-                   request,
-                   origin_thread_,
-                   getter,
-                   exported_object));
+        base::Bind(&ProxyResolverImpl::ResolveProxyInternal, request));
   }
 
  private:
   // Helper function for ResolveProxy().
-  static void ResolveProxyInternal(
-      Request* request,
-      scoped_refptr<base::SingleThreadTaskRunner> origin_thread,
-      scoped_refptr<net::URLRequestContextGetter> getter,
-      scoped_refptr<dbus::ExportedObject> exported_object) {
-    // Make sure we're running on IO thread.
-    DCHECK(getter->GetNetworkTaskRunner()->BelongsToCurrentThread());
-
-    // Check if we have the URLRequestContextGetter.
-    if (!getter.get()) {
+  static void ResolveProxyInternal(scoped_refptr<Request> request) {
+    DCHECK(request->context_getter_->GetNetworkTaskRunner()
+               ->BelongsToCurrentThread());
+
+    if (!request->context_getter_.get()) {
       request->error_ = "No URLRequestContextGetter";

[Daniel Erat, 2017/03/22 00:54:00]

i suspect that the old code is susceptible to use-after-free races. ResolveProxy
posts this static method to the network thread, but it looks to me like
ProxyResolverImpl could be deleted before it runs, resulting in all outstanding
requests being deleted.

-      request->OnCompletion(origin_thread, net::ERR_UNEXPECTED);
+      request->OnCompletion(net::ERR_UNEXPECTED);
       return;
     }
 
-    // Retrieve ProxyService from profile's request context.
     net::ProxyService* proxy_service =
-        getter->GetURLRequestContext()->proxy_service();
+        request->context_getter_->GetURLRequestContext()->proxy_service();
     if (!proxy_service) {
       request->error_ = "No proxy service in chrome";
-      request->OnCompletion(origin_thread, net::ERR_UNEXPECTED);
+      request->OnCompletion(net::ERR_UNEXPECTED);
       return;
     }
 
     VLOG(1) << "Starting network proxy resolution for "
             << request->source_url_;
     net::CompletionCallback completion_callback =
-        base::Bind(&Request::OnCompletion,
-                   base::Unretained(request),
-                   origin_thread);
+        base::Bind(&Request::OnCompletion, request);
     const int result = proxy_service->ResolveProxy(
         GURL(request->source_url_), std::string(), &request->proxy_info_,
-        completion_callback, NULL, NULL, net::NetLogWithSource());
+        completion_callback, nullptr, nullptr, net::NetLogWithSource());
     if (result != net::ERR_IO_PENDING) {
       VLOG(1) << "Network proxy resolution completed synchronously.";
       completion_callback.Run(result);
     }
   }
 
-  // Called on UI thread as task posted from Request::OnCompletion on IO
+  // Called on UI thread as task posted from Request::OnCompletion on network
   // thread.
-  void NotifyProxyResolved(
-      const std::string& signal_interface,
-      const std::string& signal_name,
-      scoped_refptr<dbus::ExportedObject> exported_object,
-      Request* request) {
+  void NotifyProxyResolved(scoped_refptr<Request> request) {
     DCHECK(OnOriginThread());
 
     // Send a signal to the client.
-    dbus::Signal signal(signal_interface, signal_name);
+    dbus::Signal signal(request->signal_interface_, request->signal_name_);

[Daniel Erat, 2017/03/22 00:54:00]

moving the implementation into Request will also avoid these weird member
accesses.

     dbus::MessageWriter writer(&signal);
     writer.AppendString(request->source_url_);
     writer.AppendString(request->proxy_info_.ToPacString());
     writer.AppendString(request->error_);
-    exported_object->SendSignal(&signal);
+    request->exported_object_->SendSignal(&signal);
     VLOG(1) << "Sending signal: " << signal.ToString();
-
-    std::set<Request*>::iterator iter = all_requests_.find(request);
-    if (iter == all_requests_.end()) {
-      LOG(ERROR) << "can't find request slot(" << request->source_url_
-                 << ") in proxy-resolution queue";
-    } else {
-      all_requests_.erase(iter);
-    }
-    delete request;
   }
 
   // Returns true if the current thread is on the origin thread.
@@ -177,7 +182,6 @@ class ProxyResolverImpl : public ProxyResolverInterface {
 
   std::unique_ptr<ProxyResolverDelegate> delegate_;
   scoped_refptr<base::SingleThreadTaskRunner> origin_thread_;
-  std::set<Request*> all_requests_;
   base::WeakPtrFactory<ProxyResolverImpl> weak_ptr_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ProxyResolverImpl);
@@ -190,8 +194,7 @@ ProxyResolutionServiceProvider::ProxyResolutionServiceProvider(
       weak_ptr_factory_(this) {
 }
 
-ProxyResolutionServiceProvider::~ProxyResolutionServiceProvider() {
-}
+ProxyResolutionServiceProvider::~ProxyResolutionServiceProvider() = default;
 
 void ProxyResolutionServiceProvider::Start(
     scoped_refptr<dbus::ExportedObject> exported_object) {
@@ -217,8 +220,9 @@ void ProxyResolutionServiceProvider::OnExported(
   if (!success) {
     LOG(ERROR) << "Failed to export " << interface_name << "."
                << method_name;
+  } else {
+    VLOG(1) << "Method exported: " << interface_name << "." << method_name;
   }
-  VLOG(1) << "Method exported: " << interface_name << "." << method_name;
 }
 
 bool ProxyResolutionServiceProvider::OnOriginThread() {
@@ -272,7 +276,6 @@ ProxyResolutionServiceProvider* ProxyResolutionServiceProvider::Create(
       new ProxyResolverImpl(std::move(delegate)));
 }
 
-ProxyResolverInterface::~ProxyResolverInterface() {
-}
+ProxyResolverInterface::~ProxyResolverInterface() = default;
 
 }  // namespace chromeos