Upstream Android proxy config service.

net/proxy/proxy_config_service_android.cc

+// Copyright (c) 2012 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 "net/proxy/proxy_config_service_android.h"
+
+#include "base/android/jni_android.h"
+#include "base/android/jni_string.h"
+#include "base/bind.h"
+#include "base/logging.h"
+#include "base/message_loop.h"
+#include "base/message_loop_proxy.h"
+#include "base/string_tokenizer.h"
+#include "base/string_util.h"
+#include "jni/proxy_change_listener_jni.h"
+#include "net/proxy/proxy_config.h"
+
+#include <sys/system_properties.h>
+
+using base::android::AttachCurrentThread;
+using base::android::ConvertUTF8ToJavaString;
+using base::android::ConvertJavaStringToUTF8;
+using base::android::CheckException;
+using base::android::ClearException;
+using base::android::GetMethodID;
+
+namespace net {
+
+namespace {
+// This class implements the link to Android's proxy broadcast mechanism.

[Ryan Sleevi, 2012/04/24 18:20:04]

nit: Line break between 29 & 30

[Philippe, 2012/05/09 11:48:39]

Done.

+class DelegateImpl : public ProxyConfigServiceAndroid::Delegate {
+ public:
+  DelegateImpl();

[Ryan Sleevi, 2012/04/24 18:20:04]

nit: Declare a destructor, add a line break between 33 & 34

[Philippe, 2012/05/09 11:48:39]

Declaring the destructor here won't serve us in this case since we are in a cc
file (unless I'm missing something). You might have thought that this was in a
header since the implementation was not inlined (although nothing prevented it
from being). To avoid any confusion I have now inlined the implementation.

+  // ProxyConfigServiceAndroid::Delegate:
+  virtual void Start(ProxyConfigServiceAndroid* service) OVERRIDE;
+  virtual void Stop() OVERRIDE;
+  virtual std::string GetProperty(const std::string& property) OVERRIDE;
+
+ private:
+  ProxyConfigServiceAndroid* service_;
+  base::android::ScopedJavaGlobalRef<jobject>
+      java_proxy_change_listener_android_;
+};
+
+DelegateImpl::DelegateImpl() : service_(NULL) {}
+
+void DelegateImpl::Start(ProxyConfigServiceAndroid* service) {
+  DCHECK(!service_);
+  JNIEnv* env = AttachCurrentThread();
+  service_ = service;
+  if (java_proxy_change_listener_android_.is_null()) {
+    java_proxy_change_listener_android_.Reset(
+        Java_ProxyChangeListener_create(
+            env, base::android::GetApplicationContext()));
+    CHECK(!java_proxy_change_listener_android_.is_null());
+  }
+  Java_ProxyChangeListener_start(
+      env,
+      java_proxy_change_listener_android_.obj(),
+      reinterpret_cast<jint>(service));
+}
+
+void DelegateImpl::Stop() {
+  // The java object will be NULL if Stop() was already called (e.g. after a
+  // Start()), or if Start() was never called. Stop() is called from the
+  // destructor of ProxyConfigServiceAndroid.
+  if (java_proxy_change_listener_android_.is_null())
+    return;
+  JNIEnv* env = AttachCurrentThread();
+  Java_ProxyChangeListener_stop(env,
+                                java_proxy_change_listener_android_.obj());
+}
+
+std::string DelegateImpl::GetProperty(const std::string& property) {
+  // Use Java System.getProperty to get configuration information.
+  // Question: Conversion to/from UTF8 ok here?
+  JNIEnv* env = AttachCurrentThread();
+  ScopedJavaLocalRef<jstring> str = ConvertUTF8ToJavaString(env, property);
+  ScopedJavaLocalRef<jstring> result =
+      Java_ProxyChangeListener_getProperty(env, str.obj());
+  return result.is_null() ? "" : ConvertJavaStringToUTF8(env, result.obj());
+}
+}  // namespace

[Ryan Sleevi, 2012/04/24 18:20:04]

nit: Line break between 82 & 83

[Philippe, 2012/05/09 11:48:39]

Done.

+
+ProxyConfigServiceAndroid::ProxyConfigServiceAndroid()
+    : delegate_(new DelegateImpl),
+      notify_task_(NULL) {
+}
+
+ProxyConfigServiceAndroid::ProxyConfigServiceAndroid(
+    ProxyConfigServiceAndroid::Delegate* delegate)
+    : delegate_(delegate),
+      notify_task_(NULL) {
+}
+
+ProxyConfigServiceAndroid::~ProxyConfigServiceAndroid() {
+  DCHECK(OnObserverThread());

[Ryan Sleevi, 2012/04/24 18:20:04]

This is very surprising, and I think dangerously subtle.

Instead of matching "Create on thread X, delete on thread X", it's "Create on
thread Y, call AddObserver on thread X, delete on thread X"

[Philippe, 2012/05/09 11:48:39]

I agree. Unfortunately I can't change that.

+  if (delegate_.get())
+    delegate_->Stop();
+  // Cancel the notify task last, as the java side may have been mid-way through
+  // posting a new notify task when we requested it stop
+  CancelNotifyTask();
+}
+
+bool ProxyConfigServiceAndroid::Init(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+
+void ProxyConfigServiceAndroid::AddObserver(Observer* observer) {
+  DCHECK(OnObserverThread());
+  if (observers_.size() == 0) {
+    AssignObserverThread();
+    delegate_->Start(this);
+  }
+  // Note that any callbacks from the delegate will result in a scheduled task
+  // on this same thread, so there is no race between delegate_->Start() and
+  // AddObserver().
+  observers_.AddObserver(observer);
+}
+
+void ProxyConfigServiceAndroid::RemoveObserver(Observer* observer) {
+  DCHECK(OnObserverThread());
+  observers_.RemoveObserver(observer);
+  if (observers_.size() == 0) {
+    delegate_->Stop();
+  }
+}
+
+namespace {

[Ryan Sleevi, 2012/04/24 18:20:04]

This is very weird to read that you jump from an unnamed namespace into net
(line 83) and then back into an unnamed namespace. Can you move these methods
up?

[Philippe, 2012/05/09 11:48:39]

Done.

+
+ProxyServer ConstructProxyServer(ProxyServer::Scheme scheme,
+                                 const std::string& proxyHost,

[Ryan Sleevi, 2012/04/24 18:20:04]

Style: Please go through these methods and make sure they match Chromium's C++
style (proxy_host / proxy_port) rather than Java naming style (proxyHost /
proxyPort)

[Philippe, 2012/05/09 11:48:39]

Done.

+                                 const std::string& proxyPort) {
+  DCHECK(!proxyHost.empty());
+  if (proxyPort.empty())
+    return ProxyServer::FromURI(proxyHost, scheme);
+  return ProxyServer::FromURI(proxyHost + ":" + proxyPort, scheme);
+}
+
+ProxyServer LookupProxy(
+    ProxyConfigServiceAndroid::Delegate& delegate,
+    const std::string& prefix,
+    ProxyServer::Scheme scheme) {
+  std::string proxyHost = delegate.GetProperty(prefix + ".proxyHost");
+  if (!proxyHost.empty()) {
+    std::string proxyPort = delegate.GetProperty(prefix + ".proxyPort");
+    return ConstructProxyServer(scheme, proxyHost, proxyPort);
+  }
+  // Fall back to default proxy, if any.
+  proxyHost = delegate.GetProperty("proxyHost");
+  if (!proxyHost.empty()) {
+    std::string proxyPort = delegate.GetProperty("proxyPort");
+    return ConstructProxyServer(scheme, proxyHost, proxyPort);
+  }
+  return ProxyServer();
+}
+
+ProxyServer LookupSocksProxy(
+    ProxyConfigServiceAndroid::Delegate& delegate) {
+  std::string proxyHost = delegate.GetProperty("socksProxyHost");
+  if (!proxyHost.empty()) {
+    std::string proxyPort = delegate.GetProperty("socksProxyPort");
+    return ConstructProxyServer(ProxyServer::SCHEME_SOCKS5, proxyHost,
+                                proxyPort);
+  }
+  return ProxyServer();
+}
+
+void AddBypassRules(
+    ProxyConfigServiceAndroid::Delegate& delegate,
+    const std::string& scheme,
+    ProxyBypassRules* bypass_rules) {
+  // The format of a hostname pattern is a list of hostnames that are separated
+  // by | and that use * as a wildcard. For example, setting the
+  // http.nonProxyHosts property to *.android.com|*.kernel.org will cause
+  // requests to http://developer.android.com to be made without a proxy.
+  std::string nonProxyHosts = delegate.GetProperty(scheme + ".nonProxyHosts");
+  if (nonProxyHosts.empty())
+    return;
+  StringTokenizer tokenizer(nonProxyHosts, "|");
+  while (tokenizer.GetNext()) {
+    std::string token = tokenizer.token();
+    std::string pattern;
+    TrimWhitespaceASCII(token, TRIM_ALL, &pattern);
+    if (pattern.empty())
+      continue;
+    // '?' is not one of the specified pattern characters above.
+    DCHECK_EQ(std::string::npos, pattern.find('?'));
+    bypass_rules->AddRuleForHostname(scheme, pattern, -1);
+  }
+}
+
+// returns true iff a valid proxy was found.
+bool GetProxyRules(
+    ProxyConfigServiceAndroid::Delegate& delegate,
+    ProxyConfig::ProxyRules* rules) {
+  // See libcore/luni/src/main/java/java/net/ProxySelectorImpl.java for the
+  // semantics we're trying to match.
+  rules->type = ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME;
+  rules->proxy_for_http = LookupProxy(delegate, "http",
+                                      ProxyServer::SCHEME_HTTP);
+  rules->proxy_for_https = LookupProxy(delegate, "https",
+                                       ProxyServer::SCHEME_HTTPS);
+  rules->proxy_for_ftp = LookupProxy(delegate, "ftp", ProxyServer::SCHEME_HTTP);
+  rules->fallback_proxy = LookupSocksProxy(delegate);
+  rules->bypass_rules.Clear();
+  AddBypassRules(delegate, "ftp", &rules->bypass_rules);
+  AddBypassRules(delegate, "http", &rules->bypass_rules);
+  AddBypassRules(delegate, "https", &rules->bypass_rules);
+  return rules->proxy_for_http.is_valid() ||
+      rules->proxy_for_https.is_valid() ||
+      rules->proxy_for_ftp.is_valid() ||
+      rules->fallback_proxy.is_valid();
+};
+
+void GetLatestProxyConfigInternal(
+    ProxyConfigServiceAndroid::Delegate& delegate,
+    ProxyConfig* config) {
+  if (!GetProxyRules(delegate, &config->proxy_rules()))
+    *config = ProxyConfig::CreateDirect();
+}
+
+}  // namespace
+
+ProxyConfigService::ConfigAvailability
+ProxyConfigServiceAndroid::GetLatestProxyConfig(ProxyConfig* config) {
+  DCHECK(OnObserverThread());
+  if (!config)
+    return ProxyConfigService::CONFIG_UNSET;
+  GetLatestProxyConfigInternal(*delegate_.get(), config);
+  return ProxyConfigService::CONFIG_VALID;
+}
+
+// Posted on observer_loop_. See header file for intended behaviour.
+class ProxyConfigServiceAndroid::NotifyTask {
+ public:
+  NotifyTask(ProxyConfigServiceAndroid* service);
+  ~NotifyTask();
+  void Cancel();
+  void Run();
+
+ private:
+  ProxyConfigServiceAndroid* service_;
+};
+
+ProxyConfigServiceAndroid::NotifyTask::NotifyTask(
+    ProxyConfigServiceAndroid* service)
+    : service_(service) {
+}
+
+ProxyConfigServiceAndroid::NotifyTask::~NotifyTask() {
+  // Should be called from IO thread.

[Ryan Sleevi, 2012/04/24 18:20:04]

network thread

[Philippe, 2012/05/09 11:48:39]

I removed NotifyTask.

+  if (service_) {
+    // This could happen if the notify task is removed from the message loop
+    // before it has been run (e.g. if the message loop is destroyed).
+    service_->ClearNotifyTask();
+  }
+}
+
+void ProxyConfigServiceAndroid::NotifyTask::Cancel() {
+  // Should be called from IO thread.

[Ryan Sleevi, 2012/04/24 18:20:04]

This doesn't seem to be met - ProxyConfigServiceAndroid::CancelNotifyTask is
called in the destructor, which does DCHECK(OnObserverThread())

[Philippe, 2012/05/09 11:48:39]

I removed NotifyTask.

+  service_ = NULL;
+}
+
+void ProxyConfigServiceAndroid::NotifyTask::Run() {
+  // Should be called from IO thread.
+  if (service_) {
+    service_->ProxySettingsChangedCallback();
+    service_ = NULL;
+  }
+}
+
+void ProxyConfigServiceAndroid::ProxySettingsChanged() {
+  // Called on any thread.
+  DCHECK(observer_loop_);  // Should have been assigned in AddObserver().
+  NotifyTask* task = NULL;
+  {
+    base::AutoLock lock(notify_task_lock_);
+    // If there is already a task scheduled, don't schedule another one.
+    if (notify_task_)
+      return;
+    notify_task_ = task = new NotifyTask(this);
+  }
+  observer_loop_->PostTask(FROM_HERE,

[Ryan Sleevi, 2012/04/24 18:20:04]

Possible crash here, AFAICT

Two threads: T1, T2
T1 is in ProxySettingsChanged
T2 is in ~ProxyConfigServiceAndroid()

T1 wakes, runs, gets lock (line 278)
T1 creates task, releases lock (line 283), OS puts to sleep
T2 wakes, runs, begins tearing down. Calls to CancelNotifyTask (line 102). Gets
lock, tears down, object is now destroyed.
T1 wakes, runs line 284, |observer_loop_| is now destroyed, mayhem ensues.

Having the lock cover line 284 would ensure that the OS puts it to sleep (line
283), the lock is still held, so T1 will block destruction in CancelNotifyTask,
since it's trying to grab the lock (305)

However, I think there are many a subtle threading semantic here that I think
can be largely simplified.

[Philippe, 2012/05/09 11:48:39]

This code is now much simpler.

+                     base::Bind(&NotifyTask::Run, base::Owned(task)));
+}
+
+void ProxyConfigServiceAndroid::ProxySettingsChangedCallback() {
+  DCHECK(OnObserverThread());
+  // Allow another task to be scheduled (before running observers).
+  ClearNotifyTask();
+  ProxyConfig config;
+  GetLatestProxyConfigInternal(*delegate_.get(), &config);
+  FOR_EACH_OBSERVER(Observer, observers_,
+                    OnProxyConfigChanged(config,
+                                         ProxyConfigService::CONFIG_VALID));
+}
+
+void ProxyConfigServiceAndroid::ClearNotifyTask() {
+  base::AutoLock lock(notify_task_lock_);
+  notify_task_ = NULL;
+}
+
+void ProxyConfigServiceAndroid::CancelNotifyTask() {
+  base::AutoLock lock(notify_task_lock_);
+  if (notify_task_) {
+    notify_task_->Cancel();
+    notify_task_ = NULL;
+  }
+}
+
+bool ProxyConfigServiceAndroid::OnObserverThread() {
+  // Check that we're on the observer thread or the observer thread hasn't been
+  // assigned yet. The observer thread is only assigned in AddObserver(), which,
+  // in theory, might not be called.
+  return !observer_loop_ || observer_loop_->BelongsToCurrentThread();

[Ryan Sleevi, 2012/04/24 18:20:04]

This is equally subtle and surprising.

I think these comments belong in the header in some form (here and line
320-322), but I think it's very non-intuitive that OnObserverThread == true when
there is no ObserverThread

[Philippe, 2012/05/09 11:48:39]

Since the message loop proxy is now injected in the class' constructor we don't
have these subtleties anymore.

+}
+
+void ProxyConfigServiceAndroid::AssignObserverThread() {
+  // Called from AddObserver. We assign observer_loop_ here under a lock so that
+  // it is guaranteed that ProxySettingsChanged(), which is called from another
+  // thread, will see the value of observer_loop_.

[Ryan Sleevi, 2012/04/24 18:20:04]

Lock? I don't see any lock.

[Philippe, 2012/05/09 11:48:39]

Indeed. I removed this method since the observer loop is injected in the class'
constructor.

+  if (!observer_loop_) {
+    observer_loop_ = base::MessageLoopProxy::current();
+  } else {
+    DCHECK(OnObserverThread());

[Ryan Sleevi, 2012/04/24 18:20:04]

It seems like this could be written:
if (!observer_loop_)
  observer_loop_ = base::MessageLoopProxy::current();
DCHECK(observer_loop_->BelongsToCurrentThread())

[Philippe, 2012/05/09 11:48:39]

Done.

+  }
+}
+
+} // namespace net