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/location.h"
+#include "base/logging.h"
+#include "base/memory/ref_counted.h"
+#include "base/single_thread_task_runner.h"
+#include "base/string_number_conversions.h"
+#include "base/string_tokenizer.h"
+#include "base/string_util.h"
+#include "jni/proxy_change_listener_jni.h"

[Ryan Sleevi, 2012/05/14 18:00:25]

Is this absolute to the source tree? Or is this really
"<(SHARED_INTERMEDIATE_DIR)/net/jni/proxy_change_listener_jni.h" but with a
hidden -I "<(SHARED_INTERMEDIATE_DIR)/net"

For generated headers, we try to generate them with their full names, and then
just -I the <(SHARED_INTERMEDIATE_DIR) that they were generated in, rather than
-I the more specific path. This makes it easier to cross-reference the source
tree and find where the thing-that-generates-the-header is stored.

[Philippe, 2012/05/15 16:12:48]

Indeed, it is "<(SHARED_INTERMEDIATE_DIR)/net/jni/proxy_change_listener_jni.h"
with a hidden -I "<(SHARED_INTERMEDIATE_DIR)/net".

But I don't see where "<(SHARED_INTERMEDIATE_DIR)/net" is being included.
<(SHARED_INTERMEDIATE_DIR) does not seem to be added to the include dirs either
which means that I can't include "net/jni/proxy_change_listener_jni.h" instead.

+#include "net/base/host_port_pair.h"
+#include "net/proxy/proxy_config.h"
+
+#include <sys/system_properties.h>

[Ryan Sleevi, 2012/05/14 18:00:25]

C headers before C++ & Project headers (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Names_...
)

[Philippe, 2012/05/15 16:12:48]

Done.

