Report InvalidatorState correctly from GCMNetworkChannel

The problem:
Sync observes Invalidator state to decide whenever to use PreCommit GU
optimization, therefore INVALIDATIONS_ENABLED should be reported only
when it is certain that communication channel between client and
invalidations server works both ways.
SyncNetworkChannel::NotifyStateChange is currently used for two purposes:
- Notify observers of invalidations about InvalidatorState
- Nudge cacheinvalidations to send heartbeat on network status change.
Because of this dual purpose it is called with INVALIDATIONS_ENABLED when
network status changes. In this case sync is notified that invalidations
are enabled while there is no GCM connection established.

Solution:
- Separate two purposes into separate functions and call them appropriately
- Only set INVALIDATIONS_ENABLED when it is confirmed that both http and
gcm channels are working.

BUG=377911
R=rlarocque@chromium.org,zea@chromium.org

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

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

chrome/browser/invalidation/gcm_invalidation_bridge_unittest.cc

@@ -47,8 +47,7 @@ class CustomFakeGCMDriver : public gcm::FakeGCMDriver {
 class GCMInvalidationBridgeTest : public ::testing::Test {
  protected:
   GCMInvalidationBridgeTest()
-      : connection_state_(
-            syncer::GCMNetworkChannelDelegate::CONNECTION_STATE_OFFLINE) {}
+      : connection_online_(false) {}
 
   virtual ~GCMInvalidationBridgeTest() {}
 
@@ -92,9 +91,8 @@ class GCMInvalidationBridgeTest : public ::testing::Test {
     request_token_errors_.push_back(error);
   }
 
-  void ConnectionStateChanged(
-      syncer::GCMNetworkChannelDelegate::ConnectionState connection_state) {
-    connection_state_ = connection_state;
+  void ConnectionStateChanged(bool online) {
+    connection_online_ = online;
   }
 
   content::TestBrowserThreadBundle thread_bundle_;
@@ -105,7 +103,7 @@ class GCMInvalidationBridgeTest : public ::testing::Test {
   std::vector<std::string> issued_tokens_;
   std::vector<GoogleServiceAuthError> request_token_errors_;
   std::string registration_id_;
-  syncer::GCMNetworkChannelDelegate::ConnectionState connection_state_;
+  bool connection_online_;
 
   scoped_ptr<GCMInvalidationBridge> bridge_;
   scoped_ptr<syncer::GCMNetworkChannelDelegate> delegate_;
@@ -154,16 +152,13 @@ TEST_F(GCMInvalidationBridgeTest, Register) {
 }
 
 TEST_F(GCMInvalidationBridgeTest, ConnectionState) {
-  EXPECT_EQ(syncer::GCMNetworkChannelDelegate::CONNECTION_STATE_OFFLINE,
-            connection_state_);
+  EXPECT_FALSE(connection_online_);
   bridge_->OnConnected(net::IPEndPoint());
   RunLoop();
-  EXPECT_EQ(syncer::GCMNetworkChannelDelegate::CONNECTION_STATE_ONLINE,
-            connection_state_);
+  EXPECT_TRUE(connection_online_);
   bridge_->OnDisconnected();
   RunLoop();
-  EXPECT_EQ(syncer::GCMNetworkChannelDelegate::CONNECTION_STATE_OFFLINE,
-            connection_state_);
+  EXPECT_FALSE(connection_online_);
 }
 
 }  // namespace

components/invalidation/gcm_invalidation_bridge.cc

@@ -58,7 +58,7 @@ class GCMInvalidationBridge::Core : public syncer::GCMNetworkChannelDelegate,
   void OnIncomingMessage(const std::string& message,
                          const std::string& echo_token);
 
-  void OnConnectionStateChanged(ConnectionState connection_state);
+  void OnConnectionStateChanged(bool online);
 
  private:
   base::WeakPtr<GCMInvalidationBridge> bridge_;
@@ -149,10 +149,9 @@ void GCMInvalidationBridge::Core::OnIncomingMessage(
   message_callback_.Run(message, echo_token);
 }
 
-void GCMInvalidationBridge::Core::OnConnectionStateChanged(
-    ConnectionState connection_state) {
+void GCMInvalidationBridge::Core::OnConnectionStateChanged(bool online) {
   if (!connection_state_callback_.is_null()) {
-    connection_state_callback_.Run(connection_state);
+    connection_state_callback_.Run(online);
   }
 }
 
@@ -286,6 +285,12 @@ void GCMInvalidationBridge::SubscribeForIncomingMessages() {
 
   DCHECK(!subscribed_for_incoming_messages_);
   gcm_driver_->AddAppHandler(kInvalidationsAppId, this);
+  core_thread_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&GCMInvalidationBridge::Core::OnConnectionStateChanged,
+                 core_,
+                 gcm_driver_->IsConnected()));
+
   subscribed_for_incoming_messages_ = true;
 }
 
@@ -330,9 +335,8 @@ void GCMInvalidationBridge::OnSendError(
 void GCMInvalidationBridge::OnConnected(const net::IPEndPoint& ip_endpoint) {
   core_thread_task_runner_->PostTask(
       FROM_HERE,
-      base::Bind(&GCMInvalidationBridge::Core::OnConnectionStateChanged,
-                 core_,
-                 syncer::GCMNetworkChannelDelegate::CONNECTION_STATE_ONLINE));
+      base::Bind(
+          &GCMInvalidationBridge::Core::OnConnectionStateChanged, core_, true));
 }
 
 void GCMInvalidationBridge::OnDisconnected() {
@@ -340,7 +344,7 @@ void GCMInvalidationBridge::OnDisconnected() {
       FROM_HERE,
       base::Bind(&GCMInvalidationBridge::Core::OnConnectionStateChanged,
                  core_,
-                 syncer::GCMNetworkChannelDelegate::CONNECTION_STATE_OFFLINE));
+                 false));
 }
 
 

