GNOME.org
 

[libsoup] SoupProxyResolverGNOME: improve behavior with more-recent libproxy

commit 25bfd5cbcfb93b94efc7c0f6f0d4bbfa51244e42
Author: Dan Winship <danw gnome org>
Date:   Mon Jun 7 14:57:52 2010 +0200

    SoupProxyResolverGNOME: improve behavior with more-recent libproxy
    
    SoupProxyResolverGNOME had a nasty workaround for the fact that
    libproxy 0.2 would crash inside GConf if called from a thread other
    than the main one. Unfortunately, the workaround involves leaking
    proxy settings into the environment, which is particularly bad since
    gnome-panel uses libsoup (via libgweather via intlclock) and so the
    environment can become polluted for everything started from the panel.
    
    In libproxy 0.3 and later, the workaround is no longer necessary, so
    remove it if libproxy is new enough.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=603285

 configure.ac                        |   15 ++-
 libsoup/soup-proxy-resolver-gnome.c |  262 ++++++++++++++++++++++++-----------
 2 files changed, 194 insertions(+), 83 deletions(-)
---
diff --git a/configure.ac b/configure.ac
index bc504ab..ce1e909 100644
--- a/configure.ac
+++ b/configure.ac
@@ -173,13 +173,24 @@ This is required for GNOME proxy support.
 Pass "--without-gnome" to configure if you want to build libsoup
 without GNOME support.])])
 
+	AC_MSG_CHECKING(libproxy version)
+	libproxy_version=`$PKG_CONFIG --modversion libproxy-1.0`
+	case $libproxy_version in
+	0.2.*)
+		AC_MSG_RESULT([$libproxy_version, using workarounds...])
+		AC_DEFINE(HAVE_LIBPROXY_WITH_NON_THREAD_SAFE_GNOME_MODULE, 1, [Defined if libproxy is old and has a non-thread-safe gnome module])
+		;;
+	*)
+		AC_MSG_RESULT([$libproxy_version, ok])
+		;;
+	esac
+
 	PKG_CHECK_MODULES(GCONF, gconf-2.0, , [AC_MSG_ERROR(dnl
 [Could not find GConf:
 
 $GCONF_PKG_ERRORS
 
-Due to bugs in libproxy as of the GNOME 2.26 freeze date, GConf is
-*also* required for GNOME proxy support.
+This is required for GNOME proxy support.
 
 Pass "--without-gnome" to configure if you want to build libsoup
 without GNOME support.])])
diff --git a/libsoup/soup-proxy-resolver-gnome.c b/libsoup/soup-proxy-resolver-gnome.c
index 1873fbb..186ca4e 100644
--- a/libsoup/soup-proxy-resolver-gnome.c
+++ b/libsoup/soup-proxy-resolver-gnome.c
@@ -22,24 +22,55 @@
 #include <gconf/gconf-client.h>
 #include <proxy.h>
 
