Fix potential threading issues in NetworkChangeNotifierAndroid.

NetworkChangeNotifierAndroid's constructor can be called on any thread. It
calls some Java code (still on any thread) initializing a singleton.
This singleton is also accessed from the UI thread on the application side.

While in practice it didn't seem to be a problem it seems safer to perform all
the JNI calls on a single thread (UI thread) to avoid accessing mutable state
from multiple threads.

Another potential issue with the current implementation is that
NetworkChangeNotifierAndroid could be deleted on the network thread while it
was receiving at the same time a notification on the UI thread. That could
have led to a user after free.

This CL addresses these two issues by introducing a Delegate which outlives
NetworkChangeNotifierAndroid and calls JNI methods on the UI thread.


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

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

1 files changed, 137 insertions, 54 deletions

diff --git a/net/android/network_change_notifier_android_unittest.cc b/net/android/network_change_notifier_android_unittest.cc
index 9b86470..b66d647 100644
--- a/net/android/network_change_notifier_android_unittest.cc
+++ b/net/android/network_change_notifier_android_unittest.cc
@@ -2,10 +2,15 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "net/android/network_change_notifier_android.h"
+// See network_change_notifier_android.h for design explanations.
 
+#include "base/basictypes.h"
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/compiler_specific.h"
 #include "base/message_loop.h"