+
+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 {
+
+ProxyServer ConstructProxyServer(ProxyServer::Scheme scheme,
+                                 const std::string& proxy_host,
+                                 const std::string& proxy_port) {
+  DCHECK(!proxy_host.empty());
+  if (proxy_port.empty())
+    return ProxyServer::FromURI(proxy_host, scheme);

[Ryan Sleevi, 2012/05/14 18:00:25]

I'm still not convinced this is handling IPv6 properly - but I'll readily admit
I may not be familiar with how Android is storing things.

if .proxyHost == "a:b:c::d:1", and .proxyPort == "", then when this is called,
it will try to construct as "[a:b:c::d]:1" rather than "[a:b:c::d:1]"

Since you've got unit tests - can you add and see what would happen with an IPv6
proxy?

[Philippe, 2012/05/15 16:12:48]

I fixed the implementation to use HostPortPair in all cases and added some unit
tests.

+  uint port_as_uint = 0;
+  base::StringToUint(proxy_port, &port_as_uint);

[Ryan Sleevi, 2012/05/14 18:00:25]

BUG? Using a GURL seems more appropriate here for parsing, especially since you
can end up with a port > UINT16_MAX with this approach

[Philippe, 2012/05/15 16:12:48]

Done.

+  return ProxyServer::FromURI(
+      HostPortPair(proxy_host, port_as_uint).ToString(), scheme);
+}
+
+ProxyServer LookupProxy(
+    const std::string& prefix,
+    ProxyConfigServiceAndroid::Delegate* delegate,
+    ProxyServer::Scheme scheme) {
+  DCHECK(!prefix.empty());
+  std::string proxy_host = delegate->GetProperty(prefix + ".proxyHost");
+  if (!proxy_host.empty()) {
+    std::string proxy_port = delegate->GetProperty(prefix + ".proxyPort");
+    return ConstructProxyServer(scheme, proxy_host, proxy_port);
+  }
+  // Fall back to default proxy, if any.
+  proxy_host = delegate->GetProperty("proxyHost");
+  if (!proxy_host.empty()) {
+    std::string proxy_port = delegate->GetProperty("proxyPort");
+    return ConstructProxyServer(scheme, proxy_host, proxy_port);
+  }
+  return ProxyServer();
+}
+
+ProxyServer LookupSocksProxy(
+    ProxyConfigServiceAndroid::Delegate* delegate) {
+  std::string proxy_host = delegate->GetProperty("socksProxyHost");
+  if (!proxy_host.empty()) {
+    std::string proxy_port = delegate->GetProperty("socksProxyPort");
+    return ConstructProxyServer(ProxyServer::SCHEME_SOCKS5, proxy_host,
+                                proxy_port);
+  }
+  return ProxyServer();
+}
+
+void AddBypassRules(
+    const std::string& scheme,
+    ProxyConfigServiceAndroid::Delegate* delegate,
+    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 non_proxy_hosts =
+      delegate->GetProperty(scheme + ".nonProxyHosts");
+  if (non_proxy_hosts.empty())
+    return;
+  StringTokenizer tokenizer(non_proxy_hosts, "|");
+  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 if 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.

[Ryan Sleevi, 2012/05/14 18:00:25]

ocd nit: It's been raised by reviewers of my code in the past to avoid writing
comments in first-person ("we're"). This is not a hard rule, but I think it's
justified, so I like to ask others to consider as well :)

// See libcore/luni/src/main/java/java/net/ProxySelectorImpl.java for
// the equivalent Android implementation.

[Philippe, 2012/05/15 16:12:48]

Done.

+  rules->type = ProxyConfig::ProxyRules::TYPE_PROXY_PER_SCHEME;
+  rules->proxy_for_http = LookupProxy("http", delegate,
+                                      ProxyServer::SCHEME_HTTP);
+  rules->proxy_for_https = LookupProxy("https", delegate,
+                                       ProxyServer::SCHEME_HTTPS);
+  rules->proxy_for_ftp = LookupProxy("ftp", delegate, ProxyServer::SCHEME_HTTP);
+  rules->fallback_proxy = LookupSocksProxy(delegate);
+  rules->bypass_rules.Clear();
+  AddBypassRules("ftp", delegate, &rules->bypass_rules);
+  AddBypassRules("http", delegate, &rules->bypass_rules);
+  AddBypassRules("https", delegate, &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();
+}
+
+// This class implements the link to Android's proxy broadcast mechanism.
+class DelegateImpl : public ProxyConfigServiceAndroid::Delegate {
+ public:
+  DelegateImpl() : service_(NULL) {}
+
+  virtual void Start(ProxyConfigServiceAndroid* service) OVERRIDE {
+    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));
+  }
+
+  virtual void Stop() OVERRIDE {
+    // 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());
+  }
+
+  virtual std::string GetProperty(const std::string& property) OVERRIDE {
+    // Use Java System.getProperty to get configuration information.
+    // Question: Conversion to/from UTF8 ok here?

[Ryan Sleevi, 2012/05/14 18:00:25]

Should there be a TODO and a bug followed for follow-up?

Otherwise, this comment will never go answered and just serve to confuse future
readers as to whether or not there is or isn't a bug a here.

[Philippe, 2012/05/15 16:12:48]

I made it a TODO and will file a bug.

[joth, 2012/06/21 01:12:43]

Was there a bug?
AFAICT UTF8 should be fine: the property names are well known and all simple
ascii.
The results should only be hostnames (with possible wild card or | delim) and so
again ascii. If we do get invalid uf8 back, the device is broken, but anyway the
converter will drop it and give us an empty string thereby disabling that part
of proxy support which is probably the least worse thing to do in that scenario.

[Philippe, 2012/06/21 11:32:55]

No, it seems that I forgot to create the bug :/ I'm glad that there is finally
no bug here. I will create a CL to remove the TODO.

Thanks Jonathan!

+    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());

[Ryan Sleevi, 2012/05/14 18:00:25]

ocd style: std::string() rather than ""

[Philippe, 2012/05/15 16:12:48]

Done.

+  }
+
+ private:
+  ProxyConfigServiceAndroid* service_;

[Ryan Sleevi, 2012/05/14 18:00:25]

Did I miss where |service_| is actually used here? Doesn't seem like you need it
(or need it to be passed in Start())

[Philippe, 2012/05/15 16:12:48]

No you didn't :) It was used before when we had NotifyTask. We don't need it
anymore. Start() uses |service| though when calling
Java_ProxyChangeListener_start() but we don't need to store it anymore.

+  base::android::ScopedJavaGlobalRef<jobject>
+      java_proxy_change_listener_android_;
+};
+
+}  // namespace
+
+struct ProxyConfigServiceAndroid::SharedState

