[Tango->FCM] Make lazyness work for the new invalidations

Currently messages are not persisted for the FCM invalidations, because
the sender id for which invaliations are marked lazy mismatches the id
received in the incoming messages. In addition, messages are replayed
before app handler is knows, so they aren't passed to it.

This patch implements the fix.

Bug: 882887, 944484
Change-Id: I6f53d65488f196f76dedc0b8ae23f9889d5c13a5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1533875Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Peter Beverloo <peter@chromium.org>
Commit-Queue: Tatiana Gornak <melandory@chromium.org>
Cr-Commit-Position: refs/heads/master@{#644773}

components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/GCMDriver.java

@@ -53,22 +53,38 @@ public class GCMDriver {
             throw new IllegalStateException("Already instantiated");
         }
         sInstance = new GCMDriver(nativeGCMDriverAndroid);
-        // Don't bother to read the stored messages unless there are actually
-        // messages persisted on disk. Calling
-        // LazySubscriptionsManager.hasPersistedMessages() should be a cheap way
-        // to avoid unnecessary disk reads.
+        return sInstance;
+    }
+
+    /**
+     * Called when our C++ counterpart is deleted. Clear the handle to our
+     * native C++ object, ensuring it's never called.
+     */
+    @CalledByNative
+    private void destroy() {
+        assert sInstance == this;
+        sInstance = null;
+        mNativeGCMDriverAndroid = 0;
+    }
+
+    @CalledByNative
+    private void replayPersistedMessages(final String appId) {
         if (LazySubscriptionsManager.hasPersistedMessages()) {
             long time = SystemClock.elapsedRealtime();
             Set<String> lazySubscriptionIds = LazySubscriptionsManager.getLazySubscriptionIds();
+            boolean hasRemainingMessages = false;
             for (String id : lazySubscriptionIds) {
+                if (!id.startsWith(appId)) {
+                    hasRemainingMessages = (LazySubscriptionsManager.readMessages(id).length != 0);
+                    continue;
+                }
                 GCMMessage[] messages = LazySubscriptionsManager.readMessages(id);
                 for (GCMMessage message : messages) {
                     dispatchMessage(message);
                 }
                 LazySubscriptionsManager.deletePersistedMessagesForSubscriptionId(id);
             }
-            LazySubscriptionsManager.storeHasPersistedMessages(/*hasPersistedMessages=*/false);
-
+            LazySubscriptionsManager.storeHasPersistedMessages(hasRemainingMessages);
             long duration = SystemClock.elapsedRealtime() - time;
             // Call RecordHistogram.recordTimesHistogram() on a background thread to avoid expensive
             // JNI calls in the critical path.
@@ -77,18 +93,6 @@ public class GCMDriver {
                         "PushMessaging.TimeToReadPersistedMessages", duration);
             });
         }
-        return sInstance;
-    }
-
-    /**
-     * Called when our C++ counterpart is deleted. Clear the handle to our
-     * native C++ object, ensuring it's never called.
-     */
-    @CalledByNative
-    private void destroy() {
-        assert sInstance == this;
-        sInstance = null;
-        mNativeGCMDriverAndroid = 0;
     }
 
     @CalledByNative
@@ -110,8 +114,7 @@ public class GCMDriver {
                 nativeOnRegisterFinished(mNativeGCMDriverAndroid, appId, registrationId,
                                          !registrationId.isEmpty());
             }
-        }
-                .executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
+        }.executeOnExecutor(AsyncTask.THREAD_POOL_EXECUTOR);
     }
 
     @CalledByNative

components/gcm_driver/android/java/src/org/chromium/components/gcm_driver/LazySubscriptionsManager.java