-typedef enum {
-	SOUP_PROXY_RESOLVER_GNOME_MODE_NONE,
-	SOUP_PROXY_RESOLVER_GNOME_MODE_MANUAL,
-	SOUP_PROXY_RESOLVER_GNOME_MODE_AUTO
-} SoupProxyResolverGNOMEMode;
-
-/* Since GConf is not thread-safe, making it annoying for us to deal
- * with, we make all of these static variables rather than instance
- * variables, so that we only have to deal with it once, rather than
- * per-resolver.
+/* libproxy/GConf-based proxy resolution, working around multiple
+ * problems:
+ *
+ *   1. GConf is not thread-safe, but a SoupProxyResolver may be used
+ *      from multiple threads. We need to take steps to ensure that if
+ *      there is a thread running the default main loop, then GConf
+ *      calls are only made from that thread.
+ *
+ *   2. libproxy 0.2 was unaware of GConf's non-thread-safety, so it
+ *      is unsafe to call into libproxy other than from the main
+ *      thread as well, unless we can guarantee that it won't use the
+ *      "gnome" plugin.
+ *
+ *   3. libproxy 0.3 fixes the thread-safety issues by just calling
+ *      out to gconftool-2 on every call. That fixes the crashes but
+ *      makes things a lot slower, so ideally we still don't want to
+ *      call it if it's going to use the "gnome" plugin.
+ *
+ *   4. libproxy is not cancellable, and may block (eg, doing WPAD or
+ *      just DNS, or running a PAC script), so if we want to be able
+ *      to cancel calls to it, we need to do them in another thread
+ *      even if we're making a sync call.
+ *
+ * The only good way to ensure that libproxy won't use the gnome
+ * plugin is to ensure that it will use environment variables instead.
+ * But if we just copy the GConf settings to the environment, then
+ * we'll leak them to child processes. For libproxy 0.2, we *can't*
+ * let it use the gnome plugin, so if we need to use libproxy (to get
+ * PAC/WPAD or ignore_host support) then we just suffer through
+ * environment leakage. For libproxy 0.3, the gconftool-calling
+ * slowness is less bad than environment variable leakage would be, so
+ * we let it use the gnome plugin in that case.
+ */
+
+/* We make these variables static rather than per-resolver, since
+ * they're awkward to deal with because of GConf's non-thread-safety,
+ * and because there's only one GConf database regardless of how many
+ * SoupProxyResolverGNOMEs there are anyway.
  */
 G_LOCK_DEFINE_STATIC (resolver_gnome);
-static SoupProxyResolverGNOMEMode proxy_mode;
 static GConfClient *gconf_client;
 static char *proxy_user, *proxy_password;
+static char *http_proxy, *https_proxy;
 
 static pxProxyFactory *libproxy_factory;
 static GThreadPool *libproxy_threadpool;
+#ifdef HAVE_LIBPROXY_WITH_NON_THREAD_SAFE_GNOME_MODULE
+static gboolean overrode_environment;
+#endif
 
 static void soup_proxy_resolver_gnome_interface_init (SoupProxyURIResolverInterface *proxy_resolver_interface);
 
