Closed Bug 1349299 Opened 7 years ago Closed 7 years ago

Device not registering to FxA if no Google Account

Categories

(Firefox for Android Graveyard :: Android Sync, enhancement, P1)

Product:
Firefox for Android Graveyard
Component:
Android Sync
Type:
enhancement

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: eoger, Assigned: eoger)

References

Details

Attachments

(1 file)

Bug 1349299 - Register device on FxA even on Push registration failure.
59 bytes, text/x-review-board-request
Grisha
: review+
Details 
If we don't have a Google Account, the push registration will fail (which is normal since we use GCM as a backend).
Problem is, we could still register to FxA, even without Push Callback URLS, but we don't, because we just log the error and move on with our lives [0].
What this means is that when we try to change the name of the device in the settings screen, it is not reflected on the FxA device manager.

[0]: http://searchfox.org/mozilla-central/source/mobile/android/components/FxAccountsPush.js#78

This should be fairly simple to correct.
IIRC, GCM does not require a Google Account on devices running ICS+.
See Also: → 1351085
Assignee: nobody → eoger
Status: NEW → ASSIGNED
Priority: -- → P1
Comment on attachment 8852107 [details]
Bug 1349299 - Register device on FxA even on Push registration failure.

https://reviewboard.mozilla.org/r/124344/#review128776

This is too hacky for my taste.
You're relying on the fact that we perform sanity null checks in the java layer.
You have two states here: registered for push, or failed to register for push. I think you should make that explicit. Instead of sending down a subscription dict with a bunch of nulls, send an explicit message saying that subscription failed. The receiving java side, `FxAccountDeviceRegistrator`, should then decide what to do, and it should be explicit about what's going on.
E.g. you'll have two code paths, "all good, registering fxa device along with push subscription tuple" and "registering fxa device without a push subscription".

For bonus points, write unit tests to cover this flow.
Attachment #8852107 - Flags: review?(gkruglov) → review-
Would that work for you Grisha? I would have preferred to construct the FxAccountDevice object earlier in the call hierarchy but because of the recursive call in recoverFromDeviceSessionConflict we have to it right before the http request.
I think you might have forgot to push some changes.
Flags: needinfo?(eoger)
Woops my bad, here it is corrected.
Flags: needinfo?(eoger)
Comment on attachment 8852107 [details]
Bug 1349299 - Register device on FxA even on Push registration failure.

https://reviewboard.mozilla.org/r/124344/#review129006

That's better! I think we can improve this a bit more.

The main goal of this patch is that we perform an FxA device registration if a client failed to complete push registration. That's fine in case of "can not do push registration for some fundamental reasons (no GCM available...)". There are cases where the failure is going to be intermittent (it's likely those cases will be the more prominent ones!). Perhaps autopush service is having problems right now, or we fail to obtain a GCM token at this particular moment for some reason, or whatever else happens that might not happen if we were to try an hour later. In that case, once we registered a device, we won't attempt to update our registration for a whole month (the "registrationRenewal" bit), which is not good. We might not have push for this client for a whole month! And if they're unlikely with timing, for longer.

Does the above analysis sound correct to you? Or am I missing a reason why failed subscription is largely permanent? I haven't traced all of the error handling in some time.

