Synchronize events of CFNetworkExecuteProxyAutoConfigurationURL

net/proxy/proxy_resolver_mac.cc

Index: net/proxy/proxy_resolver_mac.cc
diff --git a/net/proxy/proxy_resolver_mac.cc b/net/proxy/proxy_resolver_mac.cc
index cd4873c3f1e55581f3a1cd5f29b2cb7691504c5e..22e6fcf8a67f9f6ddf42044125c35ff8a443c942 100644
--- a/net/proxy/proxy_resolver_mac.cc
+++ b/net/proxy/proxy_resolver_mac.cc
@@ -11,6 +11,7 @@
 #include "base/mac/scoped_cftyperef.h"
 #include "base/strings/string_util.h"
 #include "base/strings/sys_string_conversions.h"
+#include "base/synchronization/lock.h"
 #include "net/base/net_errors.h"
 #include "net/proxy/proxy_info.h"
 #include "net/proxy/proxy_resolver.h"
@@ -26,6 +27,12 @@ namespace net {
 
 namespace {
 
+// Forward declaration of the callback function used by the
+// SynchronizedRunLoopObserver class.
+void RunLoopObserverCallBackFunc(CFRunLoopObserverRef observer,
+                                 CFRunLoopActivity activity,
+                                 void* info);
+
 // Utility function to map a CFProxyType to a ProxyServer::Scheme.
 // If the type is unknown, returns ProxyServer::SCHEME_INVALID.
 ProxyServer::Scheme GetProxyServerScheme(CFStringRef proxy_type) {
@@ -63,6 +70,92 @@ void ResultCallback(void* client, CFArrayRef proxies, CFErrorRef error) {
   CFRunLoopStop(CFRunLoopGetCurrent());
 }
 
+#pragma mark - SynchronizedRunLoopObserver
+// A run loop observer that guarantees that no any two run loop sources
+// protected by the same lock will be fired concurrently in different threads.
+// The observer does not prevent the parallel execution of the sources but only
+// synchronizes the run loop events associated with the sources. In the context
+// of proxy resolver, the observer is used to synchronize the execution of the
+// callbacks function that handles the result of
+// CFNetworkExecuteProxyAutoConfigurationURL execution.
+class SynchronizedRunLoopObserver {
+ public:
+  // Creates the instance of an observer that will synchronize the sources
+  // using a given |lock|.
+  SynchronizedRunLoopObserver(base::Lock& lock);

[mef, 2016/06/28 19:41:06]

add destructor with DCHECK(!lock_acquired_);

[kapishnikov, 2016/06/28 21:31:04]

Done.

+  // Adds the observer to the current run loop for a given run loop mode.
+  // This method should always be paired with |RemoveObserver|.
+  void AddObserver(const CFStringRef mode);
+  // Removes the observer from the current run loop for a given run loop mode.
+  // This method should always be paired with |AddObserver|.
+  void RemoveObserver(const CFStringRef mode);
+  // Callback function that is called when an observable run loop event occurs.
+  void RunLoopObserverCallBack(CFRunLoopObserverRef observer,
+                               CFRunLoopActivity activity);
+
+ private:
+  // Lock to use to synchronize the run loop sources.
+  base::Lock& lock_;
+  // Indicates whether the current observer holds the lock. It is used to
+  // avoid double locking and releasing.
+  bool lock_acquired_;
+  // The underlying CFRunLoopObserverRef structure wrapped by this instance.
+  base::ScopedCFTypeRef<CFRunLoopObserverRef> observer_;
+};
+
+SynchronizedRunLoopObserver::SynchronizedRunLoopObserver(base::Lock& lock)
+    : lock_(lock), lock_acquired_(false) {
+  CFRunLoopObserverContext observer_context = {0, this, NULL, NULL, NULL};
+  observer_.reset(CFRunLoopObserverCreate(
+      kCFAllocatorDefault,
+      kCFRunLoopBeforeSources | kCFRunLoopBeforeWaiting | kCFRunLoopExit, true,
+      0, RunLoopObserverCallBackFunc, &observer_context));
+}
+
+void SynchronizedRunLoopObserver::AddObserver(const CFStringRef mode) {

[mef, 2016/06/28 19:41:06]

Rename into AddToCurrentRunLoop().

[kapishnikov, 2016/06/28 21:31:04]

Done.

+  CFRunLoopAddObserver(CFRunLoopGetCurrent(), observer_.get(), mode);
+}
+
+void SynchronizedRunLoopObserver::RemoveObserver(const CFStringRef mode) {

[mef, 2016/06/28 19:41:06]

Rename into RemoveFromCurrentRunLoop().

[kapishnikov, 2016/06/28 21:31:04]

Done.

+  CFRunLoopRemoveObserver(CFRunLoopGetCurrent(), observer_.get(), mode);
+}
+
+void SynchronizedRunLoopObserver::RunLoopObserverCallBack(
+    CFRunLoopObserverRef observer,
+    CFRunLoopActivity activity) {
+  // Acquire the lock when a source has been signaled and going to be fired.
+  // In the context of the proxy resolver that happens when the proxy for a
+  // given URL has been resolved and the callback function that handles the
+  // result is going to be fired.
+  // Release the lock when all source events have been handled.
+  switch (activity) {
+    case kCFRunLoopBeforeSources:
+      if (!lock_acquired_) {

[mef, 2016/06/28 16:59:51]

Having |lock_acquired_| flag and checking it outside of lock seems error-prone.

[kapishnikov, 2016/06/28 18:51:20]

A SynchronizedRunLoopObservers instance is always associated with a single run
loop (thread). Therefore, |lock_acquired_| is always changed by a single (same)
thread. |lock_acquired_| indicates whether the current thread possesses the lock
or not. It's value is changed only after the lock was actually acquired or
released.

+        lock_.Acquire();
+        lock_acquired_ = true;
+      }
+      break;
+    case kCFRunLoopBeforeWaiting:
+    case kCFRunLoopExit:
+      if (lock_acquired_) {
+        lock_acquired_ = false;
+        lock_.Release();
+      }
+      break;
+  }
+}
+
+void RunLoopObserverCallBackFunc(CFRunLoopObserverRef observer,
+                                 CFRunLoopActivity activity,
+                                 void* info) {
+  // Forward the call to the instance of SynchronizedRunLoopObserver
+  // that is associated with the current CF run loop observer.
+  SynchronizedRunLoopObserver* observerInstance =
+      (SynchronizedRunLoopObserver*)info;
+  observerInstance->RunLoopObserverCallBack(observer, activity);
+}
+
+#pragma mark - ProxyResolverMac
 class ProxyResolverMac : public ProxyResolver {
  public:
   explicit ProxyResolverMac(
@@ -98,6 +191,14 @@ int ProxyResolverMac::GetProxyForURL(const GURL& query_url,
                                      const CompletionCallback& /*callback*/,
                                      RequestHandle* /*request*/,
                                      const BoundNetLog& net_log) {
+  // A lock shared by all ProxyResolverMac instances. It is used to synchronize
+  // the events of multiple CFNetworkExecuteProxyAutoConfigurationURL run loop
+  // sources. These events are:
+  // 1. Adding the source to the run loop.
+  // 2. Handling the source result.
+  // 3. Removing the source from the run loop.
+  CR_DEFINE_STATIC_LOCAL(base::Lock, lock, ());

[mef, 2016/06/28 16:59:52]

Would it make sense to make lock a static member of SynchronizedRunLoopObserver
class?

[kapishnikov, 2016/06/28 18:51:20]

The lock is used in this method to synchronize 'add' and 'remove' operations of
run loop source. These operations are independent from the observer. It looks
backward for the method to ask for the lock from the observer. On the other
hand, SynchronizedRunLoopObserver is designed to accept any external lock.

[mef, 2016/06/28 19:41:07]

Use base/lazy_instance.h instead, rename into 'cfnetwork_pac_runloop_lock'.

[kapishnikov, 2016/06/28 21:31:04]

Done & Done

+
   base::ScopedCFTypeRef<CFStringRef> query_ref(
       base::SysUTF8ToCFStringRef(query_url.spec()));
   base::ScopedCFTypeRef<CFURLRef> query_url_ref(
@@ -140,10 +241,29 @@ int ProxyResolverMac::GetProxyForURL(const GURL& query_url,
   const CFStringRef private_runloop_mode =
       CFSTR("org.chromium.ProxyResolverMac");
 
+  // Add the run loop observer to synchronize events of
+  // CFNetworkExecuteProxyAutoConfigurationURL sources. See the definition of
+  // |lock| above.
+  std::unique_ptr<SynchronizedRunLoopObserver> observer(

[mef, 2016/06/28 19:41:07]

drop unique_ptr.

[kapishnikov, 2016/06/28 21:31:04]

Done.

+      new SynchronizedRunLoopObserver(lock));
+  observer->AddObserver(private_runloop_mode);
+
+  // Make sure that no CFNetworkExecuteProxyAutoConfigurationURL sources
+  // are added to the run loop concurrently.
+  lock.Acquire();
   CFRunLoopAddSource(CFRunLoopGetCurrent(), runloop_source.get(),
                      private_runloop_mode);
+  lock.Release();
   CFRunLoopRunInMode(private_runloop_mode, DBL_MAX, false);
-  CFRunLoopSourceInvalidate(runloop_source.get());
+  // Make sure that no CFNetworkExecuteProxyAutoConfigurationURL sources
+  // are removed from the run loop concurrently.
+  lock.Acquire();

[mef, 2016/06/28 19:41:06]

Consider AutoLock

[kapishnikov, 2016/06/28 21:31:04]

Done.

+  CFRunLoopRemoveSource(CFRunLoopGetCurrent(), runloop_source.get(),
+                        private_runloop_mode);
+  lock.Release();
+
+  observer->RemoveObserver(private_runloop_mode);
+
   DCHECK(result != NULL);
 
   if (CFGetTypeID(result) == CFErrorGetTypeID()) {