Move ProxyConfigService construction onto the IO thread.

net/proxy/proxy_config_service_android.cc

Index: net/proxy/proxy_config_service_android.cc
diff --git a/net/proxy/proxy_config_service_android.cc b/net/proxy/proxy_config_service_android.cc
index 6a0be78d020561dfc193375b40c051b47eaa2008..95c6980e7cb0f03c5abfdfdf4492a4212184f17c 100644
--- a/net/proxy/proxy_config_service_android.cc
+++ b/net/proxy/proxy_config_service_android.cc
@@ -161,16 +161,16 @@ class ProxyConfigServiceAndroid::Delegate
     : public base::RefCountedThreadSafe<Delegate> {
  public:
   Delegate(base::SequencedTaskRunner* network_task_runner,
-           base::SequencedTaskRunner* jni_task_runner,
+           base::SequencedTaskRunner* glib_task_runner,
            const GetPropertyCallback& get_property_callback)
       : ALLOW_THIS_IN_INITIALIZER_LIST(jni_delegate_(this)),
         network_task_runner_(network_task_runner),
-        jni_task_runner_(jni_task_runner),
+        glib_task_runner_(glib_task_runner),
         get_property_callback_(get_property_callback) {
   }
 
   void SetupJNI() {
-    DCHECK(OnJNIThread());
+    DCHECK(OnNetworkThread());
     JNIEnv* env = AttachCurrentThread();
     if (java_proxy_change_listener_.is_null()) {
       java_proxy_change_listener_.Reset(
@@ -185,22 +185,16 @@ class ProxyConfigServiceAndroid::Delegate
   }
 
   void FetchInitialConfig() {
-    DCHECK(OnJNIThread());
-    ProxyConfig proxy_config;
-    GetLatestProxyConfigInternal(get_property_callback_, &proxy_config);
-    network_task_runner_->PostTask(
-        FROM_HERE,
-        base::Bind(&Delegate::SetNewConfigOnNetworkThread, this, proxy_config));
+    DCHECK(OnNetworkThread());
+    SetNewConfigOnNetworkThread();
   }
 
-  void Shutdown() {
-    if (OnJNIThread()) {
-      ShutdownOnJNIThread();
-    } else {
-      jni_task_runner_->PostTask(
-          FROM_HERE,
-          base::Bind(&Delegate::ShutdownOnJNIThread, this));
-    }
+  void StartTearDown() {

[Philippe, 2012/10/19 09:35:25]

Sorry, I didn't see at first that this was called on the JNI thread. I'm
concerned about creating the Java non-stateless instance on the Network thread
and then destroying it on the JNI thread.

What do you think?

I think this could be addressed by keeping the initial design (i.e. all the JNI
calls performed on the JNI thread).
You would have to add an extra Init() method called on the network thread that
would post SetupJNI() on the JNI thread.
You can also merge SetupJNI() and FetchInitialConfig() into a InitOnJNIThread()
method which would setup JNI then fetch the initial config.

How does that sound?

[pauljensen, 2012/10/19 16:00:13]

I wondered about all this JNI thread interaction and discussed with sleevi and
yourself in an e-mail thread on 9/13/12.  

With regards to stopping the notifications on the UI thread: The notifications
from Java to C++ will always arrive on the UI thread and because of this I feel
we need to disable them on the same thread to keep things simple and avoid
races.  This is especially simple because StartTearDown() arrives on the UI
thread.

With regards to commencing the notifications (via SetupJNI()) on the network
thread:  Since the ProxyConfigServices are now created on the network thread it
seemed simplest to do it right on the same thread rather than post over to the
other thread.  sleevi said tedchoc said this was OK.  I guess I figured since
the notifications will arrive on the UI thread no matter where I call
Java_ProxyChangeListener_start() there wasn't must point to posting from one
thread to another to call the same function.

With regards to querying the proxy info from the networking thread:  This again
made things simpler by avoiding the previous PostTask() in FetchInitialConfig().
 My impression is that getting the initial proxy config quickly is also
important so this helps.

[Philippe, 2012/10/19 16:34:12]

I agree that destruction should happen on the UI thread (as before).

I was just concerned about possible races on the Java side (see
ProxyChangeListener.java) due to non-synchronized concurrent accesses (UI vs I/O
thread) to |mNativePtr|, |mProxyReceiver|, |mContext|.
It is probably fine in practice with the current Java-side implementation but
that could change later.

I agree too that your design minimizes the amount of PostTask() and makes the
initial proxy config available as soon as possible which is good.

+    DCHECK(OnGlibThread());
+    if (java_proxy_change_listener_.is_null())
+      return;
+    JNIEnv* env = AttachCurrentThread();
+    Java_ProxyChangeListener_stop(env, java_proxy_change_listener_.obj());
   }
 
   // Called only on the network thread.
@@ -222,15 +216,12 @@ class ProxyConfigServiceAndroid::Delegate
     return ProxyConfigService::CONFIG_VALID;
   }
 
-  // Called on the JNI thread.
+  // Called on the glib thread.
   void ProxySettingsChanged() {
-    DCHECK(OnJNIThread());
-    ProxyConfig proxy_config;
-    GetLatestProxyConfigInternal(get_property_callback_, &proxy_config);
+    DCHECK(OnGlibThread());
     network_task_runner_->PostTask(
         FROM_HERE,
-        base::Bind(
-            &Delegate::SetNewConfigOnNetworkThread, this, proxy_config));
+        base::Bind(&Delegate::SetNewConfigOnNetworkThread, this));
   }
 
  private:
@@ -251,24 +242,17 @@ class ProxyConfigServiceAndroid::Delegate
 
   virtual ~Delegate() {}
 
-  void ShutdownOnJNIThread() {
-    if (java_proxy_change_listener_.is_null())
-      return;
-    JNIEnv* env = AttachCurrentThread();
-    Java_ProxyChangeListener_stop(env, java_proxy_change_listener_.obj());
-  }
-
   // Called on the network thread.
-  void SetNewConfigOnNetworkThread(const ProxyConfig& proxy_config) {
+  void SetNewConfigOnNetworkThread() {
     DCHECK(OnNetworkThread());
-    proxy_config_ = proxy_config;
+    GetLatestProxyConfigInternal(get_property_callback_, &proxy_config_);
     FOR_EACH_OBSERVER(Observer, observers_,
-                      OnProxyConfigChanged(proxy_config,
+                      OnProxyConfigChanged(proxy_config_,
                                            ProxyConfigService::CONFIG_VALID));
   }
 
-  bool OnJNIThread() const {
-    return jni_task_runner_->RunsTasksOnCurrentThread();
+  bool OnGlibThread() const {
+    return glib_task_runner_->RunsTasksOnCurrentThread();
   }
 
   bool OnNetworkThread() const {
@@ -280,7 +264,7 @@ class ProxyConfigServiceAndroid::Delegate
   JNIDelegateImpl jni_delegate_;
   ObserverList<Observer> observers_;
   scoped_refptr<base::SequencedTaskRunner> network_task_runner_;
-  scoped_refptr<base::SequencedTaskRunner> jni_task_runner_;
+  scoped_refptr<base::SequencedTaskRunner> glib_task_runner_;
   GetPropertyCallback get_property_callback_;
   ProxyConfig proxy_config_;
 
@@ -288,16 +272,18 @@ class ProxyConfigServiceAndroid::Delegate
 };
 
 ProxyConfigServiceAndroid::ProxyConfigServiceAndroid(
-    base::SequencedTaskRunner* network_task_runner,
-    base::SequencedTaskRunner* jni_task_runner)
+    base::SequencedTaskRunner* glib_task_runner,
+    base::SequencedTaskRunner* network_task_runner)
     : delegate_(new Delegate(
-        network_task_runner, jni_task_runner, base::Bind(&GetJavaProperty))) {
+        network_task_runner, glib_task_runner, base::Bind(&GetJavaProperty))) {
   delegate_->SetupJNI();
   delegate_->FetchInitialConfig();
 }
 
-ProxyConfigServiceAndroid::~ProxyConfigServiceAndroid() {
-  delegate_->Shutdown();
+ProxyConfigServiceAndroid::~ProxyConfigServiceAndroid() {}
+
+void ProxyConfigServiceAndroid::StartTearDown() {
+  delegate_->StartTearDown();
 }
 
 // static
@@ -320,10 +306,10 @@ ProxyConfigServiceAndroid::GetLatestProxyConfig(ProxyConfig* config) {
 
 ProxyConfigServiceAndroid::ProxyConfigServiceAndroid(
     base::SequencedTaskRunner* network_task_runner,
-    base::SequencedTaskRunner* jni_task_runner,
+    base::SequencedTaskRunner* glib_task_runner,
     GetPropertyCallback get_property_callback)
     : delegate_(new Delegate(
-        network_task_runner, jni_task_runner, get_property_callback)) {
+        network_task_runner, glib_task_runner, get_property_callback)) {
   delegate_->FetchInitialConfig();
 }