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/memory/ptr_util.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 {

[Daniel Erat, 2017/03/22 14:43:43]

this is moved to the private section. i also made it a (non-copyable) struct,
removed the OnCompletion method (which is now
ProxyResolverImpl::OnResolutionComplete), and changed the members around a bit.

-   public:
-    explicit Request(const std::string& source_url) : source_url_(source_url) {
-    }
-
-    virtual ~Request() {}
-
-    // Callback on IO thread for when net::ProxyService::ResolveProxy
-    // completes, synchronously or asynchronously.
-    void OnCompletion(scoped_refptr<base::SingleThreadTaskRunner> origin_thread,
-                      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_);
-    }
-
-    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.
-
-   private:
-    DISALLOW_COPY_AND_ASSIGN(Request);
-  };
-
   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(

[James Cook, 2017/03/22 19:53:20]

nit: There's a lot of inline code in these class methods, which makes the member
variables far away. I think it would be more readable if the method
implementations were de-inlined.

[Daniel Erat, 2017/03/22 20:11:18]

sure, i've moved the implementations out.

       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);
+    auto request = base::MakeUnique<Request>(source_url, signal_interface,
+                                             signal_name, exported_object,
+                                             delegate_->GetRequestContext());
 
-    // GetRequestContext() must be called on UI thread.
-    scoped_refptr<net::URLRequestContextGetter> getter =
-        delegate_->GetRequestContext();
-
-    getter->GetNetworkTaskRunner()->PostTask(
-        FROM_HERE,
-        base::Bind(&ProxyResolverImpl::ResolveProxyInternal,
-                   request,
-                   origin_thread_,
-                   getter,
-                   exported_object));
+    // 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,
+                              weak_ptr_factory_.GetWeakPtr(),
+                              base::Passed(std::move(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());
+  // Data being used in one proxy resolution.
+  struct Request {
+   public:
+    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)
+        : source_url(source_url),
+          signal_interface(signal_interface),
+          signal_name(signal_name),
+          exported_object(exported_object),
+          context_getter(context_getter) {}
+    ~Request() = default;
 
-    // Check if we have the URLRequestContextGetter.
-    if (!getter.get()) {

[Daniel Erat, 2017/03/22 14:43:43]

i don't understand the reason for this check. it's the same object that's used
to get the thread that this method is posted to, so it was obviously valid at
the time of the refcount being incremented.

-      request->error_ = "No URLRequestContextGetter";
-      request->OnCompletion(origin_thread, net::ERR_UNEXPECTED);
+    // 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 the network context associated with the profile used to run
+    // this request.
+    const scoped_refptr<net::URLRequestContextGetter> context_getter;
+
+    // ProxyInfo resolved for |source_url|.
+    net::ProxyInfo proxy_info;
+
+    // Error from proxy resolution.
+    std::string error;
+
+   private:
+    DISALLOW_COPY_AND_ASSIGN(Request);
+  };
+
+  // Returns true if the current thread is on the origin thread.
+  bool OnOriginThread() { return origin_thread_->BelongsToCurrentThread(); }
+
+  // Helper method for ResolveProxy() that runs on network thread.
+  void ResolveProxyInternal(std::unique_ptr<Request> request) {

[James Cook, 2017/03/22 19:53:20]

nit: Maybe ResolveProxyOnNetworkThread()?

[Daniel Erat, 2017/03/22 20:11:18]

Done.

+    DCHECK(request->context_getter->GetNetworkTaskRunner()
+               ->BelongsToCurrentThread());
+
+    net::ProxyService* proxy_service =
+        request->context_getter->GetURLRequestContext()->proxy_service();
+    if (!proxy_service) {
+      request->error = "No proxy service in chrome";
+      OnResolutionComplete(std::move(request), net::ERR_UNEXPECTED);
       return;
     }
 
-    // Retrieve ProxyService from profile's request context.
-    net::ProxyService* proxy_service =
-        getter->GetURLRequestContext()->proxy_service();
-    if (!proxy_service) {
-      request->error_ = "No proxy service in chrome";
-      request->OnCompletion(origin_thread, net::ERR_UNEXPECTED);
-      return;
-    }
+    Request* request_ptr = request.get();
+    net::CompletionCallback callback = base::Bind(
+        &ProxyResolverImpl::OnResolutionComplete,
+        weak_ptr_factory_.GetWeakPtr(), base::Passed(std::move(request)));
 
     VLOG(1) << "Starting network proxy resolution for "
-            << request->source_url_;
-    net::CompletionCallback completion_callback =
-        base::Bind(&Request::OnCompletion,
-                   base::Unretained(request),
-                   origin_thread);
+            << request_ptr->source_url;
     const int result = proxy_service->ResolveProxy(
-        GURL(request->source_url_), std::string(), &request->proxy_info_,

[Daniel Erat, 2017/03/22 14:43:43]

i mentioned it in an earlier patch set, but i think that the references to
|request| in the old version of this method are potential use-after-frees. this
is a static method, so there's no weak_ptr to guarantee that ProxyResolverImpl
is still around, and it deletes all of its Requests on destruction.

-        completion_callback, NULL, NULL, net::NetLogWithSource());
+        GURL(request_ptr->source_url), std::string(), &request_ptr->proxy_info,
+        callback, nullptr, nullptr, net::NetLogWithSource());
     if (result != net::ERR_IO_PENDING) {
       VLOG(1) << "Network proxy resolution completed synchronously.";
-      completion_callback.Run(result);
+      callback.Run(result);
     }
   }
 
-  // Called on UI thread as task posted from Request::OnCompletion on IO
-  // thread.
-  void NotifyProxyResolved(
-      const std::string& signal_interface,
-      const std::string& signal_name,
-      scoped_refptr<dbus::ExportedObject> exported_object,
-      Request* request) {
+  // Callback on network thread for when net::ProxyService::ResolveProxy()
+  // completes, synchronously or asynchronously.
+  void OnResolutionComplete(std::unique_ptr<Request> request, int result) {
+    DCHECK(request->context_getter->GetNetworkTaskRunner()
+               ->BelongsToCurrentThread());
+
+    if (request->error.empty() && result != net::OK)
+      request->error = net::ErrorToString(result);
+
+    origin_thread_->PostTask(FROM_HERE,
+                             base::Bind(&ProxyResolverImpl::NotifyProxyResolved,
+                                        weak_ptr_factory_.GetWeakPtr(),
+                                        base::Passed(std::move(request))));
+  }
+
+  // Called on UI thread from OnResolutionCompletion().
+  void NotifyProxyResolved(std::unique_ptr<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);
     dbus::MessageWriter writer(&signal);
-    writer.AppendString(request->source_url_);
-    writer.AppendString(request->proxy_info_.ToPacString());
-    writer.AppendString(request->error_);
-    exported_object->SendSignal(&signal);
+    writer.AppendString(request->source_url);
+    writer.AppendString(request->proxy_info.ToPacString());
+    writer.AppendString(request->error);
+    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