[Ryan Sleevi, 2012/05/14 18:00:25]

style: Classes that are RefCountedThreadSafe<> should have private destructors,
which argues in favour of class vs struct (generally).

Also, DISALLOW_COPY_AND_ASSIGN(SharedState)

[Philippe, 2012/05/15 16:12:48]

Done.

+    : public base::RefCountedThreadSafe<SharedState> {
+  SharedState(Delegate* delegate) : delegate_(delegate) {

[Ryan Sleevi, 2012/05/14 18:00:25]

style: explicit SharedState(...) {

[Philippe, 2012/05/15 16:12:48]

Done.

+    DCHECK(delegate);
+  }
+
+  // Non-reassignable pointer thus safely readable by any thread.
+  const scoped_ptr<Delegate> delegate_;
+  // Note that this is only accessed from the observer thread.
+  ObserverList<Observer> observers_;
+};
+
+ProxyConfigServiceAndroid::ProxyConfigServiceAndroid(
+    scoped_refptr<base::SingleThreadTaskRunner> observer_runner)
+    : shared_state_(new SharedState(new DelegateImpl)),
+      observer_runner_(observer_runner) {
+  DCHECK(observer_runner_.get());
+}
+
+ProxyConfigServiceAndroid::ProxyConfigServiceAndroid(
+    scoped_refptr<base::SingleThreadTaskRunner> observer_runner,
+    ProxyConfigServiceAndroid::Delegate* delegate)
+    : shared_state_(new SharedState(delegate)),
+      observer_runner_(observer_runner) {
+  DCHECK(observer_runner_.get());
+}

[Ryan Sleevi, 2012/05/14 18:00:25]

style: Make sure that the order matches the header - that is, this should appear
after ProxySettingsChanged (line 247)

[Philippe, 2012/05/15 16:12:48]

Done.

+
+ProxyConfigServiceAndroid::~ProxyConfigServiceAndroid() {
+  shared_state_->delegate_->Stop();
+}
+
+// static
+bool ProxyConfigServiceAndroid::Register(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+
+void ProxyConfigServiceAndroid::AddObserver(Observer* observer) {
+  DCHECK(OnObserverThread());
+  if (shared_state_->observers_.size() == 0)
+    shared_state_->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().
+  shared_state_->observers_.AddObserver(observer);
+}
+
+void ProxyConfigServiceAndroid::RemoveObserver(Observer* observer) {
+  DCHECK(OnObserverThread());
+  shared_state_->observers_.RemoveObserver(observer);
+  if (shared_state_->observers_.size() == 0) {
+    shared_state_->delegate_->Stop();

[Ryan Sleevi, 2012/05/14 18:00:25]

style nit: Remove the braces around one-line ifs for consistency with the rest
of the file

[Philippe, 2012/05/15 16:12:48]

Done.

+  }
+}
+
+ProxyConfigService::ConfigAvailability
+ProxyConfigServiceAndroid::GetLatestProxyConfig(ProxyConfig* config) {
+  if (!config)
+    return ProxyConfigService::CONFIG_UNSET;
+  GetLatestProxyConfigInternal(shared_state_->delegate_.get(), config);
+  return ProxyConfigService::CONFIG_VALID;
+}
+
+void ProxyConfigServiceAndroid::ProxySettingsChanged() {
+  observer_runner_->PostTask(
+      FROM_HERE, base::Bind(&ProxySettingsChangedCallback, shared_state_));
+}
+
+// static
+void ProxyConfigServiceAndroid::ProxySettingsChangedCallback(
+    scoped_refptr<SharedState> callback_state) {
+  // Allow another task to be scheduled (before running observers).
+  ProxyConfig config;
+  GetLatestProxyConfigInternal(callback_state->delegate_.get(), &config);
+  FOR_EACH_OBSERVER(Observer, callback_state->observers_,
+                    OnProxyConfigChanged(config,
+                                         ProxyConfigService::CONFIG_VALID));
+}
+
+bool ProxyConfigServiceAndroid::OnObserverThread() const {
+  return observer_runner_->BelongsToCurrentThread();
+}
+
+} // namespace net