@@ -167,21 +198,39 @@ soup_proxy_resolver_gnome_interface_init (SoupProxyURIResolverInterface *proxy_u
 #define SOUP_GCONF_USE_SAME_PROXY       "/system/http_proxy/use_same_proxy"
 #define SOUP_GCONF_PROXY_IGNORE_HOSTS   "/system/http_proxy/ignore_hosts"
 
+typedef enum {
+	SOUP_PROXY_RESOLVER_GNOME_GCONF_MODE_NONE,
+	SOUP_PROXY_RESOLVER_GNOME_GCONF_MODE_MANUAL,
+	SOUP_PROXY_RESOLVER_GNOME_GCONF_MODE_AUTO
+} SoupProxyResolverGNOMEGConfMode;
+
 static GConfEnumStringPair proxy_mode_map [] = {
-	{ SOUP_PROXY_RESOLVER_GNOME_MODE_NONE,   "none"   },
-	{ SOUP_PROXY_RESOLVER_GNOME_MODE_MANUAL, "manual" },
-	{ SOUP_PROXY_RESOLVER_GNOME_MODE_AUTO,   "auto"   },
+	{ SOUP_PROXY_RESOLVER_GNOME_GCONF_MODE_NONE,   "none"   },
+	{ SOUP_PROXY_RESOLVER_GNOME_GCONF_MODE_MANUAL, "manual" },
+	{ SOUP_PROXY_RESOLVER_GNOME_GCONF_MODE_AUTO,   "auto"   },
 	{ 0, NULL }
 };
 
 static void
 update_proxy_settings (void)
 {
-	char *mode, *http_proxy, *https_proxy = NULL, *no_proxy = NULL;
+	SoupProxyResolverGNOMEGConfMode proxy_mode;
+	char *mode, *no_proxy = NULL;
+	char *autoconfig_url, *host;
+	guint port;
 	GSList *ignore;
+	gboolean need_libproxy;
 
 	/* resolver_gnome is locked */
 
+	if (http_proxy) {
+		g_free (http_proxy);
+		http_proxy = NULL;
+	}
+	if (https_proxy) {
+		g_free (https_proxy);
+		https_proxy = NULL;
+	}
 	if (proxy_user) {
 		g_free (proxy_user);
 		proxy_user = NULL;
@@ -192,42 +241,51 @@ update_proxy_settings (void)
 		proxy_password = NULL;
 	}
 
+#ifdef HAVE_LIBPROXY_WITH_NON_THREAD_SAFE_GNOME_MODULE
+	if (overrode_environment) {
+		g_unsetenv ("PX_CONFIG_ORDER");
+		g_unsetenv ("http_proxy");
+		g_unsetenv ("https_proxy");
+		g_unsetenv ("no_proxy");
+		overrode_environment = FALSE;
+	}
+#endif
+
 	/* Get new settings */
 	mode = gconf_client_get_string (
 		gconf_client, SOUP_GCONF_PROXY_MODE, NULL);
 	if (!mode || !gconf_string_to_enum (proxy_mode_map, mode,
 					    (int *)&proxy_mode))
-		proxy_mode = SOUP_PROXY_RESOLVER_GNOME_MODE_NONE;
+		proxy_mode = SOUP_PROXY_RESOLVER_GNOME_GCONF_MODE_NONE;
 	g_free (mode);
 
-	if (proxy_mode == SOUP_PROXY_RESOLVER_GNOME_MODE_NONE) {
-		if (libproxy_factory) {
-			/* Unset anything we previously set */
-			g_unsetenv ("PX_CONFIG_ORDER");
-			g_unsetenv ("http_proxy");
-			g_unsetenv ("https_proxy");
-			g_unsetenv ("no_proxy");
-		}
-		return;
-	} else if (proxy_mode == SOUP_PROXY_RESOLVER_GNOME_MODE_AUTO) {
-		char *autoconfig_url;
+	switch (proxy_mode) {
+	case SOUP_PROXY_RESOLVER_GNOME_GCONF_MODE_NONE:
+		/* If GConf says "no", but the environment variables are
+		 * set, then use the environment variables
+		 */
+		http_proxy = g_strdup (g_getenv ("http_proxy"));
+		https_proxy = g_strdup (g_getenv ("https_proxy"));
+		no_proxy = g_strdup (g_getenv ("no_proxy"));
+		break;
 
+	case SOUP_PROXY_RESOLVER_GNOME_GCONF_MODE_AUTO:
 		autoconfig_url = gconf_client_get_string (
 			gconf_client, SOUP_GCONF_PROXY_AUTOCONFIG_URL, NULL);
 		if (autoconfig_url && !strncmp (autoconfig_url, "http", 4))
 			http_proxy = g_strconcat ("pac+", autoconfig_url, NULL);
 		else
 			http_proxy = g_strdup ("wpad://");
+		https_proxy = g_strdup (http_proxy);
 		g_free (autoconfig_url);
-	} else /* SOUP_PROXY_RESOLVER_GNOME_MODE_MANUAL */ {
-		char *host;
-		guint port;
+		goto do_ignore_list;
 
+	case SOUP_PROXY_RESOLVER_GNOME_GCONF_MODE_MANUAL:
 		host = gconf_client_get_string (
 			gconf_client, SOUP_GCONF_HTTP_PROXY_HOST, NULL);
 		if (!host || !*host) {
 			g_free (host);
-			proxy_mode = SOUP_PROXY_RESOLVER_GNOME_MODE_NONE;
+			proxy_mode = SOUP_PROXY_RESOLVER_GNOME_GCONF_MODE_NONE;
 			return;
 		}
 		port = gconf_client_get_int (
@@ -264,50 +322,87 @@ update_proxy_settings (void)
 			proxy_password = gconf_client_get_string (
 				gconf_client, SOUP_GCONF_HTTP_PROXY_PASSWORD, NULL);
 		}
+
+		/* fall through */
+
+	do_ignore_list:
+		ignore = gconf_client_get_list (
+			gconf_client, SOUP_GCONF_PROXY_IGNORE_HOSTS,
+			GCONF_VALUE_STRING, NULL);
+		if (ignore) {
+			GString *ignore_list;
+			GSList *i;
+
+			ignore_list = g_string_new (NULL);
+			for (i = ignore; i; i = i->next) {
+				if (ignore_list->len)
+					g_string_append_c (ignore_list, ',');
+				g_string_append (ignore_list, i->data);
+				g_free (i->data);
+			}
+			g_slist_free (ignore);
+			no_proxy = g_string_free (ignore_list, FALSE);
+		}
+		break;
 	}
 
-	ignore = gconf_client_get_list (
-		gconf_client, SOUP_GCONF_PROXY_IGNORE_HOSTS,
-		GCONF_VALUE_STRING, NULL);
-	if (ignore) {
-		GString *ignore_list;
-		GSList *i;
-
-		ignore_list = g_string_new (NULL);
-		for (i = ignore; i; i = i->next) {
-			if (ignore_list->len)
-				g_string_append_c (ignore_list, ',');
-			g_string_append (ignore_list, i->data);
-			g_free (i->data);
+	/* We need to use libproxy if (a) there is an ignore list, or
+	 * (b) we're using PAC or WPAD.
+	 */
+	if (no_proxy)
+		need_libproxy = TRUE;
+	else if (http_proxy &&
+		 (g_str_has_prefix (http_proxy, "pac+") ||
+		  g_str_has_prefix (http_proxy, "wpad:")))
+		need_libproxy = TRUE;
+	else if (https_proxy &&
+		 (g_str_has_prefix (https_proxy, "pac+") ||
+		  g_str_has_prefix (https_proxy, "wpad:")))
+		need_libproxy = TRUE;
+	else
+		need_libproxy = FALSE;
+
+	if (!need_libproxy) {
+		if (libproxy_factory) {
+			px_proxy_factory_free (libproxy_factory);
+			libproxy_factory = NULL;
 		}
-		g_slist_free (ignore);
-		no_proxy = g_string_free (ignore_list, FALSE);
+		if (libproxy_threadpool) {
+			g_thread_pool_free (libproxy_threadpool, FALSE, FALSE);
+			libproxy_threadpool = NULL;
+		}
+		return;
 	}
 
+	/* If we have a "bad" libproxy, set environment variables
+	 * and force it to use them.
+	 */
+#ifdef HAVE_LIBPROXY_WITH_NON_THREAD_SAFE_GNOME_MODULE
 	g_setenv ("PX_CONFIG_ORDER", "envvar", TRUE);
-	g_setenv ("http_proxy", http_proxy, TRUE);
-	g_free (http_proxy);
-	if (https_proxy) {
+	if (http_proxy)
+		g_setenv ("http_proxy", http_proxy, TRUE);
+	else
+		g_unsetenv ("http_proxy");
+	if (https_proxy)
 		g_setenv ("https_proxy", https_proxy, TRUE);
-		g_free (https_proxy);
-	} else
+	else
 		g_unsetenv ("https_proxy");
-	if (no_proxy) {
+	if (no_proxy)
 		g_setenv ("no_proxy", no_proxy, TRUE);
-		g_free (no_proxy);
-	} else
+	else
 		g_unsetenv ("no_proxy");
+	overrode_environment = TRUE;
+#endif
 
 	/* If we haven't created a proxy factory or thread pool yet,
 	 * do so. If we already have one, we don't need to update
-	 * anything, because it rechecks the environment variables
+	 * anything, because it rechecks the environment variables, etc
 	 * every time.
 	 */
 	if (!libproxy_factory)
 		libproxy_factory = px_proxy_factory_new ();
 
-	if (proxy_mode == SOUP_PROXY_RESOLVER_GNOME_MODE_AUTO &&
-	    !libproxy_threadpool) {
+	if (!libproxy_threadpool) {
 		libproxy_threadpool =
 			g_thread_pool_new (libproxy_threadpool_func,
 					   NULL, -1, FALSE, NULL);
@@ -324,7 +419,27 @@ gconf_value_changed (GConfClient *client, const char *key,
 }
 
 static guint
-get_proxy_for_uri (SoupURI *uri, SoupURI **proxy_uri)
+get_proxy_for_uri_direct (SoupURI *uri, SoupURI **proxy_uri)
+{
+	/* resolver_gnome is locked */
+
+	if (uri->scheme == SOUP_URI_SCHEME_HTTP && http_proxy)
+		*proxy_uri = soup_uri_new (http_proxy);
+	else if (uri->scheme == SOUP_URI_SCHEME_HTTPS && https_proxy)
+		*proxy_uri = soup_uri_new (https_proxy);
+	else
+		*proxy_uri = NULL;
+
+	if (*proxy_uri && proxy_user) {
+		soup_uri_set_user (*proxy_uri, proxy_user);
+		soup_uri_set_password (*proxy_uri, proxy_password);
+	}
+
+	return SOUP_STATUS_OK;
+}
+
+static guint
+get_proxy_for_uri_via_libproxy (SoupURI *uri, SoupURI **proxy_uri)
 {
 	char *uristr, **proxies;
 	gboolean got_proxy;
@@ -430,30 +545,16 @@ get_proxy_uri_async (SoupProxyURIResolver  *proxy_uri_resolver,
 	sgad->user_data = user_data;
 
 	G_LOCK (resolver_gnome);
-	switch (proxy_mode) {
-	case SOUP_PROXY_RESOLVER_GNOME_MODE_NONE:
-		sgad->proxy_uri = NULL;
-		sgad->status = SOUP_STATUS_OK;
-		break;
-
-	case SOUP_PROXY_RESOLVER_GNOME_MODE_MANUAL:
-		/* We know libproxy won't do PAC or WPAD in this case,
-		 * so we can make a "blocking" call to it.
-		 */
-		sgad->status = get_proxy_for_uri (uri, &sgad->proxy_uri);
-		break;
-
-	case SOUP_PROXY_RESOLVER_GNOME_MODE_AUTO:
+	if (libproxy_threadpool) {
 		/* FIXME: cancellable */
 		sgad->uri = soup_uri_copy (uri);
 		sgad->async_context = async_context ? g_main_context_ref (async_context) : NULL;
 		g_thread_pool_push (libproxy_threadpool, sgad, NULL);
-		G_UNLOCK (resolver_gnome);
-		return;
+	} else {
+		sgad->status = get_proxy_for_uri_direct (uri, &sgad->proxy_uri);
+		soup_add_completion (async_context, resolved_proxy, sgad);
 	}
 	G_UNLOCK (resolver_gnome);
-
-	soup_add_completion (async_context, resolved_proxy, sgad);
 }
 
 static guint
@@ -465,11 +566,10 @@ get_proxy_uri_sync (SoupProxyURIResolver  *proxy_uri_resolver,
 	guint status;
 
 	G_LOCK (resolver_gnome);
-	if (proxy_mode == SOUP_PROXY_RESOLVER_GNOME_MODE_NONE) {
-		*proxy_uri = NULL;
-		status = SOUP_STATUS_OK;
-	} else
-		status = get_proxy_for_uri (uri, proxy_uri);
+	if (libproxy_factory)
+		status = get_proxy_for_uri_via_libproxy (uri, proxy_uri);
+	else
+		status = get_proxy_for_uri_direct (uri, proxy_uri);
 	G_UNLOCK (resolver_gnome);
 
 	return status;
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]