Fix gconf for the linux proxy config service.

-Reenables fetching of settings from gconf.

-Moves all gconf access to happen from the UI thread only, (where
 the default glib main loop runs).

-Adds support for gconf notifications, avoiding having to poll the settings.

-Fixes a small initialization glitch in the unittest. Plus minor code style tweaks.

-Permanently removes gdk and glib threading initialization calls that
 were previously disabled.

-Slight reorganization of ProxyService creation to pass down the IO
 thread MessageLoop.

BUG=11111
TEST=none

Review URL: http://codereview.chromium.org/113043

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@16739 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 611 insertions, 175 deletions

diff --git a/net/proxy/proxy_config.h b/net/proxy/proxy_config.h
index b90e9b7..f9bc6d4 100644
--- a/net/proxy/proxy_config.h
+++ b/net/proxy/proxy_config.h
@@ -28,6 +28,7 @@ class ProxyConfig {
   // Used to numerically identify this configuration.
   ID id() const { return id_; }
   void set_id(int id) { id_ = id; }
+  bool is_valid() { return id_ != INVALID_ID; }
 
   // True if the proxy configuration should be auto-detected.
   bool auto_detect;
diff --git a/net/proxy/proxy_config_service_linux.cc b/net/proxy/proxy_config_service_linux.cc
index cce5835..3421110 100644
--- a/net/proxy/proxy_config_service_linux.cc
+++ b/net/proxy/proxy_config_service_linux.cc
@@ -5,12 +5,12 @@
 #include "net/proxy/proxy_config_service_linux.h"
 
 #include <gconf/gconf-client.h>
-#include <gdk/gdk.h>
 #include <stdlib.h>
 
 #include "base/logging.h"
 #include "base/string_tokenizer.h"
 #include "base/string_util.h"
+#include "base/task.h"
 #include "googleurl/src/url_canon.h"
 #include "net/base/net_errors.h"
 #include "net/http/http_util.h"
@@ -71,8 +71,7 @@ std::string FixupProxyHostScheme(ProxyServer::Scheme scheme,
   std::string::size_type at_sign = host.find("@");
   // Should this be supported?
   if (at_sign != std::string::npos) {
-    LOG(ERROR) << "ProxyConfigServiceLinux: proxy authentication "
-        "not supported";
+    LOG(ERROR) << "Proxy authentication not supported";
     // Disregard the authentication parameters and continue with this hostname.
     host = host.substr(at_sign + 1);
   }
@@ -88,7 +87,7 @@ std::string FixupProxyHostScheme(ProxyServer::Scheme scheme,
 
 }  // namespace
 
-bool ProxyConfigServiceLinux::GetProxyFromEnvVarForScheme(
+bool ProxyConfigServiceLinux::Delegate::GetProxyFromEnvVarForScheme(
     const char* variable, ProxyServer::Scheme scheme,
     ProxyServer* result_server) {
   std::string env_value;
@@ -100,21 +99,20 @@ bool ProxyConfigServiceLinux::GetProxyFromEnvVarForScheme(
         *result_server = proxy_server;
         return true;
       } else {
-        LOG(ERROR) << "ProxyConfigServiceLinux: failed to parse "
-                   << "environment variable " << variable;
+        LOG(ERROR) << "Failed to parse environment variable " << variable;
       }
     }
   }
   return false;
 }
 
-bool ProxyConfigServiceLinux::GetProxyFromEnvVar(
+bool ProxyConfigServiceLinux::Delegate::GetProxyFromEnvVar(
     const char* variable, ProxyServer* result_server) {
   return GetProxyFromEnvVarForScheme(variable, ProxyServer::SCHEME_HTTP,
                                      result_server);
 }
 
-bool ProxyConfigServiceLinux::GetConfigFromEnv(ProxyConfig* config) {
+bool ProxyConfigServiceLinux::Delegate::GetConfigFromEnv(ProxyConfig* config) {
   // Check for automatic configuration first, in
   // "auto_proxy". Possibly only the "environment_proxy" firefox
   // extension has ever used this, but it still sounds like a good
@@ -160,7 +158,7 @@ bool ProxyConfigServiceLinux::GetConfigFromEnv(ProxyConfig* config) {
     ProxyServer::Scheme scheme = ProxyServer::SCHEME_SOCKS4;
     std::string env_version;
     if (env_var_getter_->Getenv("SOCKS_VERSION", &env_version)
-        && env_version.compare("5") == 0)
+        && env_version == "5")
       scheme = ProxyServer::SCHEME_SOCKS5;
     if (GetProxyFromEnvVarForScheme("SOCKS_SERVER", scheme, &proxy_server)) {
       config->proxy_rules.type = ProxyConfig::ProxyRules::TYPE_SINGLE_PROXY;
@@ -183,37 +181,101 @@ bool ProxyConfigServiceLinux::GetConfigFromEnv(ProxyConfig* config) {
 
 namespace {
 
+// static
+// gconf notification callback, dispatched from the default
+// glib main loop.
+void OnGConfChangeNotification(
+    GConfClient* client, guint cnxn_id,
+    GConfEntry* entry, gpointer user_data) {
+  // It would be nice to debounce multiple callbacks in quick
+  // succession, since I guess we'll get one for each changed key. As
+  // it is we will read settings from gconf once for each callback.
+  LOG(INFO) << "gconf change notification for key "
+            << gconf_entry_get_key(entry);
+  // We don't track which key has changed, just that something did change.
+  // Forward to a method on the proxy config service delegate object.
+  ProxyConfigServiceLinux::Delegate* config_service_delegate =
+      reinterpret_cast<ProxyConfigServiceLinux::Delegate*>(user_data);
+  config_service_delegate->OnCheckProxyConfigSettings();
+}
+
 class GConfSettingGetterImpl
     : public ProxyConfigServiceLinux::GConfSettingGetter {
  public:
-  GConfSettingGetterImpl() : client_(NULL) {}
+  GConfSettingGetterImpl() : client_(NULL), loop_(NULL) {}
+
   virtual ~GConfSettingGetterImpl() {
-    if (client_)
-      g_object_unref(client_);
+    LOG(INFO) << "~GConfSettingGetterImpl called";
+    // client_ should have been released before now, from
+    // Delegate::OnDestroy(), while running on the UI thread.
+    DCHECK(!client_);
   }
 
-  virtual void Enter() {
-    gdk_threads_enter();
+  virtual bool Init() {
+    DCHECK(!client_);
+    DCHECK(!loop_);
+    loop_ = MessageLoopForUI::current();
+    client_ = gconf_client_get_default();
+    if (!client_) {
+      // It's not clear whether/when this can return NULL.
+      LOG(ERROR) << "Unable to create a gconf client";
+      loop_ = NULL;
+      return false;
+    }
+    GError* error = NULL;
+    // We need to add the directories for which we'll be asking
+    // notifications, and we might as well ask to preload them.
+    gconf_client_add_dir(client_, "/system/proxy",
+                         GCONF_CLIENT_PRELOAD_ONELEVEL, &error);
+    if (error == NULL) {
+      gconf_client_add_dir(client_, "/system/http_proxy",
+                           GCONF_CLIENT_PRELOAD_ONELEVEL, &error);
+    }
+    if (error != NULL) {
+      LOG(ERROR) << "Error requesting gconf directory: " << error->message;
+      g_error_free(error);
+      Release();
+      return false;
+    }
+    return true;
   }
-  virtual void Leave() {
-    gdk_threads_leave();
+
+  void Release() {
+    if (client_) {
+      DCHECK(MessageLoop::current() == loop_);
+      // This also disables gconf notifications.
+      g_object_unref(client_);
+      client_ = NULL;
+      loop_ = NULL;
+    }
   }
 
-  virtual bool InitIfNeeded() {
-    if (!client_) {
-      Enter();
-      client_ = gconf_client_get_default();
-      Leave();
-      // It's not clear whether/when this can return NULL.
-      if (!client_)
-        LOG(ERROR) << "ProxyConfigServiceLinux: Unable to create "
-            "a gconf client";
+  bool SetupNotification(void* callback_user_data) {
+    DCHECK(client_);
+    DCHECK(MessageLoop::current() == loop_);
+    GError* error = NULL;
+    gconf_client_notify_add(
+        client_, "/system/proxy",
+        OnGConfChangeNotification, callback_user_data,
+        NULL, &error);
+    if (error == NULL) {
+      gconf_client_notify_add(
+          client_, "/system/http_proxy",
+          OnGConfChangeNotification, callback_user_data,
+          NULL, &error);
     }
-    return client_ != NULL;
+    if (error != NULL) {
+      LOG(ERROR) << "Error requesting gconf notifications: " << error->message;
+      g_error_free(error);
+      Release();
+      return false;
+    }
+    return true;
   }
 
   virtual bool GetString(const char* key, std::string* result) {
-    CHECK(client_);
+    DCHECK(client_);
+    DCHECK(MessageLoop::current() == loop_);
     GError* error = NULL;
     gchar* value = gconf_client_get_string(client_, key, &error);
     if (HandleGError(error, key))
@@ -225,7 +287,8 @@ class GConfSettingGetterImpl
     return true;
   }
   virtual bool GetBoolean(const char* key, bool* result) {
-    CHECK(client_);
+    DCHECK(client_);
+    DCHECK(MessageLoop::current() == loop_);
     GError* error = NULL;
     // We want to distinguish unset values from values defaulting to
     // false. For that we need to use the type-generic
@@ -247,7 +310,8 @@ class GConfSettingGetterImpl
     return true;
   }
   virtual bool GetInt(const char* key, int* result) {
-    CHECK(client_);
+    DCHECK(client_);
+    DCHECK(MessageLoop::current() == loop_);
     GError* error = NULL;
     int value = gconf_client_get_int(client_, key, &error);
     if (HandleGError(error, key))
@@ -259,7 +323,8 @@ class GConfSettingGetterImpl
   }
   virtual bool GetStringList(const char* key,
                              std::vector<std::string>* result) {
-    CHECK(client_);
+    DCHECK(client_);
+    DCHECK(MessageLoop::current() == loop_);
     GError* error = NULL;
     GSList* list = gconf_client_get_list(client_, key,
                                          GCONF_VALUE_STRING, &error);
@@ -282,8 +347,8 @@ class GConfSettingGetterImpl
   // (error is NULL).
   bool HandleGError(GError* error, const char* key) {
     if (error != NULL) {
-      LOG(ERROR) << "ProxyConfigServiceLinux: error getting gconf value for "
-                 << key << ": " << error->message;
+      LOG(ERROR) << "Error getting gconf value for " << key
+                 << ": " << error->message;
       g_error_free(error);
       return true;
     }
@@ -292,12 +357,17 @@ class GConfSettingGetterImpl
 
   GConfClient* client_;
 
+  // Message loop of the thread that we make gconf calls on. It should
+  // be the UI thread and all our methods should be called on this
+  // thread. Only for assertions.
+  MessageLoop* loop_;
+
   DISALLOW_COPY_AND_ASSIGN(GConfSettingGetterImpl);
 };
 
 }  // namespace
 
-bool ProxyConfigServiceLinux::GetProxyFromGConf(
+bool ProxyConfigServiceLinux::Delegate::GetProxyFromGConf(
     const char* key_prefix, bool is_socks, ProxyServer* result_server) {
   std::string key(key_prefix);
   std::string host;
@@ -324,7 +394,8 @@ bool ProxyConfigServiceLinux::GetProxyFromGConf(
   return false;
 }
 
-bool ProxyConfigServiceLinux::GetConfigFromGConf(ProxyConfig* config) {
+bool ProxyConfigServiceLinux::Delegate::GetConfigFromGConf(
+    ProxyConfig* config) {
   std::string mode;
   if (!gconf_getter_->GetString("/system/proxy/mode", &mode)) {
     // We expect this to always be set, so if we don't see it then we
@@ -332,11 +403,12 @@ bool ProxyConfigServiceLinux::GetConfigFromGConf(ProxyConfig* config) {
     // proxy config.
     return false;
   }
-  if (mode.compare("none") == 0)
+  if (mode == "none") {
     // Specifically specifies no proxy.
     return true;
+  }
 
-  if (mode.compare("auto") == 0) {
+  if (mode == "auto") {
     // automatic proxy config
     std::string pac_url_str;
     if (gconf_getter_->GetString("/system/proxy/autoconfig_url",
@@ -353,7 +425,7 @@ bool ProxyConfigServiceLinux::GetConfigFromGConf(ProxyConfig* config) {
     return true;
   }
 
-  if (mode.compare("manual") != 0) {
+  if (mode != "manual") {
     // Mode is unrecognized.
     return false;
   }
@@ -419,8 +491,7 @@ bool ProxyConfigServiceLinux::GetConfigFromGConf(ProxyConfig* config) {
   gconf_getter_->GetBoolean("/system/http_proxy/use_authentication",
                             &use_auth);
   if (use_auth)
-    LOG(ERROR) << "ProxyConfigServiceLinux: proxy authentication "
-        "not supported";
+    LOG(ERROR) << "Proxy authentication not supported";
 
   // Now the bypass list.
   gconf_getter_->GetStringList("/system/http_proxy/ignore_hosts",
@@ -431,64 +502,160 @@ bool ProxyConfigServiceLinux::GetConfigFromGConf(ProxyConfig* config) {
   return true;
 }
 
-ProxyConfigServiceLinux::ProxyConfigServiceLinux(
+ProxyConfigServiceLinux::Delegate::Delegate(
     EnvironmentVariableGetter* env_var_getter,
     GConfSettingGetter* gconf_getter)
-    : env_var_getter_(env_var_getter), gconf_getter_(gconf_getter) {
-}
-
-ProxyConfigServiceLinux::ProxyConfigServiceLinux()
-    : env_var_getter_(new EnvironmentVariableGetterImpl()),
-      gconf_getter_(new GConfSettingGetterImpl()) {
+    : env_var_getter_(env_var_getter), gconf_getter_(gconf_getter),
+      glib_default_loop_(NULL), io_loop_(NULL) {
 }
 
-int ProxyConfigServiceLinux::GetProxyConfig(ProxyConfig* config) {
+bool ProxyConfigServiceLinux::Delegate::ShouldTryGConf() {
   // GNOME_DESKTOP_SESSION_ID being defined is a good indication that
   // we are probably running under GNOME.
   // Note: KDE_FULL_SESSION is a corresponding env var to recognize KDE.
   std::string dummy, desktop_session;
-  bool ok = false;
-#if 0  // gconf temporarily disabled because of races.
-       // See http://crbug.com/11442.
-  if (env_var_getter_->Getenv("GNOME_DESKTOP_SESSION_ID", &dummy)
+  return env_var_getter_->Getenv("GNOME_DESKTOP_SESSION_ID", &dummy)
       || (env_var_getter_->Getenv("DESKTOP_SESSION", &desktop_session)
-          && desktop_session.compare("gnome") == 0)) {
-    // Get settings from gconf.
-    //
-    // I (sdoyon) would have liked to prioritize environment variables
-    // and only fallback to gconf if env vars were unset. But
-    // gnome-terminal "helpfully" sets http_proxy and no_proxy, and it
-    // does so even if the proxy mode is set to auto, which would
-    // mislead us.
-    //
-    // We could introduce a CHROME_PROXY_OBEY_ENV_VARS variable...??
-    if (gconf_getter_->InitIfNeeded()) {
-      gconf_getter_->Enter();
-      ok = GetConfigFromGConf(config);
-      gconf_getter_->Leave();
-      if (ok)
-        LOG(INFO) << "ProxyConfigServiceLinux: obtained proxy setting "
-            "from gconf";
+          && desktop_session == "gnome");
+  // I (sdoyon) would have liked to prioritize environment variables
+  // and only fallback to gconf if env vars were unset. But
+  // gnome-terminal "helpfully" sets http_proxy and no_proxy, and it
+  // does so even if the proxy mode is set to auto, which would
+  // mislead us.
+  //
+  // We could introduce a CHROME_PROXY_OBEY_ENV_VARS variable...??
+}
+
+void ProxyConfigServiceLinux::Delegate::SetupAndFetchInitialConfig(
+    MessageLoop* glib_default_loop, MessageLoop* io_loop) {
+  // We should be running on the default glib main loop thread right
+  // now. gconf can only be accessed from this thread.
+  DCHECK(MessageLoop::current() == glib_default_loop);
+  glib_default_loop_ = glib_default_loop;
+  io_loop_ = io_loop;
+
+  // If we are passed a NULL io_loop, then we don't setup gconf
+  // notifications. This should not be the usual case but is intended
+  // to simplify test setups.
+  if (!io_loop_)
+    LOG(INFO) << "Monitoring of gconf setting changes is disabled";
+
+  // Fetch and cache the current proxy config. The config is left in
+  // cached_config_, where GetProxyConfig() running on the IO thread
+  // will expect to find it. This is safe to do because we return
+  // before this ProxyConfigServiceLinux is passed on to
+  // the ProxyService.
+  bool got_config = false;
+  if (ShouldTryGConf() &&
+      gconf_getter_->Init() &&
+      (!io_loop || gconf_getter_->SetupNotification(this))) {
+    if (GetConfigFromGConf(&cached_config_)) {
+      cached_config_.set_id(1);  // mark it as valid
+      got_config = true;
+      LOG(INFO) << "Obtained proxy setting from gconf";
       // If gconf proxy mode is "none", meaning direct, then we take
-      // that to be a valid config and will not check environment variables.
-      // The alternative would have been to look for a proxy whereever
-      // we can find one.
+      // that to be a valid config and will not check environment
+      // variables.  The alternative would have been to look for a proxy
+      // where ever we can find one.
       //
-      // TODO(sdoyon): Consider wiring in the gconf notification
-      // system. Cache this result config to return on subsequent calls,
-      // and only call GetConfigFromGConf() when we know things have
-      // actually changed.
+      // Keep a copy of the config for use from this thread for
+      // comparison with updated settings when we get notifications.
+      reference_config_ = cached_config_;
+      reference_config_.set_id(1);  // mark it as valid
+    } else {
+      gconf_getter_->Release();  // Stop notifications
+    }
+  }
+  if (!got_config) {
+    // An implementation for KDE settings would be welcome here.
+    //
+    // Consulting environment variables doesn't need to be done from
+    // the default glib main loop, but it's a tiny enough amount of
+    // work.
+    if (GetConfigFromEnv(&cached_config_)) {
+      cached_config_.set_id(1);  // mark it as valid
+      LOG(INFO) << "Obtained proxy setting from environment variables";
     }
   }
-#endif  // 0 (gconf disabled)
-  // An implementation for KDE settings would be welcome here.
-  if (!ok) {
-    ok = GetConfigFromEnv(config);
-    if (ok)
-      LOG(INFO) << "ProxyConfigServiceLinux: obtained proxy setting "
-          "from environment variables";
+}
+
+void ProxyConfigServiceLinux::Delegate::Reset() {
+  DCHECK(!glib_default_loop_ || MessageLoop::current() == glib_default_loop_);
+  gconf_getter_->Release();
+  cached_config_ = ProxyConfig();
+}
+
+int ProxyConfigServiceLinux::Delegate::GetProxyConfig(ProxyConfig* config) {
+  // This is called from the IO thread.
+  DCHECK(!io_loop_ || MessageLoop::current() == io_loop_);
+
+  // Simply return the last proxy configuration that glib_default_loop
+  // notified us of.
+  *config = cached_config_;
+  return cached_config_.is_valid() ? OK : ERR_FAILED;
+}
+
+void ProxyConfigServiceLinux::Delegate::OnCheckProxyConfigSettings() {
+  // This should be dispatched from the thread with the default glib
+  // main loop, which allows us to access gconf.
+  DCHECK(MessageLoop::current() == glib_default_loop_);
+
+  ProxyConfig new_config;
+  bool valid = GetConfigFromGConf(&new_config);
+  if (valid)
+    new_config.set_id(1);  // mark it as valid
+
+  // See if it is different than what we had before.
+  if (new_config.is_valid() != reference_config_.is_valid() ||
+      !new_config.Equals(reference_config_)) {
+    // Post a task to |io_loop| with the new configuration, so it can
+    // update |cached_config_|.
+    io_loop_->PostTask(
+        FROM_HERE,
+        NewRunnableMethod(
+            this,
+            &ProxyConfigServiceLinux::Delegate::SetNewProxyConfig,
+            new_config));
   }
-  return ok ? OK : ERR_FAILED;
+}
+
+void ProxyConfigServiceLinux::Delegate::SetNewProxyConfig(
+    const ProxyConfig& new_config) {
+  DCHECK(MessageLoop::current() == io_loop_);
+  LOG(INFO) << "Proxy configuration changed";
+  cached_config_ = new_config;
+}
+
+void ProxyConfigServiceLinux::Delegate::PostDestroyTask() {
+  if (MessageLoop::current() == glib_default_loop_) {
+    // Already on the right thread, call directly.
+    // This is the case for the unittests.
+    OnDestroy();
+  } else {
+    // Post to UI thread. Note that on browser shutdown, we may quit
+    // the UI MessageLoop and exit the program before ever running
+    // this.
+    glib_default_loop_->PostTask(
+        FROM_HERE,
+        NewRunnableMethod(
+            this,
+            &ProxyConfigServiceLinux::Delegate::OnDestroy));
+  }
+}
+void ProxyConfigServiceLinux::Delegate::OnDestroy() {
+  DCHECK(!glib_default_loop_ || MessageLoop::current() == glib_default_loop_);
+  gconf_getter_->Release();
+}
+
+ProxyConfigServiceLinux::ProxyConfigServiceLinux()
+    : delegate_(new Delegate(new EnvironmentVariableGetterImpl(),
+                             new GConfSettingGetterImpl())) {
+}
+
+ProxyConfigServiceLinux::ProxyConfigServiceLinux(
+    EnvironmentVariableGetter* env_var_getter,
+    GConfSettingGetter* gconf_getter)
+    : delegate_(new Delegate(env_var_getter, gconf_getter)) {
 }
 
 }  // namespace net
diff --git a/net/proxy/proxy_config_service_linux.h b/net/proxy/proxy_config_service_linux.h
index 354bd29..2788731 100644
--- a/net/proxy/proxy_config_service_linux.h
+++ b/net/proxy/proxy_config_service_linux.h
@@ -9,7 +9,10 @@
 #include <vector>
 
 #include "base/basictypes.h"
+#include "base/message_loop.h"
+#include "base/ref_counted.h"
 #include "base/scoped_ptr.h"
+#include "net/proxy/proxy_config.h"
 #include "net/proxy/proxy_config_service.h"
 #include "net/proxy/proxy_server.h"
 
@@ -33,25 +36,24 @@ class ProxyConfigServiceLinux : public ProxyConfigService {
    public:
     virtual ~GConfSettingGetter() {}
 
-    // GDK/GTK+ thread-safety: multi-threaded programs coordinate
-    // around a global lock. See
-    // http://library.gnome.org/devel/gdk/unstable/gdk-Threads.html
-    // and http://research.operationaldynamics.com/blogs/andrew/software/gnome-desktop/gtk-thread-awareness.html
-    // The following methods are used to grab said lock around any
-    // actual gconf usage: including calls to Init() and any of the
-    // Get* methods.
-    virtual void Enter() = 0;
-    virtual void Leave() = 0;
-
     // Initializes the class: obtains a gconf client, in the concrete
     // implementation. Returns true on success. Must be called before
-    // any of the Get* methods, and the Get* methods must not be used
-    // if this method returns false. This method may be called again,
-    // whether or not it succeeded before.
-    virtual bool InitIfNeeded() = 0;
-
-    // Gets a string type value from gconf and stores it in result. Returns
-    // false if the key is unset or on error.
+    // using other methods.
+    virtual bool Init() = 0;
+
+    // Releases the gconf client, which clears cached directories and
+    // stops notifications.
+    virtual void Release() = 0;
+
+    // Requests notification of gconf setting changes for proxy
+    // settings. Returns true on success.
+    virtual bool SetupNotification(void* callback_user_data) = 0;
+
+    // Gets a string type value from gconf and stores it in
+    // result. Returns false if the key is unset or on error. Must
+    // only be called after a successful call to Init(), and not
+    // after a failed call to SetupNotification() or after calling
+    // Release().
     virtual bool GetString(const char* key, std::string* result) = 0;
     // Same thing for a bool typed value.
     virtual bool GetBoolean(const char* key, bool* result) = 0;
@@ -62,41 +64,154 @@ class ProxyConfigServiceLinux : public ProxyConfigService {
                                std::vector<std::string>* result) = 0;
   };
 
+  // ProxyConfigServiceLinux is created on the UI thread, and
+  // SetupAndFetchInitialConfig() is immediately called to
+  // synchronously fetch the original configuration and setup gconf
+  // notifications on the UI thread.
+  //
+  // Passed that point, it is accessed periodically through
+  // GetProxyConfig() from the IO thread.
+  //
+  // gconf change notification callbacks can occur at any time and are
+  // run on the UI thread. The new gconf settings are fetched on the
+  // UI thread, and the new resulting proxy config is posted to the IO
+  // thread through Delegate::SetNewProxyConfig().
+  //
+  // ProxyConfigServiceLinux is deleted from the IO thread.
+  //
+  // The substance of the ProxyConfigServiceLinux implementation is
+  // wrapped in the Delegate ref counted class. On deleting the
+  // ProxyConfigServiceLinux, Delegate::OnDestroy() is posted to the
+  // UI thread where gconf notifications will be safely stopped before
+  // releasing Delegate.
+
+  class Delegate : public base::RefCountedThreadSafe<Delegate> {
+   public:
+    // Constructor receives gconf and env var getter implementations
+    // to use, and takes ownership of them.
+    Delegate(EnvironmentVariableGetter* env_var_getter,
+             GConfSettingGetter* gconf_getter);
+    // Synchronously obtains the proxy configuration. If gconf is
+    // used, also enables gconf notification for setting
+    // changes. gconf must only be accessed from the thread running
+    // the default glib main loop, and so this method must be called
+    // from the UI thread. The message loop for the IO thread is
+    // specified so that notifications can post tasks to it (and for
+    // assertions).
+    void SetupAndFetchInitialConfig(MessageLoop* glib_default_loop,
+                                    MessageLoop* io_loop);
+    // Resets cached_config_ and releases the gconf_getter_, making it
+    // possible to call SetupAndFetchInitialConfig() again. Only used
+    // in testing.
+    void Reset();
+
+    // Handler for gconf change notifications: fetches a new proxy
+    // configuration from gconf settings, and if this config is
+    // different than what we had before, posts a task to have it
+    // stored in cached_config_.
+    // Left public for simplicity.
+    void OnCheckProxyConfigSettings();
+
+    // Called from IO thread.
+    int GetProxyConfig(ProxyConfig* config);
+
+    // Posts a call to OnDestroy() to the UI thread. Called from
+    // ProxyConfigServiceLinux's destructor.
+    void PostDestroyTask();
+    // Safely stops gconf notifications. Posted to the UI thread.
+    void OnDestroy();
+
+   private:
+    // Obtains an environment variable's value. Parses a proxy server
+    // specification from it and puts it in result. Returns true if the
+    // requested variable is defined and the value valid.
+    bool GetProxyFromEnvVarForScheme(const char* variable,
+                                     ProxyServer::Scheme scheme,
+                                     ProxyServer* result_server);
+    // As above but with scheme set to HTTP, for convenience.
+    bool GetProxyFromEnvVar(const char* variable, ProxyServer* result_server);
+    // Fills proxy config from environment variables. Returns true if
+    // variables were found and the configuration is valid.
+    bool GetConfigFromEnv(ProxyConfig* config);
+
+    // Obtains host and port gconf settings and parses a proxy server
+    // specification from it and puts it in result. Returns true if the
+    // requested variable is defined and the value valid.
+    bool GetProxyFromGConf(const char* key_prefix, bool is_socks,
+                           ProxyServer* result_server);
+    // Fills proxy config from gconf. Returns true if settings were found
+    // and the configuration is valid.
+    bool GetConfigFromGConf(ProxyConfig* config);
+
+    // Returns true if environment variables indicate that we are
+    // running GNOME (and therefore we want to use gconf settings).
+    bool ShouldTryGConf();
+
+    // This method is posted from the UI thread to the IO thread to
+    // carry the new config information.
+    void SetNewProxyConfig(const ProxyConfig& new_config);
+
+    scoped_ptr<EnvironmentVariableGetter> env_var_getter_;
+    scoped_ptr<GConfSettingGetter> gconf_getter_;
+
+    // Cached proxy configuration, to be returned by
+    // GetProxyConfig. Initially populated from the UI thread, but
+    // afterwards only accessed from the IO thread.
+    ProxyConfig cached_config_;
+
+    // A copy kept on the UI thread of the last seen proxy config, so as
+    // to avoid posting a call to SetNewProxyConfig when we get a
+    // notification but the config has not actually changed.
+    ProxyConfig reference_config_;
+
+    // The MessageLoop for the UI thread, aka main browser thread. This
+    // thread is where we run the glib main loop (see
+    // base/message_pump_glib.h). It is the glib default loop in the
+    // sense that it runs the glib default context: as in the context
+    // where sources are added by g_timeout_add and g_idle_add, and
+    // returned by g_main_context_default. gconf uses glib timeouts and
+    // idles and possibly other callbacks that will all be dispatched on
+    // this thread. Since gconf is not thread safe, any use of gconf
+    // must be done on the thread running this loop.
+    MessageLoop* glib_default_loop_;
+    // MessageLoop for the IO thread. GetProxyConfig() is called from
+    // the thread running this loop.
+    MessageLoop* io_loop_;
+
+    DISALLOW_COPY_AND_ASSIGN(Delegate);
+  };
+
+  // Thin wrapper shell around Delegate.
+
   // Usual constructor
   ProxyConfigServiceLinux();
-  // For testing: pass in alternate gconf and env var getter implementations.
-  // ProxyConfigServiceLinux takes ownership of the getter objects.
+  // For testing: takes alternate gconf and env var getter implementations.
   ProxyConfigServiceLinux(EnvironmentVariableGetter* env_var_getter,
                           GConfSettingGetter* gconf_getter);
 
+  virtual ~ProxyConfigServiceLinux() {
+    delegate_->PostDestroyTask();
+  }
+
+  void SetupAndFetchInitialConfig(MessageLoop* glib_default_loop,
+                                  MessageLoop* io_loop) {
+    delegate_->SetupAndFetchInitialConfig(glib_default_loop, io_loop);
+  }
+  void Reset() {
+    delegate_->Reset();
+  }
+  void OnCheckProxyConfigSettings() {
+    delegate_->OnCheckProxyConfigSettings();
+  }
+
   // ProxyConfigService methods:
-  virtual int GetProxyConfig(ProxyConfig* config);
+  // Called from IO thread.
+  virtual int GetProxyConfig(ProxyConfig* config) {
+    return delegate_->GetProxyConfig(config);
+  }
 
  private:
-  // Obtains an environment variable's value. Parses a proxy server
-  // specification from it and puts it in result. Returns true if the
-  // requested variable is defined and the value valid.
-  bool GetProxyFromEnvVarForScheme(const char* variable,
-                                   ProxyServer::Scheme scheme,
-                                   ProxyServer* result_server);
-  // As above but with scheme set to HTTP, for convenience.
-  bool GetProxyFromEnvVar(const char* variable, ProxyServer* result_server);
-
-  // Fills proxy config from environment variables. Returns true if
-  // variables were found and the configuration is valid.
-  bool GetConfigFromEnv(ProxyConfig* config);
-
-  // Obtains host and port gconf settings and parses a proxy server
-  // specification from it and puts it in result. Returns true if the
-  // requested variable is defined and the value valid.
-  bool GetProxyFromGConf(const char* key_prefix, bool is_socks,
-                         ProxyServer* result_server);
-  // Fills proxy config from gconf. Returns true if settings were found
-  // and the configuration is valid.
-  bool GetConfigFromGConf(ProxyConfig* config);
-
-  scoped_ptr<EnvironmentVariableGetter> env_var_getter_;
-  scoped_ptr<GConfSettingGetter> gconf_getter_;
+  scoped_refptr<Delegate> delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(ProxyConfigServiceLinux);
 };
diff --git a/net/proxy/proxy_config_service_linux_unittest.cc b/net/proxy/proxy_config_service_linux_unittest.cc
index 035e846..25697ab 100644
--- a/net/proxy/proxy_config_service_linux_unittest.cc
+++ b/net/proxy/proxy_config_service_linux_unittest.cc
@@ -10,13 +10,15 @@
 
 #include "base/logging.h"
 #include "base/string_util.h"
+#include "base/task.h"
+#include "base/thread.h"
+#include "base/waitable_event.h"
 #include "net/proxy/proxy_config.h"
 #include "net/proxy/proxy_config_service_common_unittest.h"
 
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace net {
-
 namespace {
 
 // Set of values for all environment variables that we might
@@ -31,6 +33,10 @@ struct EnvVarValues {
       *no_proxy;
 };
 
+// Undo macro pollution from GDK includes (from message_loop.h).
+#undef TRUE
+#undef FALSE
+
 // So as to distinguish between an unset gconf boolean variable and
 // one that is false.
 enum BoolSettingValue {
@@ -84,11 +90,11 @@ class MockEnvironmentVariableGetter
     ENTRY(SOCKS_SERVER);
     ENTRY(SOCKS_VERSION);
 #undef ENTRY
-    reset();
+    Reset();
   }
 
   // Zeros all environment values.
-  void reset() {
+  void Reset() {
     EnvVarValues zero_values = { 0 };
     values = zero_values;
   }
@@ -138,19 +144,22 @@ class MockGConfSettingGetter
 #undef ENTRY
     string_lists_table.settings["/system/http_proxy/ignore_hosts"] =
       &values.ignore_hosts;
-    reset();
+    Reset();
   }
 
   // Zeros all environment values.
-  void reset() {
-    GConfValues zero_values;
+  void Reset() {
+    GConfValues zero_values = { 0 };
     values = zero_values;
   }
 
-  virtual void Enter() {}
-  virtual void Leave() {}
+  virtual bool Init() {
+    return true;
+  }
+
+  virtual void Release() {}
 
-  virtual bool InitIfNeeded() {
+  virtual bool SetupNotification(void* callback_user_data) {
     return true;
   }
 
@@ -201,11 +210,99 @@ class MockGConfSettingGetter
 };
 
 }  // namespace
+}  // namespace net
+
+// This helper class runs ProxyConfigServiceLinux::GetProxyConfig() on
+// the IO thread and synchronously waits for the result.
+// Some code duplicated from proxy_script_fetcher_unittest.cc.
+class SynchConfigGetter {
+ public:
+  explicit SynchConfigGetter(net::ProxyConfigServiceLinux* config_service)
+      : event_(false, false),
+        io_thread_("IO_Thread"),
+        config_service_(config_service) {
+    // Start an IO thread.
+    base::Thread::Options options;
+    options.message_loop_type = MessageLoop::TYPE_IO;
+    io_thread_.StartWithOptions(options);
+
+    // Make sure the thread started.
+    io_thread_.message_loop()->PostTask(FROM_HERE, NewRunnableMethod(
+        this, &SynchConfigGetter::Init));
+    Wait();
+  }
+
+  ~SynchConfigGetter() {
+    // Cleanup the IO thread.
+    io_thread_.message_loop()->PostTask(FROM_HERE, NewRunnableMethod(
+        this, &SynchConfigGetter::Cleanup));
+    Wait();
+  }
+
+  // Does a reset, gconf setup and initial fetch of the proxy config,
+  // all on the calling thread (meant to be the thread with the
+  // default glib main loop, which is the UI thread).
+  void SetupAndInitialFetch() {
+    config_service_->Reset();
+    config_service_->SetupAndFetchInitialConfig(
+        MessageLoop::current(), io_thread_.message_loop());
+  }
+  // Synchronously gets the proxy config.
+  int SyncGetProxyConfig(net::ProxyConfig* config) {
+    io_thread_.message_loop()->PostTask(FROM_HERE, NewRunnableMethod(
+        this, &SynchConfigGetter::GetConfigOnIOThread));
+    Wait();
+    *config = proxy_config_;
+    return get_config_result_;
+  }
+
+ private:
+  // [Runs on |io_thread_|]
+  void Init() {
+    event_.Signal();
+  }
+
+  // Calls GetProxyConfig, running on |io_thread_|] Signals |event_|
+  // on completion.
+  void GetConfigOnIOThread() {
+    get_config_result_ = config_service_->GetProxyConfig(&proxy_config_);
+    event_.Signal();
+  }
+
+  // [Runs on |io_thread_|] Signals |event_| on cleanup completion.
+  void Cleanup() {
+    MessageLoop::current()->RunAllPending();
+    event_.Signal();
+  }
+
+  void Wait() {
+    event_.Wait();
+    event_.Reset();
+  }
+
+  base::WaitableEvent event_;
+  base::Thread io_thread_;
+
+  net::ProxyConfigServiceLinux* config_service_;
+
+  // The config obtained by |io_thread_| and read back by the main
+  // thread.
+  net::ProxyConfig proxy_config_;
+  int get_config_result_;  // Return value from GetProxyConfig().
+};
+
+template<>
+void RunnableMethodTraits<SynchConfigGetter>::RetainCallee(
+    SynchConfigGetter* remover) {}
+template<>
+void RunnableMethodTraits<SynchConfigGetter>::ReleaseCallee(
+    SynchConfigGetter* remover) {}
+
+namespace net {
 
 // Builds an identifier for each test in an array.
 #define TEST_DESC(desc) StringPrintf("at line %d <%s>", __LINE__, desc)
 
-#if 0  // gconf temporarily disabled.
 TEST(ProxyConfigServiceLinuxTest, BasicGConfTest) {
   MockEnvironmentVariableGetter* env_getter =
       new MockEnvironmentVariableGetter;
@@ -450,15 +547,15 @@ TEST(ProxyConfigServiceLinuxTest, BasicGConfTest) {
     },
   };
 
+  SynchConfigGetter sync_config_getter(&service);
+
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
     SCOPED_TRACE(StringPrintf("Test[%d] %s", i, tests[i].description.c_str()));
     ProxyConfig config;
     gconf_getter->values = tests[i].values;
-    service.GetProxyConfig(&config);
+    sync_config_getter.SetupAndInitialFetch();
+    sync_config_getter.SyncGetProxyConfig(&config);
 
-    // TODO(sdoyon): Add a description field to each test, and a
-    // corresponding message to the EXPECT statements, so that it is
-    // possible to identify which one of the tests failed.
     EXPECT_EQ(tests[i].auto_detect, config.auto_detect);
     EXPECT_EQ(tests[i].pac_url, config.pac_url);
     EXPECT_EQ(tests[i].proxy_bypass_list,
@@ -467,7 +564,6 @@ TEST(ProxyConfigServiceLinuxTest, BasicGConfTest) {
     EXPECT_EQ(tests[i].proxy_rules, config.proxy_rules);
   }
 }
-#endif  // 0 (gconf disabled)
 
 TEST(ProxyConfigServiceLinuxTest, BasicEnvTest) {
   MockEnvironmentVariableGetter* env_getter =
@@ -720,11 +816,14 @@ TEST(ProxyConfigServiceLinuxTest, BasicEnvTest) {
     },
   };
 
+  SynchConfigGetter sync_config_getter(&service);
+
   for (size_t i = 0; i < ARRAYSIZE_UNSAFE(tests); ++i) {
     SCOPED_TRACE(StringPrintf("Test[%d] %s", i, tests[i].description.c_str()));
     ProxyConfig config;
     env_getter->values = tests[i].values;
-    service.GetProxyConfig(&config);
+    sync_config_getter.SetupAndInitialFetch();
+    sync_config_getter.SyncGetProxyConfig(&config);
 
     EXPECT_EQ(tests[i].auto_detect, config.auto_detect);
     EXPECT_EQ(tests[i].pac_url, config.pac_url);
@@ -753,10 +852,38 @@ TEST(ProxyConfigServiceLinuxTest, FallbackOnEnv) {
   env_getter->values.auto_proxy = "http://correct/wpad.dat";
 
   ProxyConfig config;
-  service.GetProxyConfig(&config);
+
+  SynchConfigGetter sync_config_getter(&service);
+  sync_config_getter.SetupAndInitialFetch();
+  sync_config_getter.SyncGetProxyConfig(&config);
 
   // Then we expect the environment variable to win.
   EXPECT_EQ(GURL(env_getter->values.auto_proxy), config.pac_url);
 }
 
+TEST(ProxyConfigServiceLinuxTest, GconfNotification) {
+  MockEnvironmentVariableGetter* env_getter =
+      new MockEnvironmentVariableGetter;
+  MockGConfSettingGetter* gconf_getter = new MockGConfSettingGetter;
+  ProxyConfigServiceLinux service(env_getter, gconf_getter);
+  ProxyConfig config;
+  SynchConfigGetter sync_config_getter(&service);
+
+  // Use gconf configuration.
+  env_getter->values.GNOME_DESKTOP_SESSION_ID = "defined";
+
+  // Start with no proxy.
+  gconf_getter->values.mode = "none";
+  sync_config_getter.SetupAndInitialFetch();
+  sync_config_getter.SyncGetProxyConfig(&config);
+  EXPECT_FALSE(config.auto_detect);
+
+  // Now set to auto-detect.
+  gconf_getter->values.mode = "auto";
+  // Simulate gconf notification callback.
+  service.OnCheckProxyConfigSettings();
+  sync_config_getter.SyncGetProxyConfig(&config);
+  EXPECT_TRUE(config.auto_detect);
+}
+
 }  // namespace net
diff --git a/net/proxy/proxy_script_fetcher_unittest.cc b/net/proxy/proxy_script_fetcher_unittest.cc
index e1c4179..7d0cb83 100644
--- a/net/proxy/proxy_script_fetcher_unittest.cc
+++ b/net/proxy/proxy_script_fetcher_unittest.cc
@@ -28,7 +28,7 @@ class RequestContext : public URLRequestContext {
  public:
   RequestContext() {
     net::ProxyConfig no_proxy;
-    proxy_service_ = net::ProxyService::Create(&no_proxy);
+    proxy_service_ = net::ProxyService::CreateFixed(no_proxy);
     http_transaction_factory_ = net::HttpNetworkLayer::CreateFactory(
         proxy_service_);
   }
diff --git a/net/proxy/proxy_service.cc b/net/proxy/proxy_service.cc
index 0c2828b..a827389 100644
--- a/net/proxy/proxy_service.cc
+++ b/net/proxy/proxy_service.cc
@@ -207,36 +207,38 @@ ProxyService::ProxyService(ProxyConfigService* config_service,
 }
 
 // static
-ProxyService* ProxyService::Create(const ProxyConfig* pc) {
+ProxyService* ProxyService::Create(
+    const ProxyConfig* pc,
+    bool use_v8_resolver,
+    URLRequestContext* url_request_context,
+    MessageLoop* io_loop) {
   // Choose the system configuration service appropriate for each platform.
   ProxyConfigService* proxy_config_service = pc ?
       new ProxyConfigServiceFixed(*pc) :
-      CreateSystemProxyConfigService();
+      CreateSystemProxyConfigService(io_loop);
 
-  // Try to choose a non-v8 proxy resolver implementation.
-  return new ProxyService(proxy_config_service, CreateNonV8ProxyResolver());
-}
+  ProxyResolver* proxy_resolver = use_v8_resolver ?
+      new ProxyResolverV8() : CreateNonV8ProxyResolver();
 
-// static
-ProxyService* ProxyService::CreateUsingV8Resolver(
-    const ProxyConfig* pc, URLRequestContext* url_request_context) {
-  // Choose the system configuration service appropriate for each platform.
-  ProxyConfigService* proxy_config_service = pc ?
-      new ProxyConfigServiceFixed(*pc) :
-      CreateSystemProxyConfigService();
-
-  // Create a ProxyService that uses V8 to evaluate PAC scripts.
   ProxyService* proxy_service = new ProxyService(
-      proxy_config_service, new ProxyResolverV8());
+      proxy_config_service, proxy_resolver);
 
-  // Configure PAC script downloads to be issued using |url_request_context|.
-  proxy_service->SetProxyScriptFetcher(
-      ProxyScriptFetcher::Create(url_request_context));
+  if (!proxy_resolver->does_fetch()) {
+    // Configure PAC script downloads to be issued using |url_request_context|.
+    DCHECK(url_request_context);
+    proxy_service->SetProxyScriptFetcher(
+        ProxyScriptFetcher::Create(url_request_context));
+  }
 
   return proxy_service;
 }
 
 // static
+ProxyService* ProxyService::CreateFixed(const ProxyConfig& pc) {
+  return Create(&pc, false, NULL, NULL);
+}
+
+// static
 ProxyService* ProxyService::CreateNull() {
   // Use a configuration fetcher and proxy resolver which always fail.
   return new ProxyService(new ProxyConfigServiceNull, new ProxyResolverNull);
@@ -529,13 +531,28 @@ void ProxyService::DidCompletePacRequest(int config_id, int result_code) {
 }
 
 // static
-ProxyConfigService* ProxyService::CreateSystemProxyConfigService() {
+ProxyConfigService* ProxyService::CreateSystemProxyConfigService(
+    MessageLoop* io_loop) {
 #if defined(OS_WIN)
   return new ProxyConfigServiceWin();
 #elif defined(OS_MACOSX)
   return new ProxyConfigServiceMac();
 #elif defined(OS_LINUX)
-  return new ProxyConfigServiceLinux();
+  ProxyConfigServiceLinux* linux_config_service
+      = new ProxyConfigServiceLinux();
+
+  // Assume we got called from the UI loop, which runs the default
+  // glib main loop, so the current thread is where we should be
+  // running gconf calls from.
+  MessageLoop* glib_default_loop = MessageLoopForUI::current();
+
+  // Synchronously fetch the current proxy config (since we are
+  // running on glib_default_loop). Additionally register for gconf
+  // notifications (delivered in |glib_default_loop|) to keep us
+  // updated on when the proxy config has changed.
+  linux_config_service->SetupAndFetchInitialConfig(glib_default_loop, io_loop);
+
+  return linux_config_service;
 #else
   LOG(WARNING) << "Failed to choose a system proxy settings fetcher "
                   "for this platform.";
diff --git a/net/proxy/proxy_service.h b/net/proxy/proxy_service.h
index 4227586..dc0c455 100644
--- a/net/proxy/proxy_service.h
+++ b/net/proxy/proxy_service.h
@@ -89,21 +89,29 @@ class ProxyService {
   // Creates a proxy service using the specified settings. If |pc| is NULL then
   // the system's default proxy settings will be used (on Windows this will
   // use IE's settings).
-  static ProxyService* Create(const ProxyConfig* pc);
-
-  // Creates a proxy service using the specified settings. If |pc| is NULL then
-  // the system's default proxy settings will be used. This is basically the
-  // same as Create(const ProxyConfig*), however under the hood it uses a
-  // different implementation (V8). |url_request_context| is the URL request
-  // context that will be used if a PAC script needs to be fetched.
+  // Iff |use_v8_resolver| is true, then the V8 implementation is
+  // used.
+  // |url_request_context| is only used when use_v8_resolver is true:
+  // it specifies the URL request context that will be used if a PAC
+  // script needs to be fetched.
+  // |io_loop| points to the IO thread's message loop. It is only used
+  // when pc is NULL. If both pc and io_loop are NULL, then monitoring
+  // of gconf setting changes will be disabled in
+  // ProxyConfigServiceLinux.
   // ##########################################################################
   // # See the warnings in net/proxy/proxy_resolver_v8.h describing the
   // # multi-threading model. In order for this to be safe to use, *ALL* the
   // # other V8's running in the process must use v8::Locker.
   // ##########################################################################
-  static ProxyService* CreateUsingV8Resolver(
+  static ProxyService* Create(
       const ProxyConfig* pc,
-      URLRequestContext* url_request_context);
+      bool use_v8_resolver,
+      URLRequestContext* url_request_context,
+      MessageLoop* io_loop);
+
+  // Convenience method that creates a proxy service using the
+  // specified fixed settings. |pc| must not be NULL.
+  static ProxyService* CreateFixed(const ProxyConfig& pc);
 
   // Creates a proxy service that always fails to fetch the proxy configuration,
   // so it falls back to direct connect.
@@ -114,7 +122,8 @@ class ProxyService {
 
   // Creates a config service appropriate for this platform that fetches the
   // system proxy settings.
-  static ProxyConfigService* CreateSystemProxyConfigService();
+  static ProxyConfigService* CreateSystemProxyConfigService(
+      MessageLoop* io_loop);
 
   // Creates a proxy resolver appropriate for this platform that doesn't rely
   // on V8.