Prevent AffiliatedInvalidationServiceProvider from disconnecting in case of...

Prevent AffiliatedInvalidationServiceProvider from disconnecting in case of TRANSIENT_INVALIDATION_ERROR

Currently AffiliatedInvalidationServiceProvider disconnects and connects
again even in the case of TRANSIENT_INVALIDATION_ERROR. If there are
lots of TRANSIENT_INVALIDATION_ERROR a lot of connection and
disconnection of AffiliatedInvalidationServiceProvider will eventually
cause throttling of registration requests. Handle the case of
TRANSIENT_INVALIDATION_ERROR in the following way:
1. If there is an enabled InvalidationService, do not cause
disconnection
2. If there is no InvalidationService available yet, do not cause
connection, wait until TRANSIENT_INVALIDATION_ERROR is gone (Existing
behaviour).
3. Upon receiving TRANSIENT_INVALIDATION_ERROR, post delayed task that
will check InvalidationService to see if it switched to
INVALIDATION_ENABLED. If InvalidationService still in
TRANSIENT_INVALIDATION_ERROR state, cause the disconnection.
4. If the number of TRANSIENT_INVALIDATION_ERROR followed by
disconnection in delayed task is above certain limit, do not disconnect.
This is the best effort behaviour, keep registered topics registered and
failed ones as failed.


