[GCM] Suppress connection attempts when there is no valid connection.

If the local machine's network is unavailable, there's no poin in attempting to
make connections. This change simplifies the connection listening logic and
drops connection attempts while it is known that the network is unavailable.

BUG=376556

Review URL: https://codereview.chromium.org/336473003

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

3 files changed, 59 insertions, 48 deletions

diff --git a/google_apis/gcm/engine/connection_factory_impl.cc b/google_apis/gcm/engine/connection_factory_impl.cc
index 9f71b90..bc5a745 100644
--- a/google_apis/gcm/engine/connection_factory_impl.cc
+++ b/google_apis/gcm/engine/connection_factory_impl.cc
@@ -57,6 +57,7 @@ ConnectionFactoryImpl::ConnectionFactoryImpl(
     pac_request_(NULL),
     connecting_(false),
     waiting_for_backoff_(false),
+    waiting_for_network_online_(false),
     logging_in_(false),
     recorder_(recorder),
     listener_(NULL),
@@ -65,8 +66,7 @@ ConnectionFactoryImpl::ConnectionFactoryImpl(
 }
 
 ConnectionFactoryImpl::~ConnectionFactoryImpl() {
-  net::NetworkChangeNotifier::RemoveIPAddressObserver(this);
-  net::NetworkChangeNotifier::RemoveConnectionTypeObserver(this);
+  net::NetworkChangeNotifier::RemoveNetworkChangeObserver(this);
   if (pac_request_) {
     network_session_->proxy_service()->CancelPacRequest(pac_request_);
     pac_request_ = NULL;
@@ -83,8 +83,8 @@ void ConnectionFactoryImpl::Initialize(
   backoff_entry_ = CreateBackoffEntry(&backoff_policy_);
   request_builder_ = request_builder;
 
-  net::NetworkChangeNotifier::AddIPAddressObserver(this);
-  net::NetworkChangeNotifier::AddConnectionTypeObserver(this);
+  net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
+  waiting_for_network_online_ = net::NetworkChangeNotifier::IsOffline();
   connection_handler_ = CreateConnectionHandler(
       base::TimeDelta::FromMilliseconds(kReadTimeoutMs),
       read_callback,
@@ -150,6 +150,8 @@ std::string ConnectionFactoryImpl::GetConnectionStateString() const {
     return "CONNECTING";
   if (waiting_for_backoff_)
     return "WAITING FOR BACKOFF";
+  if (waiting_for_network_online_)
+    return "WAITING FOR NETWORK CHANGE";
   return "NOT CONNECTED";
 }
 
@@ -182,6 +184,9 @@ void ConnectionFactoryImpl::SignalConnectionReset(
   CloseSocket();
   DCHECK(!IsEndpointReachable());
 
+  if (waiting_for_network_online_)
+    return;
+
   // Network changes get special treatment as they can trigger a one-off canary
   // request that bypasses backoff (but does nothing if a connection is in
   // progress). Other connection reset events can be ignored as a connection
@@ -208,8 +213,6 @@ void ConnectionFactoryImpl::SignalConnectionReset(
     // We shouldn't be in backoff in thise case.
     DCHECK_EQ(0, backoff_entry_->failure_count());
   }
-  DCHECK(!connecting_);
-  DCHECK(!waiting_for_backoff_);
 
   // At this point the last login time has been consumed or deemed irrelevant,
   // reset it.
@@ -229,21 +232,19 @@ base::TimeTicks ConnectionFactoryImpl::NextRetryAttempt() const {
   return backoff_entry_->GetReleaseTime();
 }
 
-void ConnectionFactoryImpl::OnConnectionTypeChanged(
+void ConnectionFactoryImpl::OnNetworkChanged(
     net::NetworkChangeNotifier::ConnectionType type) {
-  if (type == net::NetworkChangeNotifier::CONNECTION_NONE)
+  if (type == net::NetworkChangeNotifier::CONNECTION_NONE) {
+    DVLOG(1) << "Network lost, resettion connection.";
+    waiting_for_network_online_ = true;
+
+    // Will do nothing due to |waiting_for_network_online_ == true|.
+    SignalConnectionReset(NETWORK_CHANGE);
     return;
+  }
 
   DVLOG(1) << "Connection type changed to " << type << ", reconnecting.";
-
-  // The connection may have been silently dropped, attempt to reconnect.
-  SignalConnectionReset(NETWORK_CHANGE);
-}
-
-void ConnectionFactoryImpl::OnIPAddressChanged() {
-  DVLOG(1) << "IP Address changed, reconnecting.";
-
-  // The connection may have been silently dropped, attempt to reconnect.
+  waiting_for_network_online_ = false;
   SignalConnectionReset(NETWORK_CHANGE);
 }
 
@@ -271,6 +272,9 @@ void ConnectionFactoryImpl::ConnectImpl() {
   DCHECK(!IsEndpointReachable());
   DCHECK(!socket_handle_.socket());
 
+  if (waiting_for_network_online_)
+    return;
+
   connecting_ = true;
   GURL current_endpoint = GetCurrentEndpoint();
   recorder_->RecordConnectionInitiated(current_endpoint.host());
diff --git a/google_apis/gcm/engine/connection_factory_impl.h b/google_apis/gcm/engine/connection_factory_impl.h
index 31cecf6..80fbc94 100644
--- a/google_apis/gcm/engine/connection_factory_impl.h
+++ b/google_apis/gcm/engine/connection_factory_impl.h
@@ -29,8 +29,7 @@ class GCMStatsRecorder;
 
 class GCM_EXPORT ConnectionFactoryImpl :
     public ConnectionFactory,
-    public net::NetworkChangeNotifier::ConnectionTypeObserver,
-    public net::NetworkChangeNotifier::IPAddressObserver {
+    public net::NetworkChangeNotifier::NetworkChangeObserver {
  public:
   ConnectionFactoryImpl(
       const std::vector<GURL>& mcs_endpoints,
@@ -53,10 +52,9 @@ class GCM_EXPORT ConnectionFactoryImpl :
   virtual void SignalConnectionReset(ConnectionResetReason reason) OVERRIDE;
   virtual void SetConnectionListener(ConnectionListener* listener) OVERRIDE;
 
-  // NetworkChangeNotifier observer implementations.
-  virtual void OnConnectionTypeChanged(
+  // NetworkChangeObserver implementation.
+  virtual void OnNetworkChanged(
       net::NetworkChangeNotifier::ConnectionType type) OVERRIDE;
-  virtual void OnIPAddressChanged() OVERRIDE;
 
   // Returns the server to which the factory is currently connected, or if
   // a connection is currently pending, the server to which the next connection
@@ -149,6 +147,11 @@ class GCM_EXPORT ConnectionFactoryImpl :
   // expiration.
   bool waiting_for_backoff_;
 
+  // Whether the NetworkChangeNotifier has informed the client that there is
+  // no current connection. No connection attempts will be made until the
+  // client is informed of a valid connection type.
+  bool waiting_for_network_online_;
+
   // Whether login successfully completed after the connection was established.
   // If a connection reset happens while attempting to log in, the current
   // backoff entry is reused (after incrementing with a new failure).
diff --git a/google_apis/gcm/engine/connection_factory_impl_unittest.cc b/google_apis/gcm/engine/connection_factory_impl_unittest.cc
index ef16183..da9485f 100644
--- a/google_apis/gcm/engine/connection_factory_impl_unittest.cc
+++ b/google_apis/gcm/engine/connection_factory_impl_unittest.cc
@@ -387,8 +387,8 @@ TEST_F(ConnectionFactoryImplTest, MultipleFailuresThenSucceed) {
   EXPECT_TRUE(connected_server().is_valid());
 }
 
-// IP events should trigger canary connections.
-TEST_F(ConnectionFactoryImplTest, FailThenIPEvent) {
+// Network change events should trigger canary connections.
+TEST_F(ConnectionFactoryImplTest, FailThenNetworkChangeEvent) {
   factory()->SetConnectResult(net::ERR_CONNECTION_FAILED);
   factory()->Connect();
   WaitForConnections();
@@ -396,26 +396,7 @@ TEST_F(ConnectionFactoryImplTest, FailThenIPEvent) {
   EXPECT_FALSE(initial_backoff.is_null());
 
   factory()->SetConnectResult(net::ERR_FAILED);
-  factory()->OnIPAddressChanged();
-  WaitForConnections();
-
-  // Backoff should increase.
-  base::TimeTicks next_backoff = factory()->NextRetryAttempt();
-  EXPECT_GT(next_backoff, initial_backoff);
-  EXPECT_FALSE(factory()->IsEndpointReachable());
-}
-
-// Connection type events should trigger canary connections.
-TEST_F(ConnectionFactoryImplTest, FailThenConnectionTypeEvent) {
-  factory()->SetConnectResult(net::ERR_CONNECTION_FAILED);
-  factory()->Connect();
-  WaitForConnections();
-  base::TimeTicks initial_backoff = factory()->NextRetryAttempt();
-  EXPECT_FALSE(initial_backoff.is_null());
-
-  factory()->SetConnectResult(net::ERR_FAILED);
-  factory()->OnConnectionTypeChanged(
-      net::NetworkChangeNotifier::CONNECTION_WIFI);
+  factory()->OnNetworkChanged(net::NetworkChangeNotifier::CONNECTION_WIFI);
   WaitForConnections();
 
   // Backoff should increase.
@@ -434,8 +415,7 @@ TEST_F(ConnectionFactoryImplTest, CanarySucceedsThenDisconnects) {
   EXPECT_FALSE(initial_backoff.is_null());
 
   factory()->SetConnectResult(net::OK);
-  factory()->OnConnectionTypeChanged(
-      net::NetworkChangeNotifier::CONNECTION_WIFI);
+  factory()->OnNetworkChanged(net::NetworkChangeNotifier::CONNECTION_ETHERNET);
   WaitForConnections();
   EXPECT_TRUE(factory()->IsEndpointReachable());
   EXPECT_TRUE(connected_server().is_valid());
@@ -460,8 +440,7 @@ TEST_F(ConnectionFactoryImplTest, CanarySucceedsRetryDuringLogin) {
 
   factory()->SetDelayLogin(true);
   factory()->SetConnectResult(net::OK);
-  factory()->OnConnectionTypeChanged(
-      net::NetworkChangeNotifier::CONNECTION_WIFI);
+  factory()->OnNetworkChanged(net::NetworkChangeNotifier::CONNECTION_WIFI);
   WaitForConnections();
   EXPECT_FALSE(factory()->IsEndpointReachable());
 
@@ -545,4 +524,29 @@ TEST_F(ConnectionFactoryImplTest, SignalResetRestoresBackoff) {
   EXPECT_FALSE(connected_server().is_valid());
 }
 
+// When the network is disconnected, close the socket and suppress further
+// connection attempts until the network returns.
+TEST_F(ConnectionFactoryImplTest, SuppressConnectWhenNoNetwork) {
+  factory()->SetConnectResult(net::OK);
+  factory()->Connect();
+  EXPECT_TRUE(factory()->NextRetryAttempt().is_null());
+  EXPECT_TRUE(factory()->IsEndpointReachable());
+
+  // Advance clock so the login window reset isn't encountered.
+  factory()->tick_clock()->Advance(base::TimeDelta::FromSeconds(11));
+
+  // Will trigger reset, but will not attempt a new connection.
+  factory()->OnNetworkChanged(net::NetworkChangeNotifier::CONNECTION_NONE);
+  EXPECT_FALSE(factory()->IsEndpointReachable());
+  EXPECT_TRUE(factory()->NextRetryAttempt().is_null());
+
+  // When the network returns, attempt to connect.
+  factory()->SetConnectResult(net::OK);
+  factory()->OnNetworkChanged(net::NetworkChangeNotifier::CONNECTION_4G);
+  WaitForConnections();
+
+  EXPECT_TRUE(factory()->IsEndpointReachable());
+  EXPECT_TRUE(factory()->NextRetryAttempt().is_null());
+}
+
 }  // namespace gcm