Synchronize events of CFNetworkExecuteProxyAutoConfigurationURL

net/proxy/proxy_resolver_mac.cc

 // Copyright (c) 2011 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 "net/proxy/proxy_resolver_mac.h"
 
 #include <CoreFoundation/CoreFoundation.h>
 
 #include "base/logging.h"
 #include "base/mac/foundation_util.h"
 #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"
 #include "net/proxy/proxy_server.h"
 
 #if defined(OS_IOS)
 #include <CFNetwork/CFProxySupport.h>
 #else
 #include <CoreServices/CoreServices.h>
 #endif
 
 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) {
   if (CFEqual(proxy_type, kCFProxyTypeNone))
     return ProxyServer::SCHEME_DIRECT;
   if (CFEqual(proxy_type, kCFProxyTypeHTTP))
     return ProxyServer::SCHEME_HTTP;
   if (CFEqual(proxy_type, kCFProxyTypeHTTPS)) {
     // The "HTTPS" on the Mac side here means "proxy applies to https://" URLs;
     // the proxy itself is still expected to be an HTTP proxy.
@@ skipping 17 matching lines @@
   DCHECK(*result_ptr == NULL);
 
   if (error != NULL) {
     *result_ptr = CFRetain(error);
   } else {
     *result_ptr = CFRetain(proxies);
   }
   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(
       const scoped_refptr<ProxyResolverScriptData>& script_data);
   ~ProxyResolverMac() override;
 
   // ProxyResolver methods:
   int GetProxyForURL(const GURL& url,
                      ProxyInfo* results,
                      const CompletionCallback& callback,
@@ skipping 15 matching lines @@
 
 ProxyResolverMac::~ProxyResolverMac() {}
 
 // Gets the proxy information for a query URL from a PAC. Implementation
 // inspired by http://developer.apple.com/samplecode/CFProxySupportTool/
 int ProxyResolverMac::GetProxyForURL(const GURL& query_url,
                                      ProxyInfo* results,
                                      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(
       CFURLCreateWithString(kCFAllocatorDefault, query_ref.get(), NULL));
   if (!query_url_ref.get())
     return ERR_FAILED;
   base::ScopedCFTypeRef<CFStringRef> pac_ref(base::SysUTF8ToCFStringRef(
       script_data_->type() == ProxyResolverScriptData::TYPE_AUTO_DETECT
           ? std::string()
           : script_data_->url().spec()));
@@ skipping 22 matching lines @@
   CFStreamClientContext context = { 0, &result, NULL, NULL, NULL };
   base::ScopedCFTypeRef<CFRunLoopSourceRef> runloop_source(
       CFNetworkExecuteProxyAutoConfigurationURL(
           pac_url_ref.get(), query_url_ref.get(), ResultCallback, &context));
   if (!runloop_source)
     return ERR_FAILED;
 
   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()) {
     // TODO(avi): do something better than this
     CFRelease(result);
     return ERR_FAILED;
   }
   base::ScopedCFTypeRef<CFArrayRef> proxy_array_ref(
       base::mac::CFCastStrict<CFArrayRef>(result));
   DCHECK(proxy_array_ref != NULL);
@@ skipping 64 matching lines @@
 int ProxyResolverFactoryMac::CreateProxyResolver(
     const scoped_refptr<ProxyResolverScriptData>& pac_script,
     std::unique_ptr<ProxyResolver>* resolver,
     const CompletionCallback& callback,
     std::unique_ptr<Request>* request) {
   resolver->reset(new ProxyResolverMac(pac_script));
   return OK;
 }
 
 }  // namespace net