But if we do need to take care of this, we already have a registration renewal flow which runs on every sync and on startup. The simplest thing would be to make use of that. Can you distinguish between types of failures? Perhaps different Push registration failure modes can dictate different renewal strategies.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDevice.java:38
(Diff revision 4)
>      this.pushCallback = pushCallback;
>      this.pushPublicKey = pushPublicKey;
>      this.pushAuthKey = pushAuthKey;
>    }
>  
> -  public static FxAccountDevice forRegister(String name, String type, String pushCallback,
> +  public FxAccountDevice(Builder builder) {

Just use the constructor you already have above, no need to couple FxAccountDevice to the Builder class in such way.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:125
(Diff revision 4)
>    }
>  
>    @Override
>    public void handleMessage(String event, GeckoBundle message, EventCallback callback) {
>      if ("FxAccountsPush:Subscribe:Response".equals(event)) {
> +      boolean success = message.getBoolean("success");

final

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:127
(Diff revision 4)
>    @Override
>    public void handleMessage(String event, GeckoBundle message, EventCallback callback) {
>      if ("FxAccountsPush:Subscribe:Response".equals(event)) {
> +      boolean success = message.getBoolean("success");
> +      if (success) {
> +        Log.i(LOG_TAG, "Push registration succeeded. Beginning FxA Registration.");

"[...] Beginning normal FxA Registration."

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:131
(Diff revision 4)
> +      if (success) {
> +        Log.i(LOG_TAG, "Push registration succeeded. Beginning FxA Registration.");
> -      doFxaRegistration(message.getBundle("subscription"));
> +        doFxaRegistration(message.getBundle("subscription"));
> -    } else {
> +      } else {
> +        Log.i(LOG_TAG, "Push registration failed. Beginning degraded FxA Registration.");
> +        doFxaRegistration(null);

Can you do some light refactoring to avoid passing around nulls? I feel that the end result will be cleaner. I think separating the "build a device" from "make a request" parts of `doFxaRegistration` should help.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:160
(Diff revision 4)
>      } catch (State.NotASessionTokenState e) {
>        Log.e(LOG_TAG, "Could not get a session token", e);
>        return;
>      }
> -    final FxAccountDevice device;
> -    String deviceId = fxAccount.getDeviceId();
> +
> +    FxAccountDevice device = buildFxAccountDevice(context, subscription, fxAccount);

final

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:161
(Diff revision 4)
>        Log.e(LOG_TAG, "Could not get a session token", e);
>        return;
>      }
> -    final FxAccountDevice device;
> -    String deviceId = fxAccount.getDeviceId();
> -    String clientName = getClientName(fxAccount, context);
> +
> +    FxAccountDevice device = buildFxAccountDevice(context, subscription, fxAccount);
> +    if(device.id == null) {

nit: spacing; I think this will fail our checkstyle.
Attachment #8852107 - Flags: review?(gkruglov) → review-
Comment on attachment 8852107 [details]
Bug 1349299 - Register device on FxA even on Push registration failure.

Not ready for review.
TODO: Separate permanent/temporary handling for push registration failures.
Attachment #8852107 - Flags: review?(gkruglov)
Created a new push error code for when GCM is disabled. We pass that code around and we don't bump the device registration version in the registrator if we get that code.
Comment on attachment 8852107 [details]
Bug 1349299 - Register device on FxA even on Push registration failure.

https://reviewboard.mozilla.org/r/124344/#review131006

I'm not sure this general approach is going to be granular enough. What you're currently doing is resetting a device registration version in case of errors (well, currently just one error), and relying on a side-effect of re-registration logic.

I think we want different registration/renewal behaviour depending on the errors we encounter along the way. An autopushclient exception might indicate a problem on the server, so perhaps retrying on next sync is fine. No gcm support on a device might indicate to never try again.

What I'm thinking we can do is store the last error we got, and when we got it. Using those two bits of information we can make a better decision if we want to attempt registration again, and when. 'maybeRegisterDevice' method might be a good starting point for this logic, and perhaps device registrator should own it, akin to 'needToRenewRegistration" (perhaps that could be consumed by the new logic). This approach gains us a very explicit flow, something that should be easy to build upon if we need to, and you should be able to write good tests to assert your re-registration conditions.

What do you think?

::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java:379
(Diff revision 6)
>              if ("PushServiceAndroidGCM:RegisterUserAgent".equals(event)) {
>                  try {
>                      pushManager.registerUserAgent(geckoProfile.getName(), System.currentTimeMillis()); // For side-effects.
>                      callback.sendSuccess(null);
>                  } catch (PushManager.ProfileNeedsConfigurationException | AutopushClientException | PushClient.LocalException | IOException e) {
>                      Log.e(LOG_TAG, "Got exception in " + event, e);

See which exceptions here could be temporary, and handle them as well.

Definetely the autopushclientexceptions one, at least - unless it's indicating that the client is doing something wrong and shouldn't retry.

::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java:413
(Diff revision 6)
>                  final PushSubscription subscription;
>                  try {
>                      subscription = pushManager.subscribeChannel(geckoProfile.getName(), service, serviceData, appServerKey, System.currentTimeMillis());
>                  } catch (PushManager.ProfileNeedsConfigurationException | AutopushClientException | PushClient.LocalException | IOException e) {
>                      Log.e(LOG_TAG, "Got exception in " + event, e);
>                      callback.sendError("Got exception handling message [" + event + "]: " + e.toString());

Handle (some of) these exceptions as well.

::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java:474
(Diff revision 6)
>              // registrations and subscriptions that can't be advanced.
> -            callback.sendError("To handle event [" + event + "], user interaction is needed to enable Google Play Services.");
> +            String msg = "To handle event [" + event + "], user interaction is needed to enable Google Play Services.";
> +            GeckoBundle reply = new GeckoBundle();
> +            GeckoBundle error = new GeckoBundle();
> +            error.putString("message", msg);
> +            error.putDouble("result", 2154627078d); // = NS_ERROR_DOM_PUSH_GCM_DISABLED

if you _do_ need to keep this outside of java, make it a constant.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:132
(Diff revision 6)
>        Log.e(LOG_TAG, "No action defined for " + event);
>      }
>    }
>  
> -  private void doFxaRegistration(GeckoBundle subscription) {
> +  private void handlePushSubscriptionResponse(final GeckoBundle message) {
> +    // Sanity checks

nit: full sentence comments.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:152
(Diff revision 6)
> +      device = buildFxAccountDevice(context, fxAccount, message.getBundle("subscription"));
> +    } else {
> +      Log.i(LOG_TAG, "Push registration failed. Beginning degraded FxA Registration.");
> +      device = buildFxAccountDevice(context, fxAccount);
> +    }
> +    final boolean bumpRegistrationVersion = (error != 2154627078d); // = NS_ERROR_DOM_PUSH_GCM_DISABLED

A better name for this variable given how you're using it would be "resetRegistrationVersion".

However, I'm not convienced we should actually do this. See my top level comment.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:226
(Diff revision 6)
> +    return makeFxADeviceCommonBuilder(context, fxAccount).build();
> +  }
> +
> +  private static FxAccountDevice buildFxAccountDevice(Context context, AndroidFxAccount fxAccount, GeckoBundle subscription) {
> +    FxAccountDevice.Builder builder = makeFxADeviceCommonBuilder(context, fxAccount);
> +    if (subscription != null) {

You shouldn't be passing in null values, right? seeing a null would be a programming mistake, correct? Maybe mark the paramter as @NonNull and drop the if checks, or through an illegal argument exception for good measure.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:236
(Diff revision 6)
> +    return builder.build();
> +  }
> +
> +  // Do not call this directly, use buildFxAccountDevice instead.
> +  private static FxAccountDevice.Builder makeFxADeviceCommonBuilder(Context context, AndroidFxAccount fxAccount) {
> +    String deviceId = fxAccount.getDeviceId();

finals throughout

::: xpcom/base/ErrorList.h:961
(Diff revision 6)
>    ERROR(NS_ERROR_DOM_PUSH_DENIED_ERR,               FAILURE(2)),
>    ERROR(NS_ERROR_DOM_PUSH_ABORT_ERR,                FAILURE(3)),
>    ERROR(NS_ERROR_DOM_PUSH_SERVICE_UNREACHABLE,      FAILURE(4)),
>    ERROR(NS_ERROR_DOM_PUSH_INVALID_KEY_ERR,          FAILURE(5)),
>    ERROR(NS_ERROR_DOM_PUSH_MISMATCHED_KEY_ERR,       FAILURE(6)),
> +  ERROR(NS_ERROR_DOM_PUSH_GCM_DISABLED,             FAILURE(7)),

Can this be a java layer thing? Right now you're just passing this from one piece of java code to another. Do we need more granularity for webpush errors?
Attachment #8852107 - Flags: review?(gkruglov) → review-
Thanks Grisha, here's a new iteration of the patch. I also added tests for the new methods that were introduced.

I kept the errors defined ErrorList, because it goes through here http://searchfox.org/mozilla-central/source/dom/push/PushComponents.js#104-105 and I'd like to avoid error-code collisions.
Comment on attachment 8852107 [details]
Bug 1349299 - Register device on FxA even on Push registration failure.

https://reviewboard.mozilla.org/r/124344/#review135932

I think I'm fine with this.

I do want to highlight that we _want_ to handle more than just GCM errors. I'm fine with doing that in a follow-up bug, so let's file that. Once we start adding error handling and re-registration flows for other error types, I envision that a lot of this code will need to be refactored and generalized.

For the time being, please please augment your patch with a documentation update. `push.rst` seems like a good place.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:53
(Diff revision 7)
>    // subscriptions. This will be improved as part of Bug 1345651.
> -  private static final long TIME_BETWEEN_CHANNEL_REGISTRATION_IN_MILLIS = 21 * 24 * 60 * 60 * 1000L;
> +  @VisibleForTesting
> +  static final long TIME_BETWEEN_CHANNEL_REGISTRATION_IN_MILLIS = 21 * 24 * 60 * 60 * 1000L;
> +
> +  @VisibleForTesting
> +  static final long RETRY_TIME_AFTER_GCM_DISABLED_ERROR = 30 * 24 * 60 * 60 * 1000L;

I'm not sure how sticky these errors are; it seems like they will be _very_ sticky. It does seem that 30 days might be too infrequent on an off chance that devices are somehow able to recover from this - due to user action, perhaps? Perhaps re-try bi-weekly?

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:84
(Diff revision 7)
> -  public static boolean needToRenewRegistration(final long timestamp) {
> +  public static boolean shouldRegister(final AndroidFxAccount fxAccount) {
> +    if (fxAccount.getDeviceRegistrationVersion() != FxAccountDeviceRegistrator.DEVICE_REGISTRATION_VERSION ||
> +            TextUtils.isEmpty(fxAccount.getDeviceId())) {
> +      return true;
> +    }
> +    // At this point, we have a working up-to-date registration.

Comment here that we have an FxA device registration, but it might be a partial one, hence the second check.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:165
(Diff revision 7)
>      if (fxAccount == null) {
>        Log.e(LOG_TAG, "AndroidFxAccount is null");
>        return;
>      }
> +
> +    final double error = message.getDouble("error", 0);

Can you please take out the key names into constants? It'll make it easier to ensure things line up well.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:168
(Diff revision 7)
>      }
> +
> +    final double error = message.getDouble("error", 0);
> +    final FxAccountDevice device;
> +    if (error == 0) {
> +      fxAccount.resetDevicePushRegistrationError();

For simplicity, just reset any current errors at the top of the method. Whatever we have now, is irrelevant at this point.

_unless_ it is relevant. After all, registrations are not necessarily independent events, and so we might care what the previous state was.

As we expand on the error handling in the future, let's keep that in mind.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:244
(Diff revision 7)
> +    return makeFxADeviceCommonBuilder(context, fxAccount).build();
> +  }
> +
> +  private static FxAccountDevice buildFxAccountDevice(Context context, AndroidFxAccount fxAccount, @NonNull GeckoBundle subscription) {
> +    final FxAccountDevice.Builder builder = makeFxADeviceCommonBuilder(context, fxAccount);
> +    builder.pushCallback(subscription.getString("pushCallback"));

Any chance any of these might be missing? By error on our or autopush's side, or accident, or whatever? We really don't want to supposedely register an fxa device with a broken push state.

I'd be a bit more paranoid, and check that the subscription object actually has everything we need it to have, and do simplified registration otherwise while also logging an error.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:348
(Diff revision 7)
>          onError();
>        }
>  
>        @Override
>        public void handleSuccess(FxAccountDevice[] devices) {
> -        for (FxAccountDevice device : devices) {
> +        for (final FxAccountDevice fxaDevice : devices) {

Since you're touching this stuff, can you add a comment describing what our recovery strategy is?

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:349
(Diff revision 7)
>        }
>  
>        @Override
>        public void handleSuccess(FxAccountDevice[] devices) {
> -        for (FxAccountDevice device : devices) {
> -          if (device.isCurrentDevice) {
> +        for (final FxAccountDevice fxaDevice : devices) {
> +          if (fxaDevice.isCurrentDevice) {

Can you early continue here on `!fxaDevice.isCurrentDevice`?

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:350
(Diff revision 7)
>  
>        @Override
>        public void handleSuccess(FxAccountDevice[] devices) {
> -        for (FxAccountDevice device : devices) {
> -          if (device.isCurrentDevice) {
> -            fxAccount.setFxAUserData(device.id, 0, 0L); // Reset device registration version/timestamp
> +        for (final FxAccountDevice fxaDevice : devices) {
> +          if (fxaDevice.isCurrentDevice) {
> +            fxAccount.setFxAUserData(fxaDevice.id, 0, 0L); // Reset device registration version/timestamp

Should this also reset push registration error info?

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:355
(Diff revision 7)
> -            fxAccount.setFxAUserData(device.id, 0, 0L); // Reset device registration version/timestamp
> +            fxAccount.setFxAUserData(fxaDevice.id, 0, 0L); // Reset device registration version/timestamp
>              if (!allowRecursion) {
>                Log.d(LOG_TAG, "Failure to register a device on the second try");
>                break;
>              }
> -            doFxaRegistration(context, subscription, false);
> +            final FxAccountDevice updatedDevice = new FxAccountDevice(device.name, fxaDevice.id, device.type,

You're using the remote device's ID, which I think is different from our current behaviour (getting AndroidFxAccount from context and grabbing id there). Is that intentional?

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:831
(Diff revision 7)
> -      }
> -    }
>    }
>  
>    public synchronized long getDeviceRegistrationTimestamp() {
> -    final String timestampStr = accountManager.getUserData(account, ACCOUNT_KEY_DEVICE_REGISTRATION_TIMESTAMP);
> +    return getUserDataNumber(ACCOUNT_KEY_DEVICE_REGISTRATION_TIMESTAMP, 0l);

nit: 0L

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/authenticator/AndroidFxAccount.java:869
(Diff revision 7)
>      accountManager.setUserData(account, ACCOUNT_KEY_DEVICE_REGISTRATION_TIMESTAMP,
>              Long.toString(timestamp));
>    }
>  
> +  public synchronized void setDevicePushRegistrationError(double error, long errorTimeMs) {
> +    accountManager.setUserData(account, ACCOUNT_KEY_DEVICE_PUSH_REGISTRATION_ERROR, Double.toString(error));

I'm assuming this is only purged when:
- account is removed
- on a successful registration

I think that's fine, just make sure you gave it a thought.
Attachment #8852107 - Flags: review?(gkruglov) → review+
Comment on attachment 8852107 [details]
Bug 1349299 - Register device on FxA even on Push registration failure.

https://reviewboard.mozilla.org/r/124342/#review135962

::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java:64
(Diff revisions 6 - 7)
>      private static final String LOG_TAG = "GeckoPushService";
>  
>      public static final String SERVICE_WEBPUSH = "webpush";
>      public static final String SERVICE_FXA = "fxa";
>  
> +    public static final double ERROR_GCM_DISABLED = 2154627078l; // = NS_ERROR_DOM_PUSH_GCM_DISABLED

The use of doubles throughout here is almost certainly wrong.  You want `long` or `Long`.  (Even `int` or `Integer` is almost certainly wrong when working with JSON.)

For example, your 0 == 0 checks are suspect with `double`.

If you really think you want doubles, point me to the comment or discussion of why this is so.
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/758791de0957
Register device on FxA even on Push registration failure. r=Grisha
See Also: → 1359287
https://hg.mozilla.org/mozilla-central/rev/758791de0957
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Product: Android Background Services → Firefox for Android
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: