chrome[android]: Restrict the use of startService for invalidations.

components/invalidation/impl/android/java/src/org/chromium/components/invalidation/InvalidationService.java

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 package org.chromium.components.invalidation;
 
 import android.accounts.Account;
 import android.content.Context;
 import android.content.Intent;
 
 import com.google.protos.ipc.invalidation.Types;
 
+import org.chromium.base.ApplicationStatus;
+import org.chromium.base.BuildInfo;
+import org.chromium.base.Log;
 import org.chromium.base.ThreadUtils;
 import org.chromium.base.VisibleForTesting;
 import org.chromium.base.annotations.CalledByNative;
 import org.chromium.base.annotations.JNINamespace;
 import org.chromium.components.sync.notifier.InvalidationClientNameProvider;
 import org.chromium.components.sync.notifier.InvalidationIntentProtocol;
 import org.chromium.components.sync.notifier.InvalidationPreferences;
 
 /**
  * Wrapper for invalidations::InvalidationServiceAndroid.
  *
  * Serves as the bridge between Java and C++ for the invalidations component.
  */
 @JNINamespace("invalidation")
 public class InvalidationService {
     private final Context mContext;
 
     private final long mNativeInvalidationServiceAndroid;
 
+    private static final String TAG = "cr_invalidation";
+
     private InvalidationService(Context context, long nativeInvalidationServiceAndroid) {
         mContext = context.getApplicationContext();
         if (mContext == null) {
             throw new NullPointerException("mContext is null.");
         }
         mNativeInvalidationServiceAndroid = nativeInvalidationServiceAndroid;
     }
 
     public void notifyInvalidationToNativeChrome(
             int objectSource, String objectId, long version, String payload) {
@@ skipping 22 matching lines @@
      */
     @VisibleForTesting
     @CalledByNative
     public void setRegisteredObjectIds(int[] objectSources, String[] objectNames) {
         InvalidationPreferences invalidationPreferences = new InvalidationPreferences();
         Account account = invalidationPreferences.getSavedSyncedAccount();
         Intent registerIntent = InvalidationIntentProtocol.createRegisterIntent(
                 account, objectSources, objectNames);
         registerIntent.setClass(
                 mContext, InvalidationClientService.getRegisteredClass());
+
+        if (shouldRestrictBackgroundServices()) {

[Khushal, 2017/02/16 05:34:23]

Sigh. I considered this one in particular, since we persist the current
registrations in InvalidationPreferences and then use the diff to figure out
which objects to register and unregister. But eventually the
InvalidationClientService is going to call into the AndroidListener in cache
invalidation which can also fail, leaving us in an inconsistent state.

Though when I dug around a bit more, looks like
ensureStartedAndUpdateRegisteredTypes in InvalidationController will do this
update also. Would it be expensive to do that each time the application state
changes to foreground for this restrict case?

[ghc, 2017/02/17 19:27:38]

I think that would involve sending all registrations every time the application
goes to the foreground. That would be significantly more load on the servers and
probably not feasible. A better approach might be to keep track of whether the
latest registration state has been communicated to the ticl, and if not, try to
do so upon switching to foreground. Would something like that be hard to
implement?

[Khushal, 2017/02/17 21:12:01]

That's what the current code tries to do I think. Here we store the
registrations communicated to Ticl in SharedPrefs and only give the diff when
something changes:
https://cs.chromium.org/chromium/src/components/invalidation/impl/android/jav...

But given the fact that this request can fail later down the stack in the
service hops before going to Ticl, I don't know whether this would work. Does
TiclState try to do some internal tracking for only communicating new
registrations to the server?

[ghc, 2017/02/17 21:31:36]

There is logic to avoid sending a message if the set of registered objects
hasn't changed (see bottom of
InvalidationClientCore::performRegisterOperations). I assume this should work in
such a scenario, but I'm not 100% certain.

(The Ticl also delivers a registration status callback, and for a new
registration you'll always receive an initial invalidation. If you receive an
invalidation for an object, you can assume that a registration succeeded; that
could be another signal to avoid unnecessary re-registration.)

[Khushal, 2017/02/17 21:54:52]

What about this code:
https://cs.chromium.org/chromium/src/third_party/cacheinvalidation/src/java/c...

AndroidListener has persisted state for what has been registered, and is updated
and written to disk after each intent. So if we simply re-issue the registration
request, this code should take care of only sending a message to the server only
if there are new registrations, right? In fact that makes me wonder why we store
this in InvalidationPrefs at all in chrome. Am I missing something?

[nyquist, 2017/02/21 19:46:18]

So this is a little bit scary. If this ends up failing at some point, we will
have persisted incorrect state, and possibly will not ever fix it.

Could we instead of the internals of ticl to not ping the server too often?

[Khushal, 2017/02/21 20:52:25]

About the discussion on persisted state, on taking a deeper look at where all
state is stored I think we can be left with an inconsistent state in the
AndroidListener as well. There is InvalidationPreferences being stored here and
we can fail to communicate that to the AndroidListener. AndroidListener is
further storing it internally as desiredRegistrations and sending them to
TiclService which can also fail. And finally Ticl is storing it in the
InvalidationClientCore which I think has the final state used for deciding
whether server sync is necessary and what to send (correct me if I understood
something incorrectly).

So while we could make an effort to not store anything in InvalidationPrefs and
avoid inconsistency with AndroidListener's state, I don't have a good idea for
AndroidListener and TiclState inconsistency.

[nyquist, 2017/02/22 10:48:13]

Well then, unless ghc@ has a better idea, I guess we should just submit this.

[ghc, 2017/02/22 22:03:23]

I don't think I have any better ideas. The ticl is a subtle piece of code with
some tricky state machine logic. The risk of a code change breaking one of its
invariants seems pretty high.

+            Log.e(TAG, "Failed to register objects");
+            return;
+        }
         mContext.startService(registerIntent);
     }
 
+    private boolean shouldRestrictBackgroundServices() {
+        // Restricts the use of background services when not in foreground. See crbug.com/680812.
+        return BuildInfo.isGreaterThanN() && !ApplicationStatus.hasVisibleActivities();
+    }
+
     /**
      * Fetches the Invalidator client name.
      *
      * Note that there is a naming discrepancy here.  In C++, we refer to the invalidation client
      * identifier that is unique for every invalidation client instance in an account as the client
      * ID.  In Java, we call it the client name.
      */
     @CalledByNative
     private byte[] getInvalidatorClientId() {
         return InvalidationClientNameProvider.get().getInvalidatorClientName();
     }
 
     private native void nativeInvalidate(long nativeInvalidationServiceAndroid, int objectSource,
             String objectId, long version, String payload);
 }