Fix race condition in NetworkChangeNotifier on Android.

NetworkChangeNotifier::GetCurrentConnectionType() is called on any thread,
including the network thread.
Its implementation on Android calls some Java code (from any thread) which
indirectly reads a primitive member variable set from the UI thread.

The lack of synchronization could make
NetworkChangeNotifier::GetCurrentConnectionType() return an out-dated result on
multi-core devices.

BUG=143433

Review URL: https://chromiumcodereview.appspot.com/10905264

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@157804 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 112 insertions, 73 deletions

diff --git a/net/android/java/src/org/chromium/net/NetworkChangeNotifier.java b/net/android/java/src/org/chromium/net/NetworkChangeNotifier.java
index 7f1bdc3..d540bc3 100644
--- a/net/android/java/src/org/chromium/net/NetworkChangeNotifier.java
+++ b/net/android/java/src/org/chromium/net/NetworkChangeNotifier.java
@@ -31,7 +31,6 @@ public class NetworkChangeNotifier {
 
     private final Context mContext;
     private int mNativeChangeNotifier;
-    private int mConnectionType;
     private NetworkChangeNotifierAutoDetect mAutoDetector;
 
     private static NetworkChangeNotifier sInstance;
@@ -41,7 +40,6 @@ public class NetworkChangeNotifier {
     private NetworkChangeNotifier(Context context, int nativeChangeNotifier) {
         mContext = context;
         mNativeChangeNotifier = nativeChangeNotifier;
-        mConnectionType = CONNECTION_UNKNOWN;
     }
 
     private void destroy() {
@@ -94,16 +92,27 @@ public class NetworkChangeNotifier {
         sInstance.setAutoDetectConnectivityStateInternal(shouldAutoDetect);
     }
 
+    private void destroyAutoDetector() {
+        if (mAutoDetector != null) {
+            mAutoDetector.destroy();
+            mAutoDetector = null;
+        }
+    }
+
     private void setAutoDetectConnectivityStateInternal(boolean shouldAutoDetect) {
         if (shouldAutoDetect) {
             if (mAutoDetector == null) {
-                mAutoDetector = new NetworkChangeNotifierAutoDetect(this, mContext);
+                mAutoDetector = new NetworkChangeNotifierAutoDetect(
+                    new NetworkChangeNotifierAutoDetect.Observer() {
+                        @Override
+                        public void onConnectionTypeChanged(int newConnectionType) {
+                            notifyObserversOfConnectionTypeChange(newConnectionType);
+                        }
+                    },
+                    mContext);
             }
         } else {
-            if (mAutoDetector != null) {
-                mAutoDetector.destroy();
-                mAutoDetector = null;
-            }
+            destroyAutoDetector();
         }
     }
 
@@ -122,29 +131,29 @@ public class NetworkChangeNotifier {
     }
 
     private void forceConnectivityStateInternal(boolean forceOnline) {
-        boolean connectionCurrentlyExists = mConnectionType != CONNECTION_NONE;
+        if (mNativeChangeNotifier == 0) {
+            return;
+        }
+        boolean connectionCurrentlyExists =
+            nativeGetConnectionType(mNativeChangeNotifier) != CONNECTION_NONE;
         if (connectionCurrentlyExists != forceOnline) {
-            mConnectionType = forceOnline ? CONNECTION_UNKNOWN : CONNECTION_NONE;
-            notifyObserversOfConnectionTypeChange();
+            notifyObserversOfConnectionTypeChange(
+                    forceOnline ? CONNECTION_UNKNOWN : CONNECTION_NONE);
         }
     }
 
-    void notifyObserversOfConnectionTypeChange() {
+    void notifyObserversOfConnectionTypeChange(int newConnectionType) {
         if (mNativeChangeNotifier != 0) {
-            nativeNotifyObserversOfConnectionTypeChange(mNativeChangeNotifier);
+            nativeNotifyObserversOfConnectionTypeChange(mNativeChangeNotifier, newConnectionType);
         }
     }
 
-    @CalledByNative
-    private int connectionType() {
-        if (mAutoDetector != null) {
-            return mAutoDetector.connectionType();
-        }
-        return mConnectionType;
-    }
+    @NativeClassQualifiedName("NetworkChangeNotifierAndroid")
+    private native void nativeNotifyObserversOfConnectionTypeChange(
+            int nativePtr, int newConnectionType);
 
     @NativeClassQualifiedName("NetworkChangeNotifierAndroid")
-    private native void nativeNotifyObserversOfConnectionTypeChange(int nativePtr);
+    private native int nativeGetConnectionType(int nativePtr);
 
     // For testing only.
     public static NetworkChangeNotifierAutoDetect getAutoDetectorForTest() {
diff --git a/net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java b/net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
index 573fb9e..9118694 100644
--- a/net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
+++ b/net/android/java/src/org/chromium/net/NetworkChangeNotifierAutoDetect.java
@@ -28,14 +28,21 @@ public class NetworkChangeNotifierAutoDetect extends BroadcastReceiver
     private final NetworkConnectivityIntentFilter mIntentFilter =
             new NetworkConnectivityIntentFilter();
 
-    private final NetworkChangeNotifier mOwner;
+    private final Observer mObserver;
 
     private final Context mContext;
     private boolean mRegistered;
     private int mConnectionType;
 
-    public NetworkChangeNotifierAutoDetect(NetworkChangeNotifier owner, Context context) {
-        mOwner = owner;
+    /**
+     * Observer notified on the UI thread whenever a new connection type was detected.
+     */
+    public static interface Observer {
+        public void onConnectionTypeChanged(int newConnectionType);
+    }
+
+    public NetworkChangeNotifierAutoDetect(Observer observer, Context context) {
+        mObserver = observer;
         mContext = context;
         mConnectionType = currentConnectionType(context);
 
@@ -46,10 +53,6 @@ public class NetworkChangeNotifierAutoDetect extends BroadcastReceiver
         status.registerListener(this);
     }
 
-    public int connectionType() {
-        return mConnectionType;
-    }
-
     public void destroy() {
         unregisterReceiver();
     }
@@ -130,7 +133,7 @@ public class NetworkChangeNotifierAutoDetect extends BroadcastReceiver
         if (newConnectionType != mConnectionType) {
             mConnectionType = newConnectionType;
             Log.d(TAG, "Network connectivity changed, type is: " + mConnectionType);
-            mOwner.notifyObserversOfConnectionTypeChange();
+            mObserver.onConnectionTypeChanged(newConnectionType);
         }
     }
 
diff --git a/net/android/network_change_notifier_android.cc b/net/android/network_change_notifier_android.cc
index 66aacaf..f055af9 100644
--- a/net/android/network_change_notifier_android.cc
+++ b/net/android/network_change_notifier_android.cc
@@ -4,33 +4,72 @@
 
 #include "net/android/network_change_notifier_android.h"
 
-#include "base/logging.h"
 #include "base/android/jni_android.h"
+#include "base/logging.h"
 #include "jni/NetworkChangeNotifier_jni.h"
 
 namespace net {
 
-NetworkChangeNotifierAndroid::NetworkChangeNotifierAndroid() {
-  JNIEnv* env = base::android::AttachCurrentThread();
-  java_network_change_notifier_.Reset(
-      Java_NetworkChangeNotifier_createInstance(
-          env,
-          base::android::GetApplicationContext(),
-          reinterpret_cast<jint>(this)));
+namespace {
+
+// Returns whether the provided connection type is known.
+bool CheckConnectionType(int connection_type) {
+  switch (connection_type) {
+    case NetworkChangeNotifier::CONNECTION_UNKNOWN:
+    case NetworkChangeNotifier::CONNECTION_ETHERNET:
+    case NetworkChangeNotifier::CONNECTION_WIFI:
+    case NetworkChangeNotifier::CONNECTION_2G:
+    case NetworkChangeNotifier::CONNECTION_3G:
+    case NetworkChangeNotifier::CONNECTION_4G:
+    case NetworkChangeNotifier::CONNECTION_NONE:
+      return true;
+    default:
+      NOTREACHED() << "Unknown connection type received: " << connection_type;
+      return false;
+  }
 }
 
+}  // namespace
+
 NetworkChangeNotifierAndroid::~NetworkChangeNotifierAndroid() {
   JNIEnv* env = base::android::AttachCurrentThread();
   Java_NetworkChangeNotifier_destroyInstance(env);
-  java_network_change_notifier_.Reset();
 }
 
 void NetworkChangeNotifierAndroid::NotifyObserversOfConnectionTypeChange(
     JNIEnv* env,
-    jobject obj) {
+    jobject obj,
+    jint new_connection_type) {
+  int connection_type = CheckConnectionType(new_connection_type) ?
+      new_connection_type : CONNECTION_UNKNOWN;
+  SetConnectionType(connection_type);
   NetworkChangeNotifier::NotifyObserversOfConnectionTypeChange();
 }
 
+jint NetworkChangeNotifierAndroid::GetConnectionType(JNIEnv* env, jobject obj) {
+  return GetCurrentConnectionType();
+}
+
+// static
+bool NetworkChangeNotifierAndroid::Register(JNIEnv* env) {
+  return RegisterNativesImpl(env);
+}
+
+NetworkChangeNotifierAndroid::NetworkChangeNotifierAndroid() {
+  SetConnectionType(CONNECTION_UNKNOWN);
+  JNIEnv* env = base::android::AttachCurrentThread();
+  java_network_change_notifier_.Reset(
+      Java_NetworkChangeNotifier_createInstance(
+          env,
+          base::android::GetApplicationContext(),
+          reinterpret_cast<jint>(this)));
+}
+
+void NetworkChangeNotifierAndroid::SetConnectionType(int connection_type) {
+  base::AutoLock auto_lock(lock_);
+  connection_type_ = connection_type;
+}
+
 void NetworkChangeNotifierAndroid::ForceConnectivityState(bool state) {
   JNIEnv* env = base::android::AttachCurrentThread();
   Java_NetworkChangeNotifier_forceConnectivityState(env, state);
@@ -38,36 +77,8 @@ void NetworkChangeNotifierAndroid::ForceConnectivityState(bool state) {
 
 NetworkChangeNotifier::ConnectionType
     NetworkChangeNotifierAndroid::GetCurrentConnectionType() const {
-  JNIEnv* env = base::android::AttachCurrentThread();
-
-  // Pull the connection type from the Java-side then convert it to a
-  // native-side NetworkChangeNotifier::ConnectionType.
-  jint connection_type = Java_NetworkChangeNotifier_connectionType(
-      env, java_network_change_notifier_.obj());
-  switch (connection_type) {
-    case CONNECTION_UNKNOWN:
-      return CONNECTION_UNKNOWN;
-    case CONNECTION_ETHERNET:
-      return CONNECTION_ETHERNET;
-    case CONNECTION_WIFI:
-      return CONNECTION_WIFI;
-    case CONNECTION_2G:
-      return CONNECTION_2G;
-    case CONNECTION_3G:
-      return CONNECTION_3G;
-    case CONNECTION_4G:
-      return CONNECTION_4G;
-    case CONNECTION_NONE:
-      return CONNECTION_NONE;
-    default:
-      NOTREACHED() << "Unknown connection type received: " << connection_type;
-      return CONNECTION_NONE;
-  }
-}
-
-// static
-bool NetworkChangeNotifierAndroid::Register(JNIEnv* env) {
-  return RegisterNativesImpl(env);
+  base::AutoLock auto_lock(lock_);
+  return static_cast<NetworkChangeNotifier::ConnectionType>(connection_type_);
 }
 
 }  // namespace net
diff --git a/net/android/network_change_notifier_android.h b/net/android/network_change_notifier_android.h
index 2824600..f5311ac 100644
--- a/net/android/network_change_notifier_android.h
+++ b/net/android/network_change_notifier_android.h
@@ -8,6 +8,7 @@
 #include "base/android/scoped_java_ref.h"
 #include "base/basictypes.h"
 #include "base/compiler_specific.h"
+#include "base/synchronization/lock.h"
 #include "net/base/network_change_notifier.h"
 
 namespace net {
@@ -16,23 +17,35 @@ class NetworkChangeNotifierAndroidTest;
 
 class NetworkChangeNotifierAndroid : public NetworkChangeNotifier {
  public:
-  NetworkChangeNotifierAndroid();
   virtual ~NetworkChangeNotifierAndroid();
 
-  void NotifyObserversOfConnectionTypeChange(JNIEnv* env, jobject obj);
+  // Called from Java on the UI thread.
+  void NotifyObserversOfConnectionTypeChange(
+      JNIEnv* env, jobject obj, jint new_connection_type);
+  jint GetConnectionType(JNIEnv* env, jobject obj);
 
   static bool Register(JNIEnv* env);
 
  private:
   friend class NetworkChangeNotifierAndroidTest;
+  friend class NetworkChangeNotifierFactoryAndroid;
 
-  // NetworkChangeNotifier:
-  virtual NetworkChangeNotifier::ConnectionType
-      GetCurrentConnectionType() const OVERRIDE;
+  NetworkChangeNotifierAndroid();
+
+  void SetConnectionType(int connection_type);
 
   void ForceConnectivityState(bool state);
 
+  // NetworkChangeNotifier:
+  virtual ConnectionType GetCurrentConnectionType() const OVERRIDE;
+
   base::android::ScopedJavaGlobalRef<jobject> java_network_change_notifier_;
+  // TODO(pliard): http://crbug.com/150867. Use an atomic integer for the
+  // connection type without the lock once a non-subtle atomic integer is
+  // available under base/. That might never happen though.
+  mutable base::Lock lock_;  // Protects the state below.
+  // Written from the UI thread, read from any thread.
+  int connection_type_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkChangeNotifierAndroid);
 };
diff --git a/net/android/network_change_notifier_factory_android.cc b/net/android/network_change_notifier_factory_android.cc
index f15b39a..a28a69e 100644
--- a/net/android/network_change_notifier_factory_android.cc
+++ b/net/android/network_change_notifier_factory_android.cc
@@ -10,6 +10,8 @@ namespace net {
 
 NetworkChangeNotifierFactoryAndroid::NetworkChangeNotifierFactoryAndroid() {}
 
+NetworkChangeNotifierFactoryAndroid::~NetworkChangeNotifierFactoryAndroid() {}
+
 NetworkChangeNotifier* NetworkChangeNotifierFactoryAndroid::CreateInstance() {
   return new NetworkChangeNotifierAndroid();
 }
diff --git a/net/android/network_change_notifier_factory_android.h b/net/android/network_change_notifier_factory_android.h
index 3f5f0b6..d4bd99f 100644
--- a/net/android/network_change_notifier_factory_android.h
+++ b/net/android/network_change_notifier_factory_android.h
@@ -18,6 +18,7 @@ class NetworkChangeNotifierFactoryAndroid :
     public NetworkChangeNotifierFactory {
  public:
   NetworkChangeNotifierFactoryAndroid();
+  virtual ~NetworkChangeNotifierFactoryAndroid();
 
   // Overrides of NetworkChangeNotifierFactory.
   virtual NetworkChangeNotifier* CreateInstance() OVERRIDE;