@@ -33,6 +33,8 @@ public class LazySubscriptionsManager {
     private static final String HAS_PERSISTED_MESSAGES_KEY = "has_persisted_messages";
     private static final String PREF_PACKAGE =
             "org.chromium.components.gcm_driver.lazy_subscriptions";
+    private static final String INVALIDATION_APP_ID = "com.google.chrome.fcm.invalidations";
+    private static final String INVALIDATION_SENDER_ID = "8181035976";
 
     // The max number of most recent messages queued per lazy subscription until
     // Chrome is foregrounded.
@@ -78,7 +80,11 @@ public class LazySubscriptionsManager {
      * @return The unique identifier for the subscription.
      */
     public static String buildSubscriptionUniqueId(final String appId, final String senderId) {
-        return appId + senderId;
+        if (appId.equals(INVALIDATION_APP_ID)) {
+            return appId + INVALIDATION_SENDER_ID;
+        } else {
+            return appId + senderId;
+        }
     }
 
     /**

components/gcm_driver/gcm_driver_android.cc

@@ -123,6 +123,16 @@ void GCMDriverAndroid::OnSignedIn() {
 void GCMDriverAndroid::OnSignedOut() {
 }
 
+void GCMDriverAndroid::AddAppHandler(const std::string& app_id,
+                                     GCMAppHandler* handler) {
+  GCMDriver::AddAppHandler(app_id, handler);
+  JNIEnv* env = AttachCurrentThread();
+  // TODO(melandory, mamir): check if messages were persisted
+  // and only then go to java.
+  Java_GCMDriver_replayPersistedMessages(env, java_ref_,
+                                         ConvertUTF8ToJavaString(env, app_id));
+}
+
 void GCMDriverAndroid::AddConnectionObserver(GCMConnectionObserver* observer) {
 }
 

components/gcm_driver/gcm_driver_android.h

@@ -80,6 +80,8 @@ class GCMDriverAndroid : public GCMDriver,
   InstanceIDHandler* GetInstanceIDHandlerInternal() override;
   void AddHeartbeatInterval(const std::string& scope, int interval_ms) override;
   void RemoveHeartbeatInterval(const std::string& scope) override;
+  void AddAppHandler(const std::string& app_id,
+                     GCMAppHandler* handler) override;
 
   // GCMStatsRecorder::Delegate implementation:
   void OnActivityRecorded() override;

components/invalidation/impl/fcm_invalidation_listener.cc

@@ -50,6 +50,8 @@ void FCMInvalidationListener::Start(
       base::BindRepeating(&FCMInvalidationListener::InformTokenReceived,
                           weak_factory_.GetWeakPtr()));
   subscription_channel_state_ = SubscriptionChannelState::ENABLED;
+
+  network_channel_->StartListening();
   EmitStateChange();
   DoRegistrationUpdate();
 }
@@ -192,6 +194,7 @@ void FCMInvalidationListener::Stop() {
     per_user_topic_registration_manager_->RemoveObserver(this);
   }
   per_user_topic_registration_manager_.reset();
+  network_channel_->StopListening();
 
   subscription_channel_state_ = SubscriptionChannelState::NOT_STARTED;
   fcm_network_state_ = FcmChannelState::NOT_STARTED;

components/invalidation/impl/fcm_invalidation_listener_unittest.cc

@@ -38,6 +38,12 @@ const char kPayload2[] = "payload2";
 const int64_t kVersion1 = 1LL;
 const int64_t kVersion2 = 2LL;
 
+class TestFCMSyncNetworkChannel : public FCMSyncNetworkChannel {
+ public:
+  void StartListening() override {}
+  void StopListening() override {}
+};
+
 // Fake delegate that keeps track of invalidation counts, payloads,
 // and state.
 class FakeDelegate : public FCMInvalidationListener::Delegate {
@@ -173,7 +179,7 @@ class FCMInvalidationListenerTest : public testing::Test {
         kPreferencesTopic_("PREFERENCE"),
         kExtensionsTopic_("EXTENSION"),
         kAppsTopic_("APP"),
-        fcm_sync_network_channel_(new FCMSyncNetworkChannel()),
+        fcm_sync_network_channel_(new TestFCMSyncNetworkChannel()),
         listener_(base::WrapUnique(fcm_sync_network_channel_)),
         fake_delegate_(&listener_) {}
 

components/invalidation/impl/fcm_invalidation_service.cc

@@ -239,12 +239,13 @@ void FCMInvalidationService::StartInvalidator() {
   // We should start listening before requesting the id, because
   // valid id is only generated, once there is an app handler
   // for the app. StartListening registers the app handler.
-  network->StartListening();
-  PopulateClientID();
-
+  // We should create invalidator first, because it registers the handler
+  // for the incoming messages, which is crutial on Android, because on the
+  // startup cached messages might exists.
   invalidator_ = std::make_unique<syncer::FCMInvalidator>(
       std::move(network), identity_provider_, pref_service_, loader_factory_,
       parse_json_, kInvalidationGCMSenderId);
+  PopulateClientID();
   invalidator_->RegisterHandler(this);
   DoUpdateRegisteredIdsIfNeeded();
 }

components/invalidation/impl/fcm_invalidator_unittest.cc

@@ -28,6 +28,12 @@ namespace syncer {
 
 namespace {
 
+class TestFCMSyncNetworkChannel : public FCMSyncNetworkChannel {
+ public:
+  void StartListening() override {}
+  void StopListening() override {}
+};
+
 class FCMInvalidatorTestDelegate {
  public:
   FCMInvalidatorTestDelegate() {
@@ -41,7 +47,7 @@ class FCMInvalidatorTestDelegate {
                          const std::string&,
                          const base::WeakPtr<InvalidationStateTracker>&) {
     DCHECK(!invalidator_);
-    auto network_channel = std::make_unique<FCMSyncNetworkChannel>();
+    auto network_channel = std::make_unique<TestFCMSyncNetworkChannel>();
     identity_provider_ =
         std::make_unique<invalidation::ProfileIdentityProvider>(
             identity_test_env_.identity_manager());

components/invalidation/impl/fcm_network_handler.cc

@@ -93,6 +93,9 @@ FCMNetworkHandler::~FCMNetworkHandler() {
 }
 
 void FCMNetworkHandler::StartListening() {
+  if (IsListening()) {
+    StopListening();
+  }
   // Adding ourselves as Handler means start listening.
   // Being the listener is pre-requirement for token operations.
   gcm_driver_->AddAppHandler(app_id_, this);

components/invalidation/impl/fcm_network_handler.h

@@ -66,11 +66,13 @@ class FCMNetworkHandler : public gcm::GCMAppHandler,
 
   ~FCMNetworkHandler() override;
 
-  void StartListening();
-  void StopListening();
   bool IsListening() const;
   void UpdateChannelState(FcmChannelState state);
 
+  // FCMSyncNetworkChannel overrides.
+  void StartListening() override;
+  void StopListening() override;
+
   // GCMAppHandler overrides.
   void ShutdownHandler() override;
   void OnStoreReset() override;

components/invalidation/impl/fcm_sync_network_channel.h

@@ -31,6 +31,9 @@ class FCMSyncNetworkChannel : public NetworkChannel {
   FCMSyncNetworkChannel();
   ~FCMSyncNetworkChannel() override;
 
+  virtual void StartListening() = 0;
+  virtual void StopListening() = 0;
+
   void SetMessageReceiver(MessageCallback incoming_receiver) override;
   void SetTokenReceiver(TokenCallback token_receiver) override;