mobile: Fix bugs in the Apple PAC proxy resolver

1. Makes PAC proxy resolution run in its own thread, so it doesn't
   interfere with the main Envoy thread.
2. Adds some CFNetwork initialization logic found in Cronet that
   could account for bugs in CFNetworkExecuteProxyAutoConfigurationURL.
3. Explicitely runs the run loop source and makes it a private scoped
   run loop.
4. Fixes the weak_ptr logic in the NetworkConfigurationFilter callbacks.

Signed-off-by: Ali Beyad <abeyad@google.com>

mobile/library/common/BUILD

@@ -31,6 +31,7 @@ envoy_cc_library(
         "//library/common/http:header_utility_lib",
         "//library/common/logger:logger_delegate_lib",
         "//library/common/network:connectivity_manager_lib",
+        "//library/common/network:proxy_api_lib",
         "//library/common/stats:utility_lib",
         "//library/common/types:c_types_lib",
         "@envoy//envoy/server:lifecycle_notifier_interface",

mobile/library/common/extensions/filters/http/network_configuration/filter.cc

@@ -107,15 +107,14 @@ NetworkConfigurationFilter::resolveProxy(Http::RequestHeaderMap& request_headers
 
   std::weak_ptr<NetworkConfigurationFilter> weak_self = weak_from_this();
   Network::ProxyResolutionResult proxy_resolution_result = proxy_resolver->resolver->resolveProxy(
-      target_url, proxy_settings_,
-      [&weak_self](const std::vector<Network::ProxySettings>& proxies) {
+      target_url, proxy_settings_, [weak_self](const std::vector<Network::ProxySettings>& proxies) {
         RELEASE_ASSERT(
             Thread::MainThread::isMainOrTestThread(),
             "NetworkConfigurationProxy PAC proxy resolver callback not running on main thread.");
         // This is the callback invoked from the Apple APIs resolving the PAC file URL, which
-        // gets invoked on the Envoy thread. We keep a weak_ptr to this filter instance
-        // so that, if the stream is canceled and the filter chain is torn down in the meantime,
-        // we will fail to aquire the weak_ptr lock and won't execute any callbacks on the resolved
+        // gets invoked on a separate thread. We keep a weak_ptr to this filter instance so that,
+        // if the stream is canceled and the filter chain is torn down in the meantime, we will
+        // fail to aquire the weak_ptr lock and won't execute any callbacks on the resolved
         // proxies.
         if (auto filter_ptr = weak_self.lock()) {
           filter_ptr->onProxyResolutionComplete(Network::ProxySettings::create(proxies));

mobile/library/common/internal_engine.cc

@@ -10,6 +10,7 @@
 #include "source/common/runtime/runtime_features.h"
 
 #include "absl/synchronization/notification.h"
+#include "library/common/network/proxy_api.h"
 #include "library/common/stats/utility.h"
 
 namespace Envoy {
@@ -131,6 +132,14 @@ envoy_status_t InternalEngine::main(std::shared_ptr<Envoy::OptionsImplBase> opti
       server_ = main_common->server();
       event_dispatcher_ = &server_->dispatcher();
 
+      // If proxy resolution APIs are configured on the Engine, set the main thread's dispatcher
+      // on the proxy resolver for handling callbacks.
+      auto* proxy_resolver = static_cast<Network::ProxyResolverApi*>(
+          Api::External::retrieveApi("envoy_proxy_resolver", /*allow_absent=*/true));
+      if (proxy_resolver != nullptr) {
+        proxy_resolver->resolver->setDispatcher(event_dispatcher_);
+      }
+
       cv_.notifyAll();
     }
     END_TRY

mobile/library/common/network/BUILD

@@ -108,6 +108,7 @@ envoy_cc_library(
     repository = "@envoy",
     deps = [
         ":proxy_settings_lib",
+        "@envoy//envoy/event:dispatcher_interface",
     ],
 )
 
@@ -150,6 +151,9 @@ envoy_cc_library(
             "//library/common/apple:utility_lib",
             "//library/common/extensions/cert_validator/platform_bridge:c_types_lib",
             "//library/common/types:c_types_lib",
+            "@envoy//envoy/event:dispatcher_interface",
+            "@envoy//source/common/common:thread_impl_lib_posix",
+            "@envoy//source/common/common:thread_lib",
         ],
         "//conditions:default": [],
     }),

mobile/library/common/network/apple_pac_proxy_resolver.cc

@@ -13,13 +13,16 @@ namespace Network {
 
 namespace {
 
+CFStringRef pacRunLoopMode() {
+  static const CFStringRef runloop_mode = CFSTR("envoy.PacProxyResolver");
+  return runloop_mode;
+}
+
 // Creates a CFURLRef from a C++ string URL.
 CFURLRef createCFURL(const std::string& url_string) {
-  auto cf_url_string =
-      CFStringCreateWithCString(kCFAllocatorDefault, url_string.c_str(), kCFStringEncodingUTF8);
-  auto cf_url = CFURLCreateWithString(kCFAllocatorDefault, cf_url_string, /*baseURL=*/nullptr);
-  CFRelease(cf_url_string);
-  return cf_url;
+  return CFURLCreateWithBytes(kCFAllocatorDefault,
+                              reinterpret_cast<const UInt8*>(url_string.c_str()),
+                              url_string.length(), kCFStringEncodingUTF8, nullptr);
 }
 
 } // namespace
@@ -31,16 +34,15 @@ void proxyAutoConfigurationResultCallback(void* ptr, CFArrayRef cf_proxies, CFEr
   std::unique_ptr<ProxySettingsResolvedCallback> completion_callback(
       static_cast<ProxySettingsResolvedCallback*>(ptr));
 
-  std::vector<ProxySettings> proxies;
-  if (cf_error != nullptr || cf_proxies == nullptr) {
+  if (cf_error == nullptr) {
     ENVOY_BUG(cf_error != nullptr, Apple::toString(CFErrorCopyDescription(cf_error)));
-    // Treat the error case as if no proxy was configured. Seems to be consistent with what iOS
-    // system (URLSession) is doing.
-    (*completion_callback)(proxies);
-    return;
   }
 
-  for (int i = 0; i < CFArrayGetCount(cf_proxies); i++) {
+  // Treat the error case as if no proxy was configured. Seems to be consistent with what iOS
+  // system (URLSession) is doing.
+  const int num_proxy_entries = cf_proxies != nullptr ? CFArrayGetCount(cf_proxies) : 0;
+  std::vector<ProxySettings> proxies;
+  for (int i = 0; i < num_proxy_entries; i++) {
     CFDictionaryRef cf_dictionary =
         static_cast<CFDictionaryRef>(CFArrayGetValueAtIndex(cf_proxies, i));
     CFStringRef cf_proxy_type =
@@ -65,6 +67,8 @@ void proxyAutoConfigurationResultCallback(void* ptr, CFArrayRef cf_proxies, CFEr
   }
 
   (*completion_callback)(proxies);
+
+  CFRunLoopStop(CFRunLoopGetCurrent());
 }
 
 CFRunLoopSourceRef
@@ -80,13 +84,29 @@ ApplePacProxyResolver::createPacUrlResolverSource(CFURLRef cf_proxy_autoconfigur
 }
 
 void ApplePacProxyResolver::resolveProxies(
-    const std::string& target_url, const std::string& proxy_autoconfiguration_file_url,
+    const std::string& proxy_autoconfiguration_file_url, const std::string& target_url,
     ProxySettingsResolvedCallback proxy_resolution_completed) {
   CFURLRef cf_target_url = createCFURL(target_url);
   CFURLRef cf_proxy_autoconfiguration_file_url = createCFURL(proxy_autoconfiguration_file_url);
+  if (cf_target_url == nullptr || cf_proxy_autoconfiguration_file_url == nullptr) {
+    return;
+  }
 
   std::unique_ptr<ProxySettingsResolvedCallback> completion_callback =
       std::make_unique<ProxySettingsResolvedCallback>(std::move(proxy_resolution_completed));
+
+  // This dummy call to CFNetworkCopyProxiesForURL initializes some state within CFNetwork that is
+  // required by CFNetworkExecuteProxyAutoConfigurationURL.
+  // Chrome/Cronet on Apple does the same:
+  // https://source.chromium.org/chromium/chromium/src/+/main:net/proxy_resolution/proxy_resolver_apple.cc;l=241-248;drc=f5eeebaebe332e71aadfd5c43ec485cb23f0c7f8
+  CFDictionaryRef empty_dictionary =
+      CFDictionaryCreate(nullptr, nullptr, nullptr, 0, nullptr, nullptr);
+  CFArrayRef empty_result = CFNetworkCopyProxiesForURL(cf_target_url, empty_dictionary);
+  CFRelease(empty_dictionary);
+  if (empty_result) {
+    CFRelease(empty_result);
+  }
+
   // According to https://developer.apple.com/documentation/corefoundation/cfstreamclientcontext,
   // the version must be 0.
   auto context = std::make_unique<CFStreamClientContext>(
@@ -102,7 +122,9 @@ void ApplePacProxyResolver::resolveProxies(
   CFRunLoopSourceRef run_loop_source = createPacUrlResolverSource(
       cf_proxy_autoconfiguration_file_url, cf_target_url, context.release());
 
-  CFRunLoopAddSource(CFRunLoopGetCurrent(), run_loop_source, kCFRunLoopDefaultMode);
+  CFRunLoopAddSource(CFRunLoopGetCurrent(), run_loop_source, pacRunLoopMode());
+  CFRunLoopRunInMode(pacRunLoopMode(), DBL_MAX, false);
+  CFRunLoopRemoveSource(CFRunLoopGetCurrent(), run_loop_source, pacRunLoopMode());
 
   CFRelease(cf_target_url);
   CFRelease(cf_proxy_autoconfiguration_file_url);

mobile/library/common/network/apple_pac_proxy_resolver.h

@@ -27,13 +27,13 @@ class ApplePacProxyResolver {
   /**
    * Resolves proxy for a given URL using proxy auto configuration file that's hosted at a given
    * URL.
-   * @param target_url A request URL to resolve the proxy for.
    * @param proxy_autoconfiguration_file_url A URL at which a proxy configuration file is hosted.
+   * @param target_url A request URL to resolve the proxy for.
    * @param proxy_resolution_completed A function that's called with result proxies as its
    * arguments when proxy resolution completes.
    */
-  void resolveProxies(const std::string& target_url,
-                      const std::string& proxy_autoconfiguration_file_url,
+  void resolveProxies(const std::string& proxy_autoconfiguration_file_url,
+                      const std::string& target_url,
                       ProxySettingsResolvedCallback proxy_resolution_completed);
 
 protected:

mobile/library/common/network/apple_proxy_resolver.cc

@@ -1,14 +1,25 @@
 #include "library/common/network/apple_proxy_resolver.h"
 
 #include "source/common/common/assert.h"
+#include "source/common/common/posix/thread_impl.h"
 
 namespace Envoy {
 namespace Network {
 
 AppleProxyResolver::AppleProxyResolver()
     : proxy_settings_monitor_(
           std::make_unique<AppleSystemProxySettingsMonitor>(proxySettingsUpdater())),
-      pac_proxy_resolver_(std::make_unique<ApplePacProxyResolver>()) {}
+      pac_proxy_resolver_(std::make_unique<ApplePacProxyResolver>()),
+      thread_factory_(Thread::PosixThreadFactory::create()) {}
+
+AppleProxyResolver::~AppleProxyResolver() {
+  // Wait for the PAC resolution threads to finish.
+  for (auto& [_, thread] : pac_resolution_threads_) {
+    if (thread->joinable()) {
+      thread->join();
+    }
+  }
+}
 
 SystemProxySettingsReadCallback AppleProxyResolver::proxySettingsUpdater() {
   return SystemProxySettingsReadCallback(
@@ -23,11 +34,14 @@ void AppleProxyResolver::start() {
   started_ = true;
 }
 
+void AppleProxyResolver::setDispatcher(Event::Dispatcher* dispatcher) { dispatcher_ = dispatcher; }
+
 ProxyResolutionResult
 AppleProxyResolver::resolveProxy(const std::string& target_url_string,
                                  std::vector<ProxySettings>& proxies,
                                  ProxySettingsResolvedCallback proxy_resolution_completed) {
   ASSERT(started_, "AppleProxyResolver not started.");
+  ASSERT(dispatcher_ != nullptr, "Dispatcher not set on the AppleProxyResolver.");
 
   std::string pac_file_url;
   {
@@ -55,11 +69,26 @@ AppleProxyResolver::resolveProxy(const std::string& target_url_string,
   // implement a caching layer with TTLs for PAC file URL resolution within AppleProxyResolver
   // itself.
   ASSERT(!pac_file_url.empty(), "PAC file URL must not be empty if PAC is enabled.");
-  pac_proxy_resolver_->resolveProxies(
-      pac_file_url, target_url_string,
-      [proxy_resolution_completed](const std::vector<ProxySettings>& proxies) {
-        proxy_resolution_completed(proxies);
-      });
+  Thread::PosixThreadPtr thread = thread_factory_->createThread(
+      [this, pac_file_url, target_url_string, callback = std::move(proxy_resolution_completed)] {
+        pac_proxy_resolver_->resolveProxies(
+            pac_file_url, target_url_string,
+            [this, callback](const std::vector<ProxySettings>& proxies) {
+              auto pac_resolve_thread_id = thread_factory_->currentPthreadId();
+              dispatcher_->post([this, callback, proxies, pac_resolve_thread_id] {
+                auto thread_handle = pac_resolution_threads_.extract(pac_resolve_thread_id);
+                if (!thread_handle.empty()) {
+                  thread_handle.mapped()->join();
+                }
+
+                callback(proxies);
+              });
+            });
+      },
+      /* options= */ absl::nullopt, /* crash_on_failure=*/false);
+  Thread::ThreadId thread_id = thread->pthreadId();
+  pac_resolution_threads_[thread_id] = std::move(thread);
+
   return ProxyResolutionResult::ResultInProgress;
 }
 

mobile/library/common/network/apple_proxy_resolver.h

@@ -6,6 +6,12 @@
 #include <functional>
 #include <memory>
 
+#include "envoy/event/dispatcher.h"
+
+#include "source/common/common/posix/thread_impl.h"
+#include "source/common/common/thread.h"
+
+#include "absl/container/flat_hash_map.h"
 #include "library/common/network/apple_pac_proxy_resolver.h"
 #include "library/common/network/apple_system_proxy_settings_monitor.h"
 #include "library/common/network/proxy_resolver_interface.h"
@@ -20,7 +26,7 @@ namespace Network {
 class AppleProxyResolver : public ProxyResolver {
 public:
   AppleProxyResolver();
-  virtual ~AppleProxyResolver() = default;
+  virtual ~AppleProxyResolver();
 
   /**
    * Starts proxy resolver. It needs to be called prior to any proxy resolution attempt.
@@ -38,6 +44,8 @@ class AppleProxyResolver : public ProxyResolver {
   resolveProxy(const std::string& target_url_string, std::vector<ProxySettings>& proxies,
                ProxySettingsResolvedCallback proxy_resolution_completed) override;
 
+  virtual void setDispatcher(Event::Dispatcher* dispatcher) override;
+
   /*
    * Supplies a function that updates this instance's proxy settings.
    */
@@ -49,6 +57,9 @@ class AppleProxyResolver : public ProxyResolver {
   std::unique_ptr<AppleSystemProxySettingsMonitor> proxy_settings_monitor_;
   std::unique_ptr<ApplePacProxyResolver> pac_proxy_resolver_;
   absl::optional<SystemProxySettings> proxy_settings_;
+  std::unique_ptr<Thread::PosixThreadFactory> thread_factory_;
+  absl::flat_hash_map<Thread::ThreadId, Thread::PosixThreadPtr> pac_resolution_threads_;
+  Event::Dispatcher* dispatcher_;
   absl::Mutex mutex_;
   bool started_ = false;
 };

mobile/library/common/network/proxy_resolver_interface.h

@@ -1,5 +1,7 @@
 #pragma once
 
+#include "envoy/event/dispatcher.h"
+
 #include "library/common/network/proxy_settings.h"
 
 namespace Envoy {
@@ -28,6 +30,14 @@ class ProxyResolver {
   virtual ProxyResolutionResult
   resolveProxy(const std::string& target_url_string, std::vector<ProxySettings>& proxies,
                ProxySettingsResolvedCallback proxy_resolution_completed) PURE;
+
+  /**
+   * Sets the Event Dispatcher that a proxy resolver instance can use to post callbacks to.
+   * Typically, this would be the main Envoy thread's dispatcher (which is also the worker thread
+   * in Envoy Mobile).
+   * @param dispatcher The event dispatcher.
+   */
+  virtual void setDispatcher(Event::Dispatcher* dispatcher) PURE;
 };
 
 } // namespace Network

mobile/test/common/extensions/filters/http/network_configuration/network_configuration_filter_test.cc

@@ -191,6 +191,7 @@ class MockProxyResolver : public Network::ProxyResolver {
   MOCK_METHOD(Network::ProxyResolutionResult, resolveProxy,
               (const std::string& target_url_string, std::vector<Network::ProxySettings>& proxies,
                Network::ProxySettingsResolvedCallback proxy_resolution_completed));
+  MOCK_METHOD(void, setDispatcher, (Event::Dispatcher * dispatcher));
 };
 
 class NetworkConfigurationFilterProxyResolverApiTest : public NetworkConfigurationFilterTest {

mobile/test/common/proxy/BUILD

@@ -9,7 +9,6 @@ envoy_cc_test_library(
     srcs = select({
         "@envoy//bazel:apple": [
             "test_apple_api_registration.cc",
-            "test_apple_pac_proxy_resolver.cc",
             "test_apple_proxy_resolver.cc",
             "test_apple_proxy_settings_monitor.cc",
         ],
@@ -18,7 +17,6 @@ envoy_cc_test_library(
     hdrs = select({
         "@envoy//bazel:apple": [
             "test_apple_api_registration.h",
-            "test_apple_pac_proxy_resolver.h",
             "test_apple_proxy_resolver.h",
             "test_apple_proxy_settings_monitor.h",
         ],

mobile/test/common/proxy/test_apple_api_registration.cc

@@ -1,6 +1,5 @@
 #include "test/common/proxy/test_apple_api_registration.h"
 
-#include "test/common/proxy/test_apple_pac_proxy_resolver.h"
 #include "test/common/proxy/test_apple_proxy_resolver.h"
 #include "test/common/proxy/test_apple_proxy_settings_monitor.h"
 
@@ -30,11 +29,6 @@ void registerTestAppleProxyResolver(absl::string_view host, int port, const bool
   test_resolver->setSettingsMonitorForTest(
       std::make_unique<Envoy::Network::TestAppleSystemProxySettingsMonitor>(
           std::string(host), port, use_pac_resolver, test_resolver->proxySettingsUpdater()));
-  if (use_pac_resolver) {
-    // Create a TestApplePacProxyResolver and set the test resolver to use it.
-    test_resolver->setPacResolverForTest(
-        std::make_unique<Envoy::Network::TestApplePacProxyResolver>(std::string(host), port));
-  }
   // Start the resolver, as we do when registering the envoy_proxy_resolver API.
   test_resolver->start();
   // Create a new test ProxyResolverApi.