Bug: 1006706
Change-Id: Ie06f987eec12bc0f50a4c59098daf39ecf8bb12e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1821529
Commit-Queue: Askar Aitzhan <askaraitzhan@google.com>
Commit-Queue: Maksim Ivanov <emaxx@chromium.org>
Auto-Submit: Askar Aitzhan <askaraitzhan@google.com>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Cr-Commit-Position: refs/heads/master@{#700814}

chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl.cc

@@ -11,6 +11,7 @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/task/post_task.h"
+#include "base/timer/timer.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_process_platform_part_chromeos.h"
 #include "chrome/browser/chrome_content_browser_client.h"
@@ -53,6 +54,28 @@ namespace policy {
 
 namespace {
 
+// Currently InvalidationService sends TRANSIENT_INVALIDATION_ERROR in case if
+// topic failing to register, but at the same time InvalidationService has retry
+// logic. In case if retry is successful and all the topic succeeded to
+// register, InvalidationService sends INVALIDATION_ENABLED. So
+// InvalidationServiceObserver should give InvalidationService a chance to
+// reregister topic, and after some time if INVALIDATION_ENABLED is not being
+// received, manually check with delayed task. If after
+// |kCheckInvalidatorStateDelay|, InvalidationService is still in
+// TRANSIENT_INVALIDATION_ERROR state, disconnect from it and try to reregister
+// all the topics.
+constexpr base::TimeDelta kCheckInvalidatorStateDelay =
+    base::TimeDelta::FromMinutes(3);
+
+// After reregistering all the topics |kTransientErrorDisconnectLimit| number of
+// times, when InvalidationService is failing due to
+// TRANSIENT_INVALIDATION_ERROR, stop disconnecting, and let registered topics
+// to keep being registered and failing topics to keep being failed. This is the
+// best effort behaviour. If |kTransientErrorDisconnectLimit| is too high, at
+// some point Firebase Cloud Message will start throttling register request for
+// this client.
+constexpr int kTransientErrorDisconnectLimit = 3;
+
 invalidation::ProfileInvalidationProvider* GetInvalidationProvider(
     Profile* profile) {
   if (base::FeatureList::IsEnabled(features::kPolicyFcmInvalidations)) {
@@ -96,6 +119,7 @@ class AffiliatedInvalidationServiceProviderImpl::InvalidationServiceObserver
   ~InvalidationServiceObserver() override;
 
   invalidation::InvalidationService* GetInvalidationService();
+  void CheckInvalidatorState();
   bool IsServiceConnected() const;
 
   // public syncer::InvalidationHandler:
@@ -109,6 +133,10 @@ class AffiliatedInvalidationServiceProviderImpl::InvalidationServiceObserver
   invalidation::InvalidationService* invalidation_service_;
   bool is_service_connected_;
   bool is_observer_ready_;
+  base::OneShotTimer transient_error_retry_timer_;
+
+  // The number of times TRANSIENT_INVALIDATION_ERROR should cause disconnect.
+  int transient_error_disconnect_limit_ = kTransientErrorDisconnectLimit;
 
   DISALLOW_COPY_AND_ASSIGN(InvalidationServiceObserver);
 };
@@ -123,7 +151,7 @@ AffiliatedInvalidationServiceProviderImpl::InvalidationServiceObserver::
       is_observer_ready_(false) {
   invalidation_service_->RegisterInvalidationHandler(this);
   is_service_connected_ = invalidation_service->GetInvalidatorState() ==
-      syncer::INVALIDATIONS_ENABLED;
+                          syncer::INVALIDATIONS_ENABLED;
   is_observer_ready_ = true;
 }
 
@@ -145,14 +173,27 @@ bool AffiliatedInvalidationServiceProviderImpl::InvalidationServiceObserver::
 }
 
 void AffiliatedInvalidationServiceProviderImpl::InvalidationServiceObserver::
-    OnInvalidatorStateChange(syncer::InvalidatorState state) {
-  if (!is_observer_ready_)
-    return;
+    CheckInvalidatorState() {
+  // Treat TRANSIENT_INVALIDATION_ERROR as an error, and disconnect the service
+  // if connected.
+  DCHECK(is_observer_ready_);
+  DCHECK(invalidation_service_);
+  DCHECK(parent_);
 
-  const bool is_service_connected = (state == syncer::INVALIDATIONS_ENABLED);
-  if (is_service_connected == is_service_connected_)
+  syncer::InvalidatorState state = invalidation_service_->GetInvalidatorState();
+  bool is_service_connected = (state == syncer::INVALIDATIONS_ENABLED);
+
+  if (is_service_connected_ == is_service_connected)
     return;
 
+  if (state == syncer::TRANSIENT_INVALIDATION_ERROR) {
+    // Do not cause disconnect if the number of disconnections caused by
+    // TRANSIENT_INVALIDATION_ERROR is more than the limit.
+    if (!transient_error_disconnect_limit_)
+      return;
+    --transient_error_disconnect_limit_;
+  }
+
   is_service_connected_ = is_service_connected;
   if (is_service_connected_)
     parent_->OnInvalidationServiceConnected(invalidation_service_);
@@ -160,6 +201,44 @@ void AffiliatedInvalidationServiceProviderImpl::InvalidationServiceObserver::
     parent_->OnInvalidationServiceDisconnected(invalidation_service_);
 }
 
+void AffiliatedInvalidationServiceProviderImpl::InvalidationServiceObserver::
+    OnInvalidatorStateChange(syncer::InvalidatorState state) {
+  if (!is_observer_ready_)
+    return;
+
+  // TODO(crbug/1007287): Handle 3 different states of
+  // AffiliatedInvalidationServiceProvider properly.
+  if (is_service_connected_) {
+    // If service is connected, do NOT notify parent in case:
+    //   * state == INVALIDATIONS_ENABLED
+    //   * state == TRANSIENT_INVALIDATION_ERROR, hopefully will be resolved by
+    //     InvalidationService, if not InvalidationService should notify again
+    //     with another more severe state.
+    bool should_notify = (state != syncer::INVALIDATIONS_ENABLED &&
+                          state != syncer::TRANSIENT_INVALIDATION_ERROR);
+
+    if (should_notify) {
+      is_service_connected_ = false;
+      parent_->OnInvalidationServiceDisconnected(invalidation_service_);
+    } else if (state == syncer::TRANSIENT_INVALIDATION_ERROR) {
+      transient_error_retry_timer_.Stop();
+      transient_error_retry_timer_.Start(
+          FROM_HERE, kCheckInvalidatorStateDelay,
+          base::BindOnce(&AffiliatedInvalidationServiceProviderImpl::
+                             InvalidationServiceObserver::CheckInvalidatorState,
+                         base::Unretained(this)));
+    }
+  } else {
+    // If service is disconnected, ONLY notify parent in case:
+    //   * state == INVALIDATIONS_ENABLED
+    bool should_notify = (state == syncer::INVALIDATIONS_ENABLED);
+    if (should_notify) {
+      is_service_connected_ = true;
+      parent_->OnInvalidationServiceConnected(invalidation_service_);
+    }
+  }
+}
+
 void AffiliatedInvalidationServiceProviderImpl::InvalidationServiceObserver::
     OnIncomingInvalidation(
         const syncer::ObjectIdInvalidationMap& invalidation_map) {

chrome/browser/chromeos/policy/affiliated_invalidation_service_provider_impl_unittest.cc

@@ -493,8 +493,16 @@ TEST_P(AffiliatedInvalidationServiceProviderImplTest,
   SendInvalidatorStateChangeNotification(
       is_fcm_enabled(), device_invalidation_service_,
       syncer::INVALIDATION_CREDENTIALS_REJECTED);
-  EXPECT_EQ(1, consumer_->GetAndClearInvalidationServiceSetCount());
-  EXPECT_EQ(nullptr, consumer_->GetInvalidationService());
+  if (is_fcm_enabled()) {
+    EXPECT_EQ(1, consumer_->GetAndClearInvalidationServiceSetCount());
+    EXPECT_EQ(nullptr, consumer_->GetInvalidationService());
+  } else {
+    // TiclInvalidationService replaces INVALIDATION_CREDENTIALS_REJECTED with
+    // TRANSIENT_INVALIDATION_ERROR. Which doesn't cause disconnection.
+    EXPECT_EQ(0, consumer_->GetAndClearInvalidationServiceSetCount());
+    EXPECT_EQ(provider_->GetDeviceInvalidationServiceForTest(),
+              consumer_->GetInvalidationService());
+  }
 
   // Verify that the device-global invalidation service still exists.
   EXPECT_TRUE(provider_->GetDeviceInvalidationServiceForTest());