components/invalidation/gcm_network_channel.cc

@@ -107,6 +107,8 @@ GCMNetworkChannel::GCMNetworkChannel(
     : request_context_getter_(request_context_getter),
       delegate_(delegate.Pass()),
       register_backoff_entry_(new net::BackoffEntry(&kRegisterBackoffPolicy)),
+      gcm_channel_online_(false),
+      http_channel_online_(false),
       diagnostic_info_(this),
       weak_factory_(this) {
   net::NetworkChangeNotifier::AddNetworkChangeObserver(this);
@@ -195,10 +197,8 @@ void GCMNetworkChannel::OnGetTokenComplete(
     // sending message and at that time we'll retry requesting access token.
     DVLOG(1) << "RequestAccessToken failed: " << error.ToString();
     RecordOutgoingMessageStatus(ACCESS_TOKEN_FAILURE);
-    // Message won't get sent because of connection failure. Let's retry once
-    // connection is restored.
-    if (error.state() == GoogleServiceAuthError::CONNECTION_FAILED)
-      NotifyStateChange(TRANSIENT_INVALIDATION_ERROR);
+    // Message won't get sent. Notify that http channel doesn't work.
+    UpdateHttpChannelState(false);
     cached_message_.clear();
     return;
   }
@@ -243,12 +243,14 @@ void GCMNetworkChannel::OnURLFetchComplete(const net::URLFetcher* source) {
        fetcher->GetResponseCode() != net::HTTP_NO_CONTENT)) {
     DVLOG(1) << "URLFetcher failure";
     RecordOutgoingMessageStatus(POST_FAILURE);
-    NotifyStateChange(TRANSIENT_INVALIDATION_ERROR);
+    // POST failed. Notify that http channel doesn't work.
+    UpdateHttpChannelState(false);
     return;
   }
 
   RecordOutgoingMessageStatus(OUTGOING_MESSAGE_SUCCESS);
-  NotifyStateChange(INVALIDATIONS_ENABLED);
+  // Successfully sent message. Http channel works.
+  UpdateHttpChannelState(true);
   DVLOG(2) << "URLFetcher success";
 }
 
