Dynamically refresh pref-configured proxies.

chrome/browser/net/pref_proxy_config_service.cc

+// Copyright (c) 2010 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/net/pref_proxy_config_service.h"
+
+#include "base/ref_counted.h"
+#include "chrome/browser/browser_thread.h"
+#include "chrome/browser/prefs/pref_service.h"
+#include "chrome/browser/prefs/pref_set_observer.h"
+#include "chrome/common/notification_details.h"
+#include "chrome/common/notification_observer.h"
+#include "chrome/common/notification_source.h"
+#include "chrome/common/notification_type.h"
+#include "chrome/common/pref_names.h"
+
+// A helper class that tracks proxy preferences. It translates the configuration
+// to net::ProxyConfig and proxies the result over to PrefProxyConfigService on
+// the IO thread.
+class PrefProxyConfigService::PrefProxyConfigTracker
+    : public base::RefCountedThreadSafe<PrefProxyConfigTracker,
+                                        BrowserThread::DeleteOnUIThread>,

[eroman, 2010/11/17 02:14:49]

DeleteOnUIThread is problematic, since it can lead to leaks during shutdown.
Please avoid using this.

In particular consider what happens if ProxyConfigService is 
deleted during IOThread::Stop(): we release the final reference to the tracker,
which will in turn post a task to the UI thread to delete. However since we are
already shutting down, this task will never actually get run.

It would be better to do the teardown explicitly when the pref_service is
deleted. I guess one way would be to watch for PROFILE_DESTROYED, or move the
helper object into the profile.

[Mattias Nissler (ping if slow), 2010/11/17 12:55:37]

Good point, I actually realized the same thing while thinking about it a bit
more last night. I've now moved the tracker into the profile, from which we can
shut it down cleanly.

+      public NotificationObserver {
+ public:
+  PrefProxyConfigTracker(
+      base::WeakPtr<PrefProxyConfigService> pref_config_service,
+      PrefService* pref_service)
+      : pref_config_service_(pref_config_service),
+        pref_service_(pref_service) {
+    ReadPrefConfig(&pref_config_);
+    proxy_prefs_observer_.reset(
+        PrefSetObserver::CreateProxyPrefSetObserver(pref_service_, this));
+  }
+
+  virtual ~PrefProxyConfigTracker() {}
+
+  // Get the proxy configuration currently defined by preferences.
+  const net::ProxyConfig& GetProxyConfig() { return pref_config_; }
+
+ private:
+  // NotificationObserver implementation:
+  virtual void Observe(NotificationType type,
+                       const NotificationSource& source,
+                       const NotificationDetails& details);
+
+  // Install a new configuration (to be called on the IO thread).
+  void InstallProxyConfig(const net::ProxyConfig& config);
+
+  // Creates a proxy configuration from proxy-related preferences. Sets |config|
+  // to invalid if there's no proxy configuration defined in prefs.
+  void ReadPrefConfig(net::ProxyConfig* config);
+
+  // Configuration as defined by prefs. Only to be accessed from the IO thread
+  // (expect for construction).
+  net::ProxyConfig pref_config_;
+
+  base::WeakPtr<PrefProxyConfigService> pref_config_service_;

[eroman, 2010/11/17 02:14:49]

WeakPtr is not threadsafe. Is it valid for this to be deleted from the UI
thread? (Whereas PrefProxyConfigService will be deleted from the IO thread).

[Mattias Nissler (ping if slow), 2010/11/17 12:55:37]

Replaced this hack with a proper observer interface in the tracker.

+  PrefService* pref_service_;
+  scoped_ptr<PrefSetObserver> proxy_prefs_observer_;
+
+  DISALLOW_COPY_AND_ASSIGN(PrefProxyConfigTracker);
+};
+
+void PrefProxyConfigService::PrefProxyConfigTracker::Observe(
+    NotificationType type,
+    const NotificationSource& source,
+    const NotificationDetails& details) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+  if (type == NotificationType::PREF_CHANGED &&
+      Source<PrefService>(source).ptr() == pref_service_) {
+    net::ProxyConfig new_config;
+    ReadPrefConfig(&new_config);
+    BrowserThread::PostTask(
+        BrowserThread::IO, FROM_HERE,
+        NewRunnableMethod(this,
+                          &PrefProxyConfigTracker::InstallProxyConfig,
+                          new_config));
+  } else {
+    NOTREACHED() << "Unexpected notification of type " << type.value;
+  }
+}
+
+void PrefProxyConfigService::PrefProxyConfigTracker::InstallProxyConfig(
+    const net::ProxyConfig& config) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  pref_config_ = config;
+  if (pref_config_service_.get())
+    pref_config_service_->PrefProxyConfigChanged();
+}
+
+void PrefProxyConfigService::PrefProxyConfigTracker::ReadPrefConfig(
+    net::ProxyConfig* config) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
+
+  // Invalidate the configuration.
+  config->set_id(net::ProxyConfig::INVALID_ID);

[eroman, 2010/11/17 02:14:49]

Please avoid using the set_id (see my later comment). This is a historical
detail which hasn't been cleaned up and is confusing.

Rather please return a bool or an enum, or a composite structure.

Conceptually I believe this function basically wants to return two types of
values: either (a) a ProxyConfig, or (b) an indication that we should use the
system settings (i.e. base_service).

To clarify this throughout the file, I suggest introducing a helper class like
this:

class ProxyConfigFromPrefs {
 public:
  const ProxyConfig& config() const {
    DCHECK(was_specified_);
    return config_;
  }

  bool was_specified() const {
    return was_specified_;
  }

 private:
  ProxyConfig config_;
  bool was_specified_;
};

Now all the functions which wanted to return an indication of "fall back to the
base service" and were doing so by way of setting invalid ID, would just return
a ProxyConfigFromPrefs where was_specified_ = false.


Moreover in the future it seems reasonable that we would actually want a
preference that says "Use the system proxy settings", as opposed to inferring
that action from lack of any other preference.

[Mattias Nissler (ping if slow), 2010/11/17 12:55:37]

Done.

+
+  // Scan for all "enable" type proxy switches.
+  static const char* proxy_prefs[] = {
+    prefs::kProxyPacUrl,
+    prefs::kProxyServer,
+    prefs::kProxyBypassList,
+    prefs::kProxyAutoDetect
+  };
+
+  // Check whether the preference system holds a valid proxy configuration. Note
+  // that preferences coming from a lower-priority source than the user settings
+  // are ignored. That's because chrome treats the system settings as the
+  // default values, which should apply if there's no explicit value forced by
+  // policy or the user.
+  bool found_enable_proxy_pref = false;
+  for (size_t i = 0; i < arraysize(proxy_prefs); i++) {
+    const PrefService::Preference* pref =
+        pref_service_->FindPreference(proxy_prefs[i]);
+    DCHECK(pref);
+    if (pref && (!pref->IsUserModifiable() || pref->HasUserSetting())) {
+      found_enable_proxy_pref = true;
+      break;
+    }
+  }
+
+  if (!found_enable_proxy_pref &&
+      !pref_service_->GetBoolean(prefs::kNoProxyServer)) {
+    return;
+  }
+
+  // There is a valid configuration present.
+  config->set_id(1);
+
+  if (pref_service_->GetBoolean(prefs::kNoProxyServer)) {
+    // Ignore all the other proxy config preferences if the use of a proxy
+    // has been explicitly disabled.
+    return;
+  }
+
+  if (pref_service_->HasPrefPath(prefs::kProxyServer)) {
+    std::string proxy_server = pref_service_->GetString(prefs::kProxyServer);
+    config->proxy_rules().ParseFromString(proxy_server);
+  }
+
+  if (pref_service_->HasPrefPath(prefs::kProxyPacUrl)) {
+    std::string proxy_pac = pref_service_->GetString(prefs::kProxyPacUrl);
+    config->set_pac_url(GURL(proxy_pac));
+  }
+
+  config->set_auto_detect(pref_service_->GetBoolean(prefs::kProxyAutoDetect));
+
+  if (pref_service_->HasPrefPath(prefs::kProxyBypassList)) {
+    std::string proxy_bypass =
+        pref_service_->GetString(prefs::kProxyBypassList);
+    config->proxy_rules().bypass_rules.ParseFromString(proxy_bypass);
+  }
+}
+
+PrefProxyConfigService::PrefProxyConfigService(
+    PrefService* pref_service,
+    net::ProxyConfigService* base_service)
+    : base_service_(base_service),
+      current_id_(1) {
+  pref_config_tracker_ = new PrefProxyConfigTracker(AsWeakPtr(), pref_service);
+  base_service_->AddObserver(this);
+}
+
+PrefProxyConfigService::~PrefProxyConfigService() {
+  base_service_->RemoveObserver(this);
+}
+
+void PrefProxyConfigService::PrefProxyConfigChanged() {
+  ++current_id_;

[eroman, 2010/11/17 02:14:49]

What is the use-case for this ID number? Please just leave the ID number at its
default (the ID number is an internal detail used within the ProxyService for
some book-keeping, but does not need to be set by the ProxyConfigService).

[Mattias Nissler (ping if slow), 2010/11/17 12:55:37]

Done.

+  net::ProxyConfig current_config;
+  GetLatestProxyConfig(&current_config);

[eroman, 2010/11/17 02:14:49]

Important: you are disregarding the return value from this function.

If the "base_service_" has returned false, it means we don't know what the proxy
configuration is yet (i.e. it completes asynchronously). In which case it is
incorrect to notify the observers, since |current__config| could be garbage.

[Mattias Nissler (ping if slow), 2010/11/17 12:55:37]

Done.

+  FOR_EACH_OBSERVER(net::ProxyConfigService::Observer, observers_,
+                    OnProxyConfigChanged(current_config));
+}
+
+void PrefProxyConfigService::AddObserver(
+    net::ProxyConfigService::Observer* observer) {
+  observers_.AddObserver(observer);
+}
+
+void PrefProxyConfigService::RemoveObserver(
+    net::ProxyConfigService::Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+bool PrefProxyConfigService::GetLatestProxyConfig(net::ProxyConfig* config) {
+  const net::ProxyConfig& pref_config(pref_config_tracker_->GetProxyConfig());
+  if (pref_config.is_valid()) {

[eroman, 2010/11/17 02:14:49]

See comment about not using the IDs. If you decided to use the struct, this
function could be re-written as:

const ProxyConfigFromPrefs& pref_config(pref_config_tracker_->GetProxyConfig());

if (pref_config.was_specified()) {
  *config = pref_config.config();
  return true;
}
return base_service_->GetLatestProxyConfig(config);

[Mattias Nissler (ping if slow), 2010/11/17 12:55:37]

Done.

+    *config = pref_config;
+    config->set_id(current_id_);
+    return true;
+  }
+
+  bool result = base_service_->GetLatestProxyConfig(config);
+  if (config->is_valid())
+    config->set_id(current_id_);
+  return result;
+}
+
+void PrefProxyConfigService::OnLazyPoll() {
+  base_service_->OnLazyPoll();
+}
+
+void PrefProxyConfigService::OnProxyConfigChanged(
+    const net::ProxyConfig& config) {
+  if (!pref_config_tracker_->GetProxyConfig().is_valid()) {

[eroman, 2010/11/17 02:14:49]

See earlier comment, please don't use the ID field, and instead consider a
ProxyConfig structure as meaning a (valid) configuration.

[Mattias Nissler (ping if slow), 2010/11/17 12:55:37]

Done.

+    net::ProxyConfig new_config = config;
+    new_config.set_id(++current_id_);

[eroman, 2010/11/17 02:14:49]

Shouldn't be a need for the ID.

[Mattias Nissler (ping if slow), 2010/11/17 12:55:37]

Done.

+    FOR_EACH_OBSERVER(net::ProxyConfigService::Observer, observers_,
+                      OnProxyConfigChanged(new_config));
+  }
+}