Linux: fix bug 75508, a crash when receiving notifications of proxy settings changes.

This ended up being a use-after-free bug after all, as the memory allocator was
overwriting the sentinel value I was storing to indicate this, so I had
previously thought it was stray memory corruption. The problem was that the
GConf library was returning the same GConf client object multiple times, and
merely increasing the reference count. That's fine except that we were counting
on being able to stop incoming notifications by unreferencing it; however, this
only works when there is only one reference. So, as soon as you ended an
incognito session, any further changes to proxy settings would cause a crash.

We now explicitly stop the notifications before unreferencing the client.

BUG=75508
TEST=changing proxy settings after opening and closing an incognito window does not crash

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

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

1 files changed, 35 insertions, 30 deletions

diff --git a/net/proxy/proxy_config_service_linux.cc b/net/proxy/proxy_config_service_linux.cc
index cc27e47..1ac2f09 100644
--- a/net/proxy/proxy_config_service_linux.cc
+++ b/net/proxy/proxy_config_service_linux.cc
@@ -200,15 +200,16 @@ const int kDebounceTimeoutMilliseconds = 250;
 class SettingGetterImplGConf : public ProxyConfigServiceLinux::SettingGetter {
  public:
   SettingGetterImplGConf()
-      : this_(this), client_(NULL), notify_delegate_(NULL), loop_(NULL) {}
+      : client_(NULL), system_proxy_id_(0), system_http_proxy_id_(0),
+        notify_delegate_(NULL), loop_(NULL) {
+  }
 
   virtual ~SettingGetterImplGConf() {
     // client_ should have been released before now, from
     // Delegate::OnDestroy(), while running on the UI thread. However
-    // on exiting the process, it may happen that
-    // Delegate::OnDestroy() task is left pending on the glib loop
-    // after the loop was quit, and pending tasks may then be deleted
-    // without being run.
+    // on exiting the process, it may happen that Delegate::OnDestroy()
+    // task is left pending on the glib loop after the loop was quit,
+    // and pending tasks may then be deleted without being run.
     if (client_) {
       // gconf client was not cleaned up.
       if (MessageLoop::current() == loop_) {
@@ -227,14 +228,6 @@ class SettingGetterImplGConf : public ProxyConfigServiceLinux::SettingGetter {
       }
     }
     DCHECK(!client_);
-    // Invert the bits of |this_|. This seems like a pretty unlikely coincidence
-    // for stray memory corruption, so if we see this later, it's a pretty sure
-    // bet that we have a use-after-free issue rather than external corruption.
-    // (Note that we will still have the original value of |this| in that case.)
-    // See below. This is to try and track bugs 75508 and 84673.
-    // TODO(mdm): remove this once it gives us some results.
-    this_ = reinterpret_cast<SettingGetterImplGConf*>(
-        ~reinterpret_cast<uintptr_t>(this));
   }
 
   virtual bool Init(MessageLoop* glib_default_loop,
@@ -251,18 +244,26 @@ class SettingGetterImplGConf : public ProxyConfigServiceLinux::SettingGetter {
       return false;
     }
     GError* error = NULL;
+    bool added_system_proxy = false;
     // We need to add the directories for which we'll be asking
-    // notifications, and we might as well ask to preload them.
+    // for notifications, and we might as well ask to preload them.
+    // These need to be removed again in ShutDown(); we are careful
+    // here to only leave client_ non-NULL if both have been added.
     gconf_client_add_dir(client_, "/system/proxy",
                          GCONF_CLIENT_PRELOAD_ONELEVEL, &error);
     if (error == NULL) {
+      added_system_proxy = true;
       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);
-      ShutDown();
+      if (added_system_proxy)
+        gconf_client_remove_dir(client_, "/system/proxy", NULL);
+      g_object_unref(client_);
+      client_ = NULL;
+      loop_ = NULL;
       return false;
     }
     return true;
@@ -271,7 +272,14 @@ class SettingGetterImplGConf : public ProxyConfigServiceLinux::SettingGetter {
   void ShutDown() {
     if (client_) {
       DCHECK(MessageLoop::current() == loop_);
-      // This also disables gconf notifications.
+      // We must explicitly disable gconf notifications here, because the gconf
+      // client will be shared between all setting getters, and they do not all
+      // have the same lifetimes. (For instance, incognito sessions get their
+      // own, which is destroyed when the session ends.)
+      gconf_client_notify_remove(client_, system_http_proxy_id_);
+      gconf_client_notify_remove(client_, system_proxy_id_);
+      gconf_client_remove_dir(client_, "/system/http_proxy", NULL);
+      gconf_client_remove_dir(client_, "/system/proxy", NULL);
       g_object_unref(client_);
       client_ = NULL;
       loop_ = NULL;
@@ -283,12 +291,15 @@ class SettingGetterImplGConf : public ProxyConfigServiceLinux::SettingGetter {
     DCHECK(MessageLoop::current() == loop_);
     GError* error = NULL;
     notify_delegate_ = delegate;
-    gconf_client_notify_add(
+    // We have to keep track of the IDs returned by gconf_client_notify_add() so
+    // that we can remove them in ShutDown(). (Otherwise, notifications will be
+    // delivered to this object after it is deleted, which is bad, m'kay?)
+    system_proxy_id_ = gconf_client_notify_add(
         client_, "/system/proxy",
         OnGConfChangeNotification, this,
         NULL, &error);
     if (error == NULL) {
-      gconf_client_notify_add(
+      system_http_proxy_id_ = gconf_client_notify_add(
           client_, "/system/http_proxy",
           OnGConfChangeNotification, this,
           NULL, &error);
@@ -459,11 +470,6 @@ class SettingGetterImplGConf : public ProxyConfigServiceLinux::SettingGetter {
   }
 
   void OnChangeNotification() {
-    // See below. This check is to try and track bugs 75508 and 84673.
-    // TODO(mdm): remove these checks once they give us some results.
-    CHECK_NE(this_, reinterpret_cast<SettingGetterImplGConf*>(
-        ~reinterpret_cast<uintptr_t>(this)));
-    CHECK_EQ(this_, this);
     // We don't use Reset() because the timer may not yet be running.
     // (In that case Stop() is a no-op.)
     debounce_timer_.Stop();
@@ -483,13 +489,12 @@ class SettingGetterImplGConf : public ProxyConfigServiceLinux::SettingGetter {
     setting_getter->OnChangeNotification();
   }
 
-  // See bugs 75508 and 84673. I kind of suspect we're getting bogus pointers to
-  // this object somehow in callbacks from GConf; this pointer is always set to
-  // |this| on construction and should help verify whether this object has been
-  // scribbled upon or if we have a stray pointer somehow.
-  SettingGetterImplGConf* this_;
-
   GConfClient* client_;
+  // These ids are the values returned from gconf_client_notify_add(), which we
+  // will need in order to later call gconf_client_notify_remove().
+  guint system_proxy_id_;
+  guint system_http_proxy_id_;
+
   ProxyConfigServiceLinux::Delegate* notify_delegate_;
   base::OneShotTimer<SettingGetterImplGConf> debounce_timer_;
 
@@ -1030,7 +1035,7 @@ class SettingGetterImplKDE : public ProxyConfigServiceLinux::SettingGetter,
     return file_loop_;
   }
 
-  // Implement base::MessagePumpLibevent::Delegate.
+  // Implement base::MessagePumpLibevent::Watcher.
   void OnFileCanReadWithoutBlocking(int fd) {
     DCHECK_EQ(fd, inotify_fd_);
     DCHECK(MessageLoop::current() == file_loop_);