-#include "net/android/network_change_notifier_factory_android.h"
+#include "net/android/network_change_notifier_android.h"
+#include "net/android/network_change_notifier_delegate_android.h"
 #include "net/base/network_change_notifier.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -13,22 +18,26 @@ namespace net {
 
 namespace {
 
-class TestConnectionTypeObserver :
-    public NetworkChangeNotifier::ConnectionTypeObserver {
+// Template used to generate both the NetworkChangeNotifierDelegateAndroid and
+// NetworkChangeNotifier::ConnectionTypeObserver implementations which have the
+// same interface.
+template <typename BaseObserver>
+class ObserverImpl : public BaseObserver {
  public:
-  TestConnectionTypeObserver() :
-      times_connection_type_has_been_changed_(0),
-      current_connection_(NetworkChangeNotifier::CONNECTION_UNKNOWN) {
+  ObserverImpl()
+      : times_connection_type_changed_(0),
+        current_connection_(NetworkChangeNotifier::CONNECTION_UNKNOWN) {
   }
 
-  void OnConnectionTypeChanged(
-      NetworkChangeNotifier::ConnectionType type) {
-    times_connection_type_has_been_changed_++;
+  // BaseObserver:
+  virtual void OnConnectionTypeChanged(
+      NetworkChangeNotifier::ConnectionType type) OVERRIDE {
+    times_connection_type_changed_++;
     current_connection_ = type;
   }
 
-  int times_connection_type_has_been_changed() const {
-    return times_connection_type_has_been_changed_;
+  int times_connection_type_changed() const {
+    return times_connection_type_changed_;
   }
 
   NetworkChangeNotifier::ConnectionType current_connection() const {
@@ -36,69 +45,143 @@ class TestConnectionTypeObserver :
   }
 
  private:
-  int times_connection_type_has_been_changed_;
+  int times_connection_type_changed_;
   NetworkChangeNotifier::ConnectionType current_connection_;
 
-  DISALLOW_COPY_AND_ASSIGN(TestConnectionTypeObserver);
+  DISALLOW_COPY_AND_ASSIGN(ObserverImpl);
 };
 
 }  // namespace
 
-class NetworkChangeNotifierAndroidTest : public testing::Test {
- public:
-  NetworkChangeNotifierAndroidTest() : connection_type_observer_(NULL) {
+class BaseNetworkChangeNotifierAndroidTest : public testing::Test {
+ protected:
+  typedef NetworkChangeNotifier::ConnectionType ConnectionType;
+
+  virtual ~BaseNetworkChangeNotifierAndroidTest() {}
+
+  void RunTest(
+      const base::Callback<int(void)>& times_connection_type_changed_callback,
+      const base::Callback<ConnectionType(void)>&  connection_type_getter) {
+    EXPECT_EQ(0, times_connection_type_changed_callback.Run());
+    EXPECT_EQ(NetworkChangeNotifier::CONNECTION_UNKNOWN,
+              connection_type_getter.Run());
+
+    ForceConnectivityState(NetworkChangeNotifierDelegateAndroid::OFFLINE);
+    EXPECT_EQ(1, times_connection_type_changed_callback.Run());
+    EXPECT_EQ(NetworkChangeNotifier::CONNECTION_NONE,
+              connection_type_getter.Run());
+
+    ForceConnectivityState(NetworkChangeNotifierDelegateAndroid::OFFLINE);
+    EXPECT_EQ(1, times_connection_type_changed_callback.Run());
+    EXPECT_EQ(NetworkChangeNotifier::CONNECTION_NONE,
+              connection_type_getter.Run());
+
+    ForceConnectivityState(NetworkChangeNotifierDelegateAndroid::ONLINE);
+    EXPECT_EQ(2, times_connection_type_changed_callback.Run());
+    EXPECT_EQ(NetworkChangeNotifier::CONNECTION_UNKNOWN,
+              connection_type_getter.Run());
   }
 
-  void ForceConnectivityState(bool state) {
-    notifier_->ForceConnectivityState(state);
+  void ForceConnectivityState(
+      NetworkChangeNotifierDelegateAndroid::ConnectivityState state) {
+    delegate_.ForceConnectivityState(state);
+    // Note that this is needed because ObserverListThreadSafe uses PostTask().
+    MessageLoop::current()->RunUntilIdle();
   }
 
-  const TestConnectionTypeObserver* observer() const {
-    return connection_type_observer_.get();
-  }
+  NetworkChangeNotifierDelegateAndroid delegate_;
+};
 
+class NetworkChangeNotifierDelegateAndroidTest
+    : public BaseNetworkChangeNotifierAndroidTest {
  protected:
-  virtual void SetUp() {
-    notifier_.reset(new NetworkChangeNotifierAndroid());
-    connection_type_observer_.reset(new TestConnectionTypeObserver());
-    NetworkChangeNotifier::AddConnectionTypeObserver(
-        connection_type_observer_.get());
-  }
+  typedef ObserverImpl<
+      NetworkChangeNotifierDelegateAndroid::Observer> TestDelegateObserver;
 
- private:
-  NetworkChangeNotifier::DisableForTest disable_for_test_;
-  scoped_ptr<NetworkChangeNotifierAndroid> notifier_;
-  scoped_ptr<TestConnectionTypeObserver> connection_type_observer_;
-};
+  NetworkChangeNotifierDelegateAndroidTest() {
+    delegate_.AddObserver(&delegate_observer_);
+    delegate_.AddObserver(&other_delegate_observer_);
+  }
 
+  virtual ~NetworkChangeNotifierDelegateAndroidTest() {
+    delegate_.RemoveObserver(&delegate_observer_);
+    delegate_.RemoveObserver(&other_delegate_observer_);
+  }
 
-TEST_F(NetworkChangeNotifierAndroidTest, ObserverNotified) {
-  // This test exercises JNI calls between the native-side
-  // NetworkChangeNotifierAndroid and java-side NetworkChangeNotifier.
-  EXPECT_EQ(0, observer()->times_connection_type_has_been_changed());
-  EXPECT_EQ(NetworkChangeNotifier::CONNECTION_UNKNOWN,
-      observer()->current_connection());
+  TestDelegateObserver delegate_observer_;
+  TestDelegateObserver other_delegate_observer_;
+};
 
-  ForceConnectivityState(false);
-  MessageLoop::current()->RunUntilIdle();
+// Tests that the NetworkChangeNotifierDelegateAndroid's observers are notified.
+// A testing-only observer is used here for testing. In production the
+// delegate's observers are instances of NetworkChangeNotifierAndroid.
+TEST_F(NetworkChangeNotifierDelegateAndroidTest, DelegateObserverNotified) {
+  // Test the logic with a single observer.
+  RunTest(
+      base::Bind(
+          &TestDelegateObserver::times_connection_type_changed,
+          base::Unretained(&delegate_observer_)),
+      base::Bind(
+          &TestDelegateObserver::current_connection,
+          base::Unretained(&delegate_observer_)));
+  // Check that *all* the observers are notified. Both observers should have the
+  // same state.
+  EXPECT_EQ(delegate_observer_.times_connection_type_changed(),
+            other_delegate_observer_.times_connection_type_changed());
+  EXPECT_EQ(delegate_observer_.current_connection(),
+            other_delegate_observer_.current_connection());
+}
 
-  EXPECT_EQ(1, observer()->times_connection_type_has_been_changed());
-  EXPECT_EQ(NetworkChangeNotifier::CONNECTION_NONE,
-      observer()->current_connection());
+class NetworkChangeNotifierAndroidTest
+    : public BaseNetworkChangeNotifierAndroidTest {
+ protected:
+  typedef ObserverImpl<
+      NetworkChangeNotifier::ConnectionTypeObserver> TestConnectionTypeObserver;
 
-  ForceConnectivityState(false);
-  MessageLoop::current()->RunUntilIdle();
+  NetworkChangeNotifierAndroidTest() : notifier_(&delegate_) {
+    NetworkChangeNotifier::AddConnectionTypeObserver(
+        &connection_type_observer_);
+    NetworkChangeNotifier::AddConnectionTypeObserver(
+        &other_connection_type_observer_);
+  }
 
-  EXPECT_EQ(1, observer()->times_connection_type_has_been_changed());
-  EXPECT_EQ(NetworkChangeNotifier::CONNECTION_NONE,
-      observer()->current_connection());
+  TestConnectionTypeObserver connection_type_observer_;
+  TestConnectionTypeObserver other_connection_type_observer_;
+  NetworkChangeNotifier::DisableForTest disable_for_test_;
+  NetworkChangeNotifierAndroid notifier_;
+};
 
-  ForceConnectivityState(true);
-  MessageLoop::current()->RunUntilIdle();
+// When a NetworkChangeNotifierAndroid is observing a
+// NetworkChangeNotifierDelegateAndroid for network state changes, and the
+// NetworkChangeNotifierDelegateAndroid's connectivity state changes, the
+// NetworkChangeNotifierAndroid should reflect that state.
+TEST_F(NetworkChangeNotifierAndroidTest,
+       NotificationsSentToNetworkChangeNotifierAndroid) {
+  RunTest(
+      base::Bind(
+          &TestConnectionTypeObserver::times_connection_type_changed,
+          base::Unretained(&connection_type_observer_)),
+      base::Bind(
+          &NetworkChangeNotifierAndroid::GetCurrentConnectionType,
+          base::Unretained(&notifier_)));
+}
 
-  EXPECT_EQ(2, observer()->times_connection_type_has_been_changed());
-  EXPECT_EQ(NetworkChangeNotifier::CONNECTION_UNKNOWN,
-      observer()->current_connection());
+// When a NetworkChangeNotifierAndroid's connection state changes, it should
+// notify all of its observers.
+TEST_F(NetworkChangeNotifierAndroidTest,
+       NotificationsSentToClientsOfNetworkChangeNotifier) {
+  RunTest(
+      base::Bind(
+          &TestConnectionTypeObserver::times_connection_type_changed,
+          base::Unretained(&connection_type_observer_)),
+      base::Bind(
+          &TestConnectionTypeObserver::current_connection,
+          base::Unretained(&connection_type_observer_)));
+  // Check that *all* the observers are notified.
+  EXPECT_EQ(connection_type_observer_.times_connection_type_changed(),
+            other_connection_type_observer_.times_connection_type_changed());
+  EXPECT_EQ(connection_type_observer_.current_connection(),
+            other_connection_type_observer_.current_connection());
 }
 
 }  // namespace net