Move ProxyConfigService construction onto the IO thread.

net/proxy/proxy_config_service_linux.cc

Index: net/proxy/proxy_config_service_linux.cc
diff --git a/net/proxy/proxy_config_service_linux.cc b/net/proxy/proxy_config_service_linux.cc
index 1c8e5a367d84f5de9c47ea42f7d72cde7c0f5a6a..aee217d86619462e15020739397726ab13ffeaa9 100644
--- a/net/proxy/proxy_config_service_linux.cc
+++ b/net/proxy/proxy_config_service_linux.cc
@@ -207,10 +207,7 @@ class SettingGetterImplGConf : public ProxyConfigServiceLinux::SettingGetter {
 
   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.
+    // Delegate::OnDestroy(), while running on the UI thread.
     if (client_) {
       // gconf client was not cleaned up.
       if (task_runner_->BelongsToCurrentThread()) {
@@ -285,6 +282,7 @@ class SettingGetterImplGConf : public ProxyConfigServiceLinux::SettingGetter {
       client_ = NULL;
       task_runner_ = NULL;
     }
+    debounce_timer_.reset();
   }
 
   virtual bool SetUpNotifications(
@@ -293,6 +291,7 @@ class SettingGetterImplGConf : public ProxyConfigServiceLinux::SettingGetter {
     DCHECK(task_runner_->BelongsToCurrentThread());
     GError* error = NULL;
     notify_delegate_ = delegate;
+    debounce_timer_.reset(new base::OneShotTimer<SettingGetterImplGConf>());
     // 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?)
@@ -474,8 +473,8 @@ class SettingGetterImplGConf : public ProxyConfigServiceLinux::SettingGetter {
   void OnChangeNotification() {
     // We don't use Reset() because the timer may not yet be running.
     // (In that case Stop() is a no-op.)
-    debounce_timer_.Stop();
-    debounce_timer_.Start(FROM_HERE,
+    debounce_timer_->Stop();
+    debounce_timer_->Start(FROM_HERE,
         base::TimeDelta::FromMilliseconds(kDebounceTimeoutMilliseconds),
         this, &SettingGetterImplGConf::OnDebouncedNotification);
   }
@@ -498,7 +497,7 @@ class SettingGetterImplGConf : public ProxyConfigServiceLinux::SettingGetter {
   guint system_http_proxy_id_;
 
   ProxyConfigServiceLinux::Delegate* notify_delegate_;
-  base::OneShotTimer<SettingGetterImplGConf> debounce_timer_;
+  scoped_ptr<base::OneShotTimer<SettingGetterImplGConf> > debounce_timer_;
 
   // Task runner for the thread that we make gconf calls on. It should
   // be the UI thread and all our methods should be called on this
@@ -535,11 +534,7 @@ class SettingGetterImplGSettings
 
   virtual ~SettingGetterImplGSettings() {
     // 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.
+    // Delegate::OnDestroy(), while running on the UI thread.
     if (client_) {
       // gconf client was not cleaned up.
       if (task_runner_->BelongsToCurrentThread()) {
@@ -610,6 +605,7 @@ class SettingGetterImplGSettings
       client_ = NULL;
       task_runner_ = NULL;
     }
+    debounce_timer_.reset();
   }
 
   virtual bool SetUpNotifications(
@@ -617,6 +613,8 @@ class SettingGetterImplGSettings
     DCHECK(client_);
     DCHECK(task_runner_->BelongsToCurrentThread());
     notify_delegate_ = delegate;
+    debounce_timer_.reset(
+        new base::OneShotTimer<SettingGetterImplGSettings>());
     // We could watch for the change-event signal instead of changed, but
     // since we have to watch more than one object, we'd still have to
     // debounce change notifications. This is conceptually simpler.
@@ -789,8 +787,8 @@ class SettingGetterImplGSettings
   void OnChangeNotification() {
     // We don't use Reset() because the timer may not yet be running.
     // (In that case Stop() is a no-op.)
-    debounce_timer_.Stop();
-    debounce_timer_.Start(FROM_HERE,
+    debounce_timer_->Stop();
+    debounce_timer_->Start(FROM_HERE,
         base::TimeDelta::FromMilliseconds(kDebounceTimeoutMilliseconds),
         this, &SettingGetterImplGSettings::OnDebouncedNotification);
   }
@@ -811,7 +809,7 @@ class SettingGetterImplGSettings
   GSettings* ftp_client_;
   GSettings* socks_client_;
   ProxyConfigServiceLinux::Delegate* notify_delegate_;
-  base::OneShotTimer<SettingGetterImplGSettings> debounce_timer_;
+  scoped_ptr<base::OneShotTimer<SettingGetterImplGSettings> > debounce_timer_;
 
   // Task runner for the thread that we make gsettings calls on. It should
   // be the UI thread and all our methods should be called on this
@@ -969,12 +967,7 @@ class SettingGetterImplKDE : public ProxyConfigServiceLinux::SettingGetter,
 
   virtual ~SettingGetterImplKDE() {
     // inotify_fd_ should have been closed before now, from
-    // Delegate::OnDestroy(), while running on the file thread. However
-    // on exiting the process, it may happen that Delegate::OnDestroy()
-    // task is left pending on the file loop after the loop was quit,
-    // and pending tasks may then be deleted without being run.
-    // Here in the KDE version, we can safely close the file descriptor
-    // anyway. (Not that it really matters; the process is exiting.)
+    // Delegate::OnDestroy(), while running on the file thread.
     if (inotify_fd_ >= 0)
       ShutDown();
     DCHECK(inotify_fd_ < 0);
@@ -1011,12 +1004,14 @@ class SettingGetterImplKDE : public ProxyConfigServiceLinux::SettingGetter,
       close(inotify_fd_);
       inotify_fd_ = -1;
     }
+    debounce_timer_.reset();
   }
 
   virtual bool SetUpNotifications(
       ProxyConfigServiceLinux::Delegate* delegate) OVERRIDE {
     DCHECK(inotify_fd_ >= 0);
     DCHECK(MessageLoop::current() == file_loop_);
+    debounce_timer_.reset(new base::OneShotTimer<SettingGetterImplKDE>());
     // We can't just watch the kioslaverc file directly, since KDE will write
     // a new copy of it and then rename it whenever settings are changed and
     // inotify watches inodes (so we'll be watching the old deleted file after
@@ -1362,8 +1357,8 @@ class SettingGetterImplKDE : public ProxyConfigServiceLinux::SettingGetter,
     if (kioslaverc_touched) {
       // We don't use Reset() because the timer may not yet be running.
       // (In that case Stop() is a no-op.)
-      debounce_timer_.Stop();
-      debounce_timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(
+      debounce_timer_->Stop();
+      debounce_timer_->Start(FROM_HERE, base::TimeDelta::FromMilliseconds(
           kDebounceTimeoutMilliseconds), this,
           &SettingGetterImplKDE::OnDebouncedNotification);
     }
@@ -1376,7 +1371,7 @@ class SettingGetterImplKDE : public ProxyConfigServiceLinux::SettingGetter,
   int inotify_fd_;
   base::MessagePumpLibevent::FileDescriptorWatcher inotify_watcher_;
   ProxyConfigServiceLinux::Delegate* notify_delegate_;
-  base::OneShotTimer<SettingGetterImplKDE> debounce_timer_;
+  scoped_ptr<base::OneShotTimer<SettingGetterImplKDE> > debounce_timer_;
   FilePath kde_config_dir_;
   bool indirect_manual_;
   bool auto_no_pac_;
@@ -1604,19 +1599,31 @@ ProxyConfigServiceLinux::Delegate::Delegate(base::Environment* env_var_getter)
 
 ProxyConfigServiceLinux::Delegate::Delegate(
     base::Environment* env_var_getter, SettingGetter* setting_getter)
-    : env_var_getter_(env_var_getter), setting_getter_(setting_getter) {
+    : env_var_getter_(env_var_getter), setting_getter_(setting_getter),
+      finished_initialization_(false) {
 }
 
 void ProxyConfigServiceLinux::Delegate::SetUpAndFetchInitialConfig(
     base::SingleThreadTaskRunner* glib_thread_task_runner,
     base::SingleThreadTaskRunner* io_thread_task_runner,
     MessageLoopForIO* file_loop) {
-  // We should be running on the default glib main loop thread right
-  // now. gconf can only be accessed from this thread.
-  DCHECK(glib_thread_task_runner->BelongsToCurrentThread());
+  DCHECK(io_thread_task_runner->BelongsToCurrentThread());
+
   glib_thread_task_runner_ = glib_thread_task_runner;
   io_thread_task_runner_ = io_thread_task_runner;
 
+  glib_thread_task_runner->PostTask(FROM_HERE, base::Bind(
+      &Delegate::SetUpAndFetchInitialConfigOnUIThread,
+      this,
+      file_loop));
+}
+
+void ProxyConfigServiceLinux::Delegate::SetUpAndFetchInitialConfigOnUIThread(
+    MessageLoopForIO* file_loop) {
+  // We should be running on the default glib main loop thread right
+  // now. gconf can only be accessed from this thread.
+  DCHECK(glib_thread_task_runner_->BelongsToCurrentThread());
+
   // If we are passed a NULL |io_thread_task_runner| or |file_loop|,
   // then we don't set up proxy setting change notifications. This
   // should not be the usual case but is intended to simplify test
@@ -1638,12 +1645,12 @@ void ProxyConfigServiceLinux::Delegate::SetUpAndFetchInitialConfig(
 
   bool got_config = false;
   if (setting_getter_.get() &&
-      setting_getter_->Init(glib_thread_task_runner, file_loop) &&
-      GetConfigFromSettings(&cached_config_)) {
-    cached_config_.set_id(1);  // Mark it as valid.
-    cached_config_.set_source(setting_getter_->GetConfigSource());
+      setting_getter_->Init(glib_thread_task_runner_, file_loop) &&
+      GetConfigFromSettings(&reference_config_)) {
+    reference_config_.set_id(1);  // Mark it as valid.
+    reference_config_.set_source(setting_getter_->GetConfigSource());
     VLOG(1) << "Obtained proxy settings from "
-            << ProxyConfigSourceToString(cached_config_.source());
+            << ProxyConfigSourceToString(reference_config_.source());
 
     // If gconf proxy mode is "none", meaning direct, then we take
     // that to be a valid config and will not check environment
@@ -1651,11 +1658,6 @@ void ProxyConfigServiceLinux::Delegate::SetUpAndFetchInitialConfig(
     // whereever we can find one.
     got_config = true;
 
-    // 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.
-
     // We only set up notifications if we have IO and file loops available.
     // We do this after getting the initial configuration so that we don't have
     // to worry about cancelling it if the initial fetch above fails. Note that
@@ -1663,7 +1665,7 @@ void ProxyConfigServiceLinux::Delegate::SetUpAndFetchInitialConfig(
     // that we won't lose any updates that may have happened after the initial
     // fetch and before setting up notifications. We'll detect the common case
     // of no changes in OnCheckProxyConfigSettings() (or sooner) and ignore it.
-    if (io_thread_task_runner && file_loop) {
+    if (io_thread_task_runner_ && file_loop) {
       scoped_refptr<base::SingleThreadTaskRunner> required_loop =
           setting_getter_->GetNotificationTaskRunner();
       if (!required_loop || required_loop->BelongsToCurrentThread()) {
@@ -1682,12 +1684,18 @@ void ProxyConfigServiceLinux::Delegate::SetUpAndFetchInitialConfig(
     //
     // 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_source(PROXY_CONFIG_SOURCE_ENV);
-      cached_config_.set_id(1);  // Mark it as valid.
+    if (GetConfigFromEnv(&reference_config_)) {
+      reference_config_.set_source(PROXY_CONFIG_SOURCE_ENV);
+      reference_config_.set_id(1);  // Mark it as valid.
       VLOG(1) << "Obtained proxy settings from environment variables";
     }
   }
+
+  // Post a task to the IO thread with the new configuration, so it can
+  // update |cached_config_|.
+  io_thread_task_runner_->PostTask(FROM_HERE, base::Bind(
+      &ProxyConfigServiceLinux::Delegate::SetNewProxyConfig,
+      this, reference_config_));
 }
 
 // Depending on the SettingGetter in use, this method will be called
@@ -1715,6 +1723,9 @@ ProxyConfigService::ConfigAvailability
   DCHECK(!io_thread_task_runner_ ||
          io_thread_task_runner_->BelongsToCurrentThread());
 
+  if (!finished_initialization_)
+    return CONFIG_PENDING;
+
   // Simply return the last proxy configuration that glib_default_loop
   // notified us of.
   if (cached_config_.is_valid()) {
@@ -1724,11 +1735,7 @@ ProxyConfigService::ConfigAvailability
     config->set_source(PROXY_CONFIG_SOURCE_SYSTEM_FAILED);
   }
 
-  // We return CONFIG_VALID to indicate that *config was filled in. It is always
-  // going to be available since we initialized eagerly on the UI thread.
-  // TODO(eroman): do lazy initialization instead, so we no longer need
-  //               to construct ProxyConfigServiceLinux on the UI thread.
-  //               In which case, we may return false here.
+  // We return CONFIG_VALID to indicate that *config was filled in.
   return CONFIG_VALID;
 }
 
@@ -1763,6 +1770,7 @@ void ProxyConfigServiceLinux::Delegate::SetNewProxyConfig(
   DCHECK(io_thread_task_runner_->BelongsToCurrentThread());
   VLOG(1) << "Proxy configuration changed";
   cached_config_ = new_config;
+  finished_initialization_ = true;
   FOR_EACH_OBSERVER(
       Observer, observers_,
       OnProxyConfigChanged(new_config, ProxyConfigService::CONFIG_VALID));
@@ -1776,6 +1784,8 @@ void ProxyConfigServiceLinux::Delegate::PostDestroyTask() {
   if (!shutdown_loop || shutdown_loop->BelongsToCurrentThread()) {
     // Already on the right thread, call directly.
     // This is the case for the unittests.
+    // The browser's UI thread will not execute any more tasks at this point
+    // so its necessary to call directly.
     OnDestroy();
   } else {
     // Post to shutdown thread. Note that on browser shutdown, we may quit
@@ -1791,11 +1801,26 @@ void ProxyConfigServiceLinux::Delegate::OnDestroy() {
   setting_getter_->ShutDown();
 }
 
-ProxyConfigServiceLinux::ProxyConfigServiceLinux()
+ProxyConfigServiceLinux::ProxyConfigServiceLinux(
+    base::SingleThreadTaskRunner* glib_thread_task_runner,
+    base::SingleThreadTaskRunner* io_thread_task_runner,
+    MessageLoopForIO* file_loop)
     : delegate_(new Delegate(base::Environment::Create())) {
+  SetupAndFetchInitialConfig(glib_thread_task_runner,
+                             io_thread_task_runner,
+                             file_loop);
 }
 
 ProxyConfigServiceLinux::~ProxyConfigServiceLinux() {
+}
+
+void ProxyConfigServiceLinux::StartTearDown() {
+  // This must be done prior to destruction.  Destruction occurs as the threads
+  // are dying which is too late to post tasks.  Previously this code relied on
+  // ~Delegate() being called when the unexecuted Delegate::OnDestroy() task
+  // was purged from the UI thread's MessageLoop as we shutdown. This didn't
+  // work very well as things like BelongsToCurrentThread() don't operate
+  // properly at this late a stage.
   delegate_->PostDestroyTask();
 }