chromeos: Simplify D-Bus proxy resolution service code.

chromeos/dbus/services/proxy_resolution_service_provider.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/dbus/services/proxy_resolution_service_provider.h"
 
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "dbus/bus.h"
 #include "dbus/message.h"
 #include "net/base/net_errors.h"
 #include "net/log/net_log_with_source.h"
 #include "net/proxy/proxy_service.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 namespace chromeos {
 
 // The ProxyResolverInterface implementation used in production.
 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()),
         weak_ptr_factory_(this) {}
 
   ~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.
   void ResolveProxy(
       const std::string& source_url,
       const std::string& signal_interface,
       const std::string& signal_name,
       scoped_refptr<dbus::ExportedObject> exported_object) override {
     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<Request> request =
+        new Request(source_url, signal_interface, signal_name, exported_object,
+                    delegate_->GetRequestContext(),
+                    base::Bind(&ProxyResolverImpl::NotifyProxyResolved,
+                               weak_ptr_factory_.GetWeakPtr()),
+                    origin_thread_);
 
-    // GetRequestContext() must be called on UI thread.
-    scoped_refptr<net::URLRequestContextGetter> getter =
-        delegate_->GetRequestContext();
-
-    getter->GetNetworkTaskRunner()->PostTask(
+    // 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());
+  static void ResolveProxyInternal(scoped_refptr<Request> request) {
+    DCHECK(request->context_getter_->GetNetworkTaskRunner()
+               ->BelongsToCurrentThread());
 
-    // Check if we have the URLRequestContextGetter.
-    if (!getter.get()) {
+    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.
   bool OnOriginThread() {
     return origin_thread_->BelongsToCurrentThread();
   }
 
   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);
 };
 
 ProxyResolutionServiceProvider::ProxyResolutionServiceProvider(
     ProxyResolverInterface* resolver)
     : resolver_(resolver),
       origin_thread_(base::ThreadTaskRunnerHandle::Get()),
       weak_ptr_factory_(this) {
 }
 
-ProxyResolutionServiceProvider::~ProxyResolutionServiceProvider() {
-}
+ProxyResolutionServiceProvider::~ProxyResolutionServiceProvider() = default;
 
 void ProxyResolutionServiceProvider::Start(
     scoped_refptr<dbus::ExportedObject> exported_object) {
   DCHECK(OnOriginThread());
   exported_object_ = exported_object;
   VLOG(1) << "ProxyResolutionServiceProvider started";
   exported_object_->ExportMethod(
       kLibCrosServiceInterface,
       kResolveNetworkProxy,
       // Weak pointers can only bind to methods without return values,
       // hence we cannot bind ResolveProxyInternal here. Instead we use a
       // static function to solve this problem.
       base::Bind(&ProxyResolutionServiceProvider::CallResolveProxyHandler,
                  weak_ptr_factory_.GetWeakPtr()),
       base::Bind(&ProxyResolutionServiceProvider::OnExported,
                  weak_ptr_factory_.GetWeakPtr()));
 }
 
 void ProxyResolutionServiceProvider::OnExported(
     const std::string& interface_name,
     const std::string& method_name,
     bool success) {
   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() {
   return origin_thread_->BelongsToCurrentThread();
 }
 
 void ProxyResolutionServiceProvider::ResolveProxyHandler(
     dbus::MethodCall* method_call,
     dbus::ExportedObject::ResponseSender response_sender) {
   DCHECK(OnOriginThread());
@@ skipping 33 matching lines @@
   }
   provider_weak_ptr->ResolveProxyHandler(method_call, response_sender);
 }
 
 ProxyResolutionServiceProvider* ProxyResolutionServiceProvider::Create(
     std::unique_ptr<ProxyResolverDelegate> delegate) {
   return new ProxyResolutionServiceProvider(
       new ProxyResolverImpl(std::move(delegate)));
 }
 
-ProxyResolverInterface::~ProxyResolverInterface() {
-}
+ProxyResolverInterface::~ProxyResolverInterface() = default;
 
 }  // namespace chromeos