@@ -277,6 +279,7 @@ void GCMNetworkChannel::OnIncomingMessage(const std::string& message,
   }
   DVLOG(2) << "Deliver incoming message";
   RecordIncomingMessageStatus(INCOMING_MESSAGE_SUCCESS);
+  UpdateGcmChannelState(true);
   DeliverIncomingMessage(android_message.message());
 #else
   // This code shouldn't be invoked on Android.
@@ -284,30 +287,36 @@ void GCMNetworkChannel::OnIncomingMessage(const std::string& message,
 #endif
 }
 
-void GCMNetworkChannel::OnConnectionStateChanged(
-    GCMNetworkChannelDelegate::ConnectionState connection_state) {
-  switch (connection_state) {
-    case GCMNetworkChannelDelegate::CONNECTION_STATE_OFFLINE: {
-      NotifyStateChange(TRANSIENT_INVALIDATION_ERROR);
-      break;
-    }
-    case GCMNetworkChannelDelegate::CONNECTION_STATE_ONLINE: {
-      NotifyStateChange(INVALIDATIONS_ENABLED);
-      break;
-    }
-    default: {
-      NOTREACHED();
-      break;
-    }
-  }
+void GCMNetworkChannel::OnConnectionStateChanged(bool online) {
+  UpdateGcmChannelState(online);
 }
 
 void GCMNetworkChannel::OnNetworkChanged(
     net::NetworkChangeNotifier::ConnectionType connection_type) {
   // Network connection is restored. Let's notify cacheinvalidations so it has
   // chance to retry.
-  if (connection_type != net::NetworkChangeNotifier::CONNECTION_NONE)
-    NotifyStateChange(INVALIDATIONS_ENABLED);
+  NotifyNetworkStatusChange(
+      connection_type != net::NetworkChangeNotifier::CONNECTION_NONE);
+}
+
+void GCMNetworkChannel::UpdateGcmChannelState(bool online) {
+  if (gcm_channel_online_ == online)
+    return;
+  gcm_channel_online_ = online;
+  InvalidatorState channel_state = TRANSIENT_INVALIDATION_ERROR;
+  if (gcm_channel_online_ && http_channel_online_)
+    channel_state = INVALIDATIONS_ENABLED;
+  NotifyChannelStateChange(channel_state);
+}
+
+void GCMNetworkChannel::UpdateHttpChannelState(bool online) {
+  if (http_channel_online_ == online)
+    return;
+  http_channel_online_ = online;
+  InvalidatorState channel_state = TRANSIENT_INVALIDATION_ERROR;
+  if (gcm_channel_online_ && http_channel_online_)
+    channel_state = INVALIDATIONS_ENABLED;
+  NotifyChannelStateChange(channel_state);
 }
 
 GURL GCMNetworkChannel::BuildUrl(const std::string& registration_id) {

components/invalidation/gcm_network_channel.h

@@ -93,9 +93,9 @@ class INVALIDATION_EXPORT_PRIVATE GCMNetworkChannel
                           const std::string& token);
   void OnIncomingMessage(const std::string& message,
                          const std::string& echo_token);
-  void OnConnectionStateChanged(
-      GCMNetworkChannelDelegate::ConnectionState connection_state);
-
+  void OnConnectionStateChanged(bool online);
+  void UpdateGcmChannelState(bool online);
+  void UpdateHttpChannelState(bool online);
   // Base64 encoding/decoding with URL safe alphabet.
   // http://tools.ietf.org/html/rfc4648#page-7
   static void Base64EncodeURLSafe(const std::string& input,
@@ -125,6 +125,11 @@ class INVALIDATION_EXPORT_PRIVATE GCMNetworkChannel
   // GCM and shuld include it in headers with outgoing message over http.
   std::string echo_token_;
 
+  // State of gcm and http channels. GCMNetworkChannel will only report
+  // INVALIDATIONS_ENABLED if both channels are online.
+  bool gcm_channel_online_;
+  bool http_channel_online_;
+
   GCMNetworkChannelDiagnostic diagnostic_info_;
 
   base::WeakPtrFactory<GCMNetworkChannel> weak_factory_;

components/invalidation/gcm_network_channel_delegate.h

@@ -24,19 +24,13 @@ namespace syncer {
 // thread and callbacks should be invoked there as well.
 class GCMNetworkChannelDelegate {
  public:
-  enum ConnectionState {
-    CONNECTION_STATE_OFFLINE,
-    CONNECTION_STATE_ONLINE
-  };
-
   typedef base::Callback<void(const GoogleServiceAuthError& error,
                               const std::string& token)> RequestTokenCallback;
   typedef base::Callback<void(const std::string& registration_id,
                               gcm::GCMClient::Result result)> RegisterCallback;
   typedef base::Callback<void(const std::string& message,
                               const std::string& echo_token)> MessageCallback;
-  typedef base::Callback<void(ConnectionState connection_state)>
-      ConnectionStateCallback;
+  typedef base::Callback<void(bool online)> ConnectionStateCallback;
 
   virtual ~GCMNetworkChannelDelegate() {}
 

components/invalidation/gcm_network_channel_unittest.cc

@@ -250,6 +250,8 @@ TEST_F(GCMNetworkChannelTest, HappyCase) {
                                          net::HTTP_NO_CONTENT,
                                          net::URLRequestStatus::SUCCESS);
 
+  // Emulate gcm connection state to be online.
+  delegate()->connection_state_callback.Run(true);
   // After construction GCMNetworkChannel should have called Register.
   EXPECT_FALSE(delegate()->register_callback.is_null());
   // Return valid registration id.
@@ -410,7 +412,7 @@ TEST_F(GCMNetworkChannelTest, Base64EncodeDecode) {
   EXPECT_EQ(input, plain);
 }
 
-TEST_F(GCMNetworkChannelTest, HttpTransientError) {
+TEST_F(GCMNetworkChannelTest, ChannelState) {
   EXPECT_FALSE(delegate()->message_callback.is_null());
   // POST will fail.
   url_fetcher_factory()->SetFakeResponse(GURL("http://test.url.com"),
@@ -418,6 +420,7 @@ TEST_F(GCMNetworkChannelTest, HttpTransientError) {
                                          net::HTTP_SERVICE_UNAVAILABLE,
                                          net::URLRequestStatus::SUCCESS);
 
+  delegate()->connection_state_callback.Run(true);
   delegate()->register_callback.Run("registration.id", gcm::GCMClient::SUCCESS);
 
   network_channel()->SendMessage("abra.cadabra");
@@ -428,25 +431,29 @@ TEST_F(GCMNetworkChannelTest, HttpTransientError) {
   EXPECT_EQ(url_fetchers_created_count(), 1);
   // Failing HTTP POST should cause TRANSIENT_INVALIDATION_ERROR.
   EXPECT_EQ(TRANSIENT_INVALIDATION_ERROR, get_last_invalidator_state());
-  // Network change to CONNECTION_NONE shouldn't affect invalidator state.
-  network_channel()->OnNetworkChanged(
-      net::NetworkChangeNotifier::CONNECTION_NONE);
-  EXPECT_EQ(TRANSIENT_INVALIDATION_ERROR, get_last_invalidator_state());
-  // Network change to something else should trigger retry.
-  network_channel()->OnNetworkChanged(
-      net::NetworkChangeNotifier::CONNECTION_WIFI);
+
+  // Setup POST to succeed.
+  url_fetcher_factory()->SetFakeResponse(GURL("http://test.url.com"),
+                                         "",
+                                         net::HTTP_NO_CONTENT,
+                                         net::URLRequestStatus::SUCCESS);
+  network_channel()->SendMessage("abra.cadabra");
+  EXPECT_FALSE(delegate()->request_token_callback.is_null());
+  delegate()->request_token_callback.Run(
+      GoogleServiceAuthError::AuthErrorNone(), "access.token");
+  RunLoopUntilIdle();
+  EXPECT_EQ(url_fetchers_created_count(), 2);
+  // Successful post should set invalidator state to enabled.
   EXPECT_EQ(INVALIDATIONS_ENABLED, get_last_invalidator_state());
+  // Network changed event shouldn't affect invalidator state.
   network_channel()->OnNetworkChanged(
       net::NetworkChangeNotifier::CONNECTION_NONE);
   EXPECT_EQ(INVALIDATIONS_ENABLED, get_last_invalidator_state());
-}
 
-TEST_F(GCMNetworkChannelTest, GcmConnectionState) {
-  delegate()->connection_state_callback.Run(
-      GCMNetworkChannelDelegate::CONNECTION_STATE_OFFLINE);
+  // GCM connection state should affect invalidator state.
+  delegate()->connection_state_callback.Run(false);
   EXPECT_EQ(TRANSIENT_INVALIDATION_ERROR, get_last_invalidator_state());
-  delegate()->connection_state_callback.Run(
-      GCMNetworkChannelDelegate::CONNECTION_STATE_ONLINE);
+  delegate()->connection_state_callback.Run(true);
   EXPECT_EQ(INVALIDATIONS_ENABLED, get_last_invalidator_state());
 }
 

components/invalidation/push_client_channel.cc

@@ -70,12 +70,14 @@ void PushClientChannel::SendMessage(const std::string& message) {
 }
 
 void PushClientChannel::OnNotificationsEnabled() {
-  NotifyStateChange(INVALIDATIONS_ENABLED);
+  NotifyNetworkStatusChange(true);
+  NotifyChannelStateChange(INVALIDATIONS_ENABLED);
 }
 
 void PushClientChannel::OnNotificationsDisabled(
     notifier::NotificationsDisabledReason reason) {
-  NotifyStateChange(FromNotifierReason(reason));
+  NotifyNetworkStatusChange(false);
+  NotifyChannelStateChange(FromNotifierReason(reason));
 }
 
 void PushClientChannel::OnIncomingNotification(

components/invalidation/sync_system_resources.cc

@@ -132,7 +132,7 @@ void SyncInvalidationScheduler::RunPostedTask(invalidation::Closure* task) {
 }
 
 SyncNetworkChannel::SyncNetworkChannel()
-    : invalidator_state_(DEFAULT_INVALIDATION_ERROR),
+    : last_network_status_(false),
       received_messages_count_(0) {}
 
 SyncNetworkChannel::~SyncNetworkChannel() {
@@ -146,7 +146,7 @@ void SyncNetworkChannel::SetMessageReceiver(
 
 void SyncNetworkChannel::AddNetworkStatusReceiver(
     invalidation::NetworkStatusCallback* network_status_receiver) {
-  network_status_receiver->Run(invalidator_state_ == INVALIDATIONS_ENABLED);
+  network_status_receiver->Run(last_network_status_);
   network_status_receivers_.push_back(network_status_receiver);
 }
 
@@ -178,18 +178,21 @@ scoped_ptr<SyncNetworkChannel> SyncNetworkChannel::CreateGCMNetworkChannel(
       request_context_getter, delegate.Pass()));
 }
 
-void SyncNetworkChannel::NotifyStateChange(InvalidatorState invalidator_state) {
-  // Remember state for future NetworkStatusReceivers.
-  invalidator_state_ = invalidator_state;
+void SyncNetworkChannel::NotifyNetworkStatusChange(bool online) {
+  // Remember network state for future NetworkStatusReceivers.
+  last_network_status_ = online;
   // Notify NetworkStatusReceivers in cacheinvalidation.
   for (NetworkStatusReceiverList::const_iterator it =
            network_status_receivers_.begin();
        it != network_status_receivers_.end(); ++it) {
-    (*it)->Run(invalidator_state_ == INVALIDATIONS_ENABLED);
+    (*it)->Run(online);
   }
-  // Notify observers.
+}
+
+void SyncNetworkChannel::NotifyChannelStateChange(
+    InvalidatorState invalidator_state) {
   FOR_EACH_OBSERVER(Observer, observers_,
-                    OnNetworkChannelStateChanged(invalidator_state_));
+                    OnNetworkChannelStateChanged(invalidator_state));
 }
 
 bool SyncNetworkChannel::DeliverIncomingMessage(const std::string& message) {

components/invalidation/sync_system_resources.h

@@ -143,8 +143,17 @@ class INVALIDATION_EXPORT_PRIVATE SyncNetworkChannel
   int GetReceivedMessagesCount() const;
 
  protected:
-  // Subclass should notify about connection state through NotifyStateChange.
-  void NotifyStateChange(InvalidatorState invalidator_state);
+  // Subclass should call NotifyNetworkStatusChange to notify about network
+  // changes. This triggers cacheinvalidation to try resending failed message
+  // ahead of schedule when client comes online or IP address changes.
+  void NotifyNetworkStatusChange(bool online);
+
+  // Subclass should notify about connection state through
+  // NotifyChannelStateChange. If communication doesn't work and it is possible
+  // that invalidations from server will not reach this client then channel
+  // should call this function with TRANSIENT_INVALIDATION_ERROR.
+  void NotifyChannelStateChange(InvalidatorState invalidator_state);
+
   // Subclass should call DeliverIncomingMessage for message to reach
   // invalidations library.
   bool DeliverIncomingMessage(const std::string& message);
@@ -157,8 +166,8 @@ class INVALIDATION_EXPORT_PRIVATE SyncNetworkChannel
   scoped_ptr<invalidation::MessageCallback> incoming_receiver_;
   NetworkStatusReceiverList network_status_receivers_;
 
-  // Last channel state for new network status receivers.
-  InvalidatorState invalidator_state_;
+  // Last network status for new network status receivers.
+  bool last_network_status_;
 
   int received_messages_count_;
 

components/invalidation/sync_system_resources_unittest.cc

@@ -181,7 +181,8 @@ class TestSyncNetworkChannel : public SyncNetworkChannel {
   TestSyncNetworkChannel() {}
   virtual ~TestSyncNetworkChannel() {}
 
-  using SyncNetworkChannel::NotifyStateChange;
+  using SyncNetworkChannel::NotifyNetworkStatusChange;
+  using SyncNetworkChannel::NotifyChannelStateChange;
   using SyncNetworkChannel::DeliverIncomingMessage;
 
   virtual void SendMessage(const std::string& message) OVERRIDE {
@@ -233,15 +234,21 @@ class SyncNetworkChannelTest
   bool connected_;
 };
 
-// Simulate network channel state change. It should propagate to observer.
-TEST_F(SyncNetworkChannelTest, OnNetworkChannelStateChanged) {
+// Simulate channel state change. It should propagate to observer.
+TEST_F(SyncNetworkChannelTest, ChannelStateChange) {
   EXPECT_EQ(DEFAULT_INVALIDATION_ERROR, last_invalidator_state_);
-  EXPECT_FALSE(connected_);
-  network_channel_.NotifyStateChange(INVALIDATIONS_ENABLED);
+  network_channel_.NotifyChannelStateChange(INVALIDATIONS_ENABLED);
   EXPECT_EQ(INVALIDATIONS_ENABLED, last_invalidator_state_);
-  EXPECT_TRUE(connected_);
-  network_channel_.NotifyStateChange(INVALIDATION_CREDENTIALS_REJECTED);
+  network_channel_.NotifyChannelStateChange(INVALIDATION_CREDENTIALS_REJECTED);
   EXPECT_EQ(INVALIDATION_CREDENTIALS_REJECTED, last_invalidator_state_);
+}
+
+// Simulate network state change. It should propagate to cacheinvalidations.
+TEST_F(SyncNetworkChannelTest, NetworkStateChange) {
+  EXPECT_FALSE(connected_);
+  network_channel_.NotifyNetworkStatusChange(true);
+  EXPECT_TRUE(connected_);
+  network_channel_.NotifyNetworkStatusChange(false);
   EXPECT_FALSE(connected_);
 }