Closed Bug 1329793 Opened 7 years ago Closed 7 years ago

Fennec implementation of faster "send tabs to device" using push doesn't seem to be working

Categories

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

defect

Tracking

(firefox53 fixed, firefox54 fixed, firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: markh, Assigned: Grisha, NeedInfo)

References

Details

(Whiteboard: [send-tab][MobileAS][fxsync])

Attachments

(3 files)

In bug 1289932 we added support for push to Fennec. However, it doesn't work on my 51.0b11 device - even when Firefox is running I need to explicitly "sync now" before the tab arrives.
I'm seeing this too now, but I remember it working on Nightly sometime before the Christmas break.
Seeing this from PushService when push message arrives:
01-09 15:20:22.682  6298  6418 I GeckoPushService: Google Play Services GCM message received; delivering.
01-09 15:20:22.682  6298  6418 W GeckoPushService: Cannot find registration corresponding to subscription for chid: 4990afee2fe44a989a7718b811069a80; ignoring message.
(In reply to Richard Newman [:rnewman] from comment #3)
> Related to Bug 1289629?

Doesn't seem like it. Peeking into PushManager a bit, it seems that it has a subscription matching the chid, except that it's keyed with a "dashed" uuid of sorts, while incoming chid doesn't have any dashes.
E.g. we have a registration with a subscription with chid="b7760c65-50ac-43b9-8fc6-1ce013d548f5". However, our incoming chid is "b7760c6550ac43b98fc61ce013d548f5".
Logging out and logging back in fixes this for me locally - channel ID we get back from autopush client while subscribing to a channel comes back in a general shape of something like "b7760c6550ac43b98fc61ce013d548f5" (no dashes), which matches what we later see in a push message.

Could it be that channelID formatting changed at some point recently?
JR, didn't you fix Autopush to normalize channel IDs when delivering via the GCM bridge? Or is that only for new channels?
Flags: needinfo?(jrconlin)
There was a period where the server was handing back mixed form channelids, but as of https://github.com/mozilla-services/autopush/commit/8aa1a7eb1266f792538723f35569ae13e0355adc (oct) all were normalized to lower hex. If you have a channel that may have been registered around mid October last year, it may have had dashes in the response.
Flags: needinfo?(jrconlin)
Thanks, JR. From comment 1, it sounds like this was working for Grisha before Christmas; is there a chance something changed since the new year?

I wonder if it would help to drop router registrations for all these old devices. It seems like we'll do the right thing (http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/dom/push/PushServiceAndroidGCM.jsm#176-200), though that'll require relaunching Fennec.

FTR, the client never generates its own IDs: it uses whatever the server gives it, and treats it as opaque. If the client got a dashed channel ID in the registration response, but the server later delivers to a non-dashed ID, that message will be ignored. I think we talked about having the client parse the ID as a UUID the last time this came up, but decided against that. Should we reconsider?
(In reply to Kit Cambridge [:kitcambridge] from comment #8)
> FTR, the client never generates its own IDs: it uses whatever the server
> gives it, and treats it as opaque. If the client got a dashed channel ID in
> the registration response, but the server later delivers to a non-dashed ID,
> that message will be ignored.

This is what's happening now for some clients. Ideally, we need to get "proper" channelIDs to these clients. If we can get them to re-register and obtain a new, working channelID, that seems like a good first step.

Having clients parse channel IDs as UUIDs might have prevented this... I don't have enough context around this though, but it seems more like a clutch. Different parts of the system should be in agreement over formatting of such things.

I'm not very familiar with this side of things, what would be involved in "drop[ping] router registrations for all these old devices"?
Flags: needinfo?(jrconlin)
> FTR, the client never generates its own IDs: it uses whatever the server
> gives it, and treats it as opaque. If the client got a dashed channel ID in
> the registration response, but the server later delivers to a non-dashed ID,
> that message will be ignored. I think we talked about having the client
> parse the ID as a UUID the last time this came up, but decided against that.
> Should we reconsider?

It'll be much better for everybody if we treat UUID's as opaque.  Please don't parse them or assume they're UUID-shaped.
The push server hasn't touched the UUID or GCM path since the october fix rolled out. If things are suddenly not working, I'm at a loss to know why. We've got code, tests and comments to make sure that the CHID we generate matches the CHID we distribute to. 

The problem, if I recall, was that we were returning a "dashed" UUID for registration, and then an "undashed" UUID when we send the notification event. One thing we currently do is reject any "dashed" CHID UUIDs during registration and instead return an "undashed" CHID UUID. That's been in production since late October as well.

The only thing I can think of is that the subscription request is old, an the server has requested a subscription request, which should generate a new UAID/CHID pair and endpoint. All of these should be using the link format from October. It may be that those links are older and have either stored dashes and are not updating correctly.

FWIW, Push to Tab works fine between nightly on desktop and android for me, but then, I'm a data point of one.
Flags: needinfo?(jrconlin)
Assignee: nobody → gkruglov
Status: NEW → ASSIGNED
Iteration: --- → 1.13
Priority: -- → P1
I often see this fail too when sending tabs from the desktop to my phone. However it's working some times. So for example when sending a tab after not using the app for some time, it does not work and I have to sync manually to receive the tab. However sending another tab right after that often succeeds. So at least for me it's not permanently broken.
Iteration: 1.13 → 1.14
(In reply to Sebastian Kaspari (:sebastian) from comment #12)
> I often see this fail too when sending tabs from the desktop to my phone.
> However it's working some times. So for example when sending a tab after not
> using the app for some time, it does not work and I have to sync manually to
> receive the tab. However sending another tab right after that often
> succeeds. So at least for me it's not permanently broken.

Some notes from looking at logcat:

Whenever I've seen it "not work", it tends to be 1) delayed, 2) when gecko is running but haven't been used for a while. Specifically, I see that push messages comes in almost momentarily, but it takes a while to hear back from gecko with a decoded payload. Sample logs, note the 1 minute gap:

> 02-06 10:13:09.038  4887  7249 D GeckoPushGCM: Message received.  Processing on background thread.
> 02-06 10:13:09.039  4887  5483 I GeckoPushService: Google Play Services GCM message received; delivering.
> 02-06 10:13:09.039  4887  5483 I GeckoPushService: Message directed to service: fxa
> 02-06 10:13:09.039  4887  5483 D Telemetry: SendUIEvent: event = action.1 method = service timestamp = 1647855 extras = dom-push-api
> 02-06 10:13:09.042  4887  5483 I GeckoPushService: Delivering dom/push message to decode to Gecko!
> 02-06 10:13:09.043  4887  5485 I GeckoFxAccountsPush: FxAccountsPush _decodePushMessage
> 
> [... bunch of stuff unrelated to fennec/gecko...]
> 
> 02-06 10:14:04.717  4887  5483 I GeckoPushService: Handling event: FxAccountsPush:ReceivedPushMessageToDecode:Response
> 02-06 10:14:04.717  4887  5483 I FxAccountPush: Handling FxA Push Message
> 02-06 10:14:04.719  4887  5483 I GeckoLogger: fennec :: FirefoxAccounts :: Requesting sync.

This is what happens during the "gap": https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/FxAccountsPush.js?q=FxAccountsPush+_decodePushMessage&redirect_type=single#105-120
Iteration: 1.14 → 1.15
See Also: → 1337910
From an IRC conversation with jrconlin:

- some percentage of folks are affected by the "dashed channelid" issue (these will be somewhat older accounts, as the format change happened around October), causing them to ignore incoming push messages. They need to be re-subscribed in order to receive a new (and correctly formatted) channelid.
- users with accounts which haven't been used in a while might have expired subscriptions, and they need to be re-subscribed as well.
- clients might need a mechanism to re-subscribe periodically or at various points when it's determined (or likely) that their subscription isn't valid.

I'll leave this bug to further nail down and track work on the points above.

Comment 13 describes a separate issue; I filed Bug 1337910 to track it.
Iteration: 1.15 → 1.16
As I was testing the patch in Comment 12, two things stood out:
1) my current hotel's wifi network (I'm assuming) provided such poor GCM deliverability that it almost seemed that things aren't working. Maybe 1 in 10-15 messages arrived
2) channel unsubscription doesn't seem to be working. I'm always seeing 410 GONE responses to client's DELETEs. But this is unrelated to the patch.
Comment on attachment 8843561 [details]
Bug 1329793 - Re-subscribe for a push channel periodically

https://reviewboard.mozilla.org/r/117236/#review118960

LGTM, I would feel more confident if nalexander could review this too.
Attachment #8843561 - Flags: review?(eoger) → review+
Attachment #8843561 - Flags: review?(nalexander)
Nick, could you please take a look as well? As per conversation with jrconlin, this "renews" an fxa service push subscription a bit more than once a month.

Ryan, the above change will add additional POSTs to the device endpoint. Do you think we need to fuzz or stagger the time intervals a bit, so that the additional POST request load is a bit more spread out initially? It seems to me that this should be a pretty low impact though.
Flags: needinfo?(rfkelly)
> Do you think we need to fuzz or stagger the time intervals a bit, so that the additional POST request load is a bit more spread out initially?

I think it will be fine as-is, esp. given it's unique to existing android clients.  We survive similar events on desktop OK.
Flags: needinfo?(rfkelly)
Iteration: 1.16 → 1.17
Should we do this for content ("webpush") subscriptions, too? Not necessarily in this bug; just curious.
Isn't this the responsibility of the website?
(In reply to Edouard Oger [:eoger] from comment #22)
> Isn't this the responsibility of the website?

Yep, the site needs to call `pushManager.subscribe()`, but we need to fire a `pushsubscriptionchange` event so that it knows when to do that. Maybe http://searchfox.org/mozilla-central/rev/e844f7b79d6c14f45478dc9ea1d7f7f82f94fba6/dom/push/PushServiceAndroidGCM.jsm#178-183 already does the right thing, though. Will old push subscriptions be removed from the list returned by `DumpSubscriptions`?
(In reply to Kit Cambridge [:kitcambridge] from comment #21)
> Should we do this for content ("webpush") subscriptions, too? Not
> necessarily in this bug; just curious.

I also want to understand what led us to this solution.  As I originally envisioned it (and I _think_, wrote it!), Fennec would maintain webpush subscriptions as long as Gecko reported them (via the `DumpSubscriptions` mechanism) and would maintain all other subscriptions indefinitely.  That lets  Gecko implement whatever quota mechanism it sees fit while decoupling other service types (Sync, FxA, possible future updates and product announcements, etc) from the quota mechanism.  Somehow this isn't enough, and I don't understand why it's not enough.  So, before I review the details, help me understand what the context is here.

eoger?  grisha?
Flags: needinfo?(gkruglov)
Flags: needinfo?(eoger)
Nick, does Comment 14 clarify things wrt to fxa service stuff?

It is my understanding that subscriptions which aren't used (nothing is flowing through the channel) expire after some time. In case of the FxA service, IIRC subscription which Android client receives from autopush and then registers with FxA is only used for send tab functionality (and perhaps for confirming logins?). Given that we're just starting to expose send tab functionality via desktop UI, it's likely that these android fxa subscriptions haven't been used in a while by most clients, and so are possibly not valid. Also, we want to rely on them to be valid after a period of inactivity.

(I assume we'll see 410 autopush errors in those cases on the fxa server side)

My somewhat crude solution to this is the patch above, which performs a periodic (~ every 21 days) "renewal" of a subscription (unsubscribe+subscribe), and registers the new endpoint with FxA. Right now for simplicity and upliftability it ignores the fact that the subscription might be just fine (actively used). This would make a good follow-up bug.

(also, I take it you meant to say "Gecko maintains webpush subscriptions as long as Fennec reported them"?)

As for webpush, as far as I can tell we do not have a mechanism of expiring old subscriptions on the Android side so that they're removed from the DumpSubscriptions result set. Perhaps we should look into this in another follow-up bug? If I'm reading Comment 23 correctly, we might need something like this to fire pushsubscriptionchange at the right times.
Flags: needinfo?(gkruglov)
> subscription which Android client receives from autopush and then registers with FxA is
> only used for send tab functionality (and perhaps for confirming logins?)

The list of commands you might receive through this channel is at https://github.com/mozilla/fxa-auth-server/blob/master/docs/pushpayloads.schema.json#L25
Thanks, Ryan. It seems like Android only handles `deviceDisconnected` and `collectionsChanged` changed commands (https://dxr.mozilla.org/mozilla-central/source/mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountPushHandler.java#18).
Comment on attachment 8843561 [details]
Bug 1329793 - Re-subscribe for a push channel periodically

https://reviewboard.mozilla.org/r/117236/#review119820

::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java:108
(Diff revision 2)
>      private boolean isReadyFxAccountsPush = false;
>      private final List<JSONObject> pendingPushMessages;
> +    private final Context context;
>  
>      public PushService(Context context) {
> +        this.context = context;

It's not clear to me that `context` will be the long lived `getApplicationContext()` that I think you're expecting it is.  I don't think it's always the case that `context.getApplicationContext()` will achieve what you want, since that appears to return `null` in some situations -- situations which you are likely to witness, since `PushService()` is created in many different situations by the system.  It's not mandatory for you to address this, since any issue is probably already manifest in the existing code -- I see `context` being stored in a variety of places -- but it's a concern that you should try to think through.

::: mobile/android/components/FxAccountsPush.js:44
(Diff revision 2)
>          } else if (data === "android-fxa-unsubscribe") {
>            this._unsubscribe();
> +        } else if (data === "android-fxa-resubscribe") {
> +          this._unsubscribe()
> +            .then(this._subscribe)
> +            // if unsubscription fails, we still want to try to subscribe.

nit: Capital "If ...".

If you get an exception in the `.then`, doesn't the `.catch` try to subscribe again?  That is, this is equivalent to `(unsubscribe.then(subscribe)).catch(subscribe)`, is it not?

Is this what you want?

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FirefoxAccounts.java:226
(Diff revision 2)
>    public static void removeSyncStatusListener(SyncStatusListener syncStatusListener) {
>      // stopObserving null-checks its argument.
>      FxAccountSyncStatusHelper.getInstance().stopObserving(syncStatusListener);
>    }
> +
> +  public static SharedPreferences getFxaPrefs(final Context context) {

Add a comment about what you expect to store here: transient timestamps, caches, etc.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:44
(Diff revision 2)
>   */
>  public class FxAccountDeviceRegistrator implements BundleEventListener {
>    private static final String LOG_TAG = "FxADeviceRegistrator";
>  
> +  private static final String KEY_DEVICE_REGISTRATION_TIMESTAMP = "deviceRegistrationTimestamp";
> +  // We expect that our subscriptions might expire every month or so. This defines how often we'll

Be definitive:  The autopush endpoint expires stale channel subscriptions every 30 days (at a set time during the month, although we don't depend on this).  To avoid the FxA-specific channel silently expiring from underneath us, we unsubscribe and resubscribe every ...

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:90
(Diff revision 2)
>    }
>  
> +  public static void renewRegistration(Context context) {
> +    Context appContext = context.getApplicationContext();
> +    try {
> +      getInstance(appContext).beginReRegistration(appContext);

Can we call this something less awkward than `beginReRegistration`?  Maybe `reregister`, or `renewRegistration`?

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:106
(Diff revision 2)
>      context.startService(geckoIntent);
>      // -> handleMessage()
>    }
>  
> +  private void beginReRegistration(Context context) {
> +    // Same as registration above, but first we unsubscribe first in order to get a fresh subscription

nit: full sentence, remove duplicate first: "Same as registration, but unsubscribe first to get a fresh subscription."

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:203
(Diff revision 2)
>        @Override
>        public void handleSuccess(FxAccountDevice result) {
>          Log.i(LOG_TAG, "Device registration complete");
>          Logger.pii(LOG_TAG, "Registered device ID: " + result.id);
>          fxAccount.setFxAUserData(result.id, DEVICE_REGISTRATION_VERSION);
> +        // Remember when we completed the registration, so that we'll know when to renew it.

Why are we not using the existing `AndroidFxAccount` user data approach here?

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/receivers/FxAccountDeletedService.java:47
(Diff revision 2)
>      // We have an in-memory accounts cache which we use for a variety of tasks; it needs to be cleared.
>      // It should be fine to invalidate it before doing anything else, as the tasks below do not rely
>      // on this data.
>      AndroidFxAccount.invalidateCaches();
>  
> +    FirefoxAccounts.getFxaPrefs(getApplicationContext()).edit().clear().apply();

If you used the same mechanism as the device registration ID/version number, this would be folded into `invalidateCaches`.

::: mobile/android/services/src/main/java/org/mozilla/gecko/sync/SharedPreferencesClientsDataDelegate.java:65
(Diff revision 2)
>  
>      // Update the FxA device registration
>      final Account account = FirefoxAccounts.getFirefoxAccount(context);
>      if (account != null) {
>        final AndroidFxAccount fxAccount = new AndroidFxAccount(context, account);
>        fxAccount.resetDeviceRegistrationVersion();

If you used the same mechanism as the device ID/version number, this would be folded into `AndroidFxAccount`.
Comment on attachment 8843561 [details]
Bug 1329793 - Re-subscribe for a push channel periodically

https://reviewboard.mozilla.org/r/117236/#review119826

Blast, didn't get a chance to comment the first time.

I'm basically OK with this, although I have a few high-level thoughts that I'd like to see incorporated or for you to explicitly elect not to do.  (I'll stamp after taking into account your updates, so leaving the flag for now.)

- I think using a new portion of `SharedPreferences` is unnecessary.  It seems you could use the existing `{get,set}FxAUserData` with no repurcussions.  And as a bonus, you'd be outside of the clear App data loop.
- Consider looking ahead to when you want to refresh all "webpush" service channel subscriptions.  In that case, I would want to add a "last refreshed" (and possibly an optional "first subscribed") timestamp to _all_ subscriptions (in `PushSubscription.java`, I guess), and add the various timestamp testing and maintainence functions there.  Then, you'd build the FxA-specific functionality out of that functionality, and sometime in the future we can work out the WebPush equivalent.
- I'd like you to summarize the long discussion we had about the operational failure we're seeing with the FxA service, comparing and contrasting to the WebPush situation.  That is, highlight that the browser is positioned to act as the WebPush-subscribed origin, and to use an out-of-band channel (the FxA device registration HTTP API) to refresh the channel subscription.  A link to the WebPush ticket you intend to file would be good.
See Also: → 1345651
Comment on attachment 8843561 [details]
Bug 1329793 - Re-subscribe for a push channel periodically

https://reviewboard.mozilla.org/r/117236/#review119820

> It's not clear to me that `context` will be the long lived `getApplicationContext()` that I think you're expecting it is.  I don't think it's always the case that `context.getApplicationContext()` will achieve what you want, since that appears to return `null` in some situations -- situations which you are likely to witness, since `PushService()` is created in many different situations by the system.  It's not mandatory for you to address this, since any issue is probably already manifest in the existing code -- I see `context` being stored in a variety of places -- but it's a concern that you should try to think through.

Addressed this in a comment. Briefly, it _should_ be ok to use a short-lived context in this case.

> Why are we not using the existing `AndroidFxAccount` user data approach here?

No good reason, simply because I _thought_ I couldn't get at one during startup, but that's clearly not the case. Using account's setUserData now.

> If you used the same mechanism as the device registration ID/version number, this would be folded into `invalidateCaches`.

Now that I'm using account data storage, it doesn't seem necessary (nor possible), seeing that account has been deleted at this point.
Pushed an update to the patch: do not begin registration renewal on startup if fxaccount is not in a Married state. FxA state machine isn't advanced in this process, so unless we're already in the Married state, we won't be able to do anything anyway.
Comment on attachment 8843561 [details]
Bug 1329793 - Re-subscribe for a push channel periodically

https://reviewboard.mozilla.org/r/117236/#review120678

Some nits, but this looks good to me.  If it works for you, it works for me!

::: mobile/android/base/java/org/mozilla/gecko/push/PushService.java:156
(Diff revision 5)
> +            // Nothing to renew if there isn't an account.
> +            if (fxAccounts.length == 0) {
> +                return;
> +            }
> +
> +            // Defensively obtain account state. We are in a startup situation: try to not crash.

Strictly speaking, you could do this from an `Engaged` state.  It looks like you just need a `sessionToken` to talk to the device registration API, which more states than just `Married` have.  In any case, just make a note of this and don't worry about it.

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:46
(Diff revision 5)
>    private static final String LOG_TAG = "FxADeviceRegistrator";
>  
> +  // The autopush endpoint expires stale channel subscriptions every 30 days (at a set time during
> +  // the month, although we don't depend on this). To avoid the FxA service channel silently
> +  // expiring from underneath us, we unsubscribe and resubscribe every 21 days.
> +  // Note that simple schedule means that might unsubscribe perfectly valid (but old) subscriptions.

nit: Note that _this_ simple schedule...

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:73
(Diff revision 5)
>    }
>  
> +  public static boolean needToRenewRegistration(final long timestamp) {
> +    // NB: we're comparing wall clock to wall clock, at different points in time.
> +    // It's possible that wall clocks have changed, and our comparison will be meaningless.
> +    // However, this happens in context of a sync, and we won't be able to sync anyways if our wall

nit: in _the_ context of ...

::: mobile/android/services/src/main/java/org/mozilla/gecko/fxa/FxAccountDeviceRegistrator.java:74
(Diff revision 5)
>  
> +  public static boolean needToRenewRegistration(final long timestamp) {
> +    // NB: we're comparing wall clock to wall clock, at different points in time.
> +    // It's possible that wall clocks have changed, and our comparison will be meaningless.
> +    // However, this happens in context of a sync, and we won't be able to sync anyways if our wall
> +    // clock deviates too much from "actual time".

You could say the "time on the server".
Attachment #8843561 - Flags: review?(nalexander) → review+
> Strictly speaking, you could do this from an `Engaged` state.  It looks like you just need a `sessionToken`
> to talk to the device registration API, which more states than just `Married` have.

If I understand your client states correctly, doing device-registration stuff from the `Engaged` state would let you also receive push notifications when the account and/or session gets verified.  Separate bug obviously, but worth keeping in mind.
(In reply to Ryan Kelly [:rfkelly] from comment #36)
> > Strictly speaking, you could do this from an `Engaged` state.  It looks like you just need a `sessionToken`
> > to talk to the device registration API, which more states than just `Married` have.
> 
> If I understand your client states correctly, doing device-registration
> stuff from the `Engaged` state would let you also receive push notifications
> when the account and/or session gets verified.  Separate bug obviously, but
> worth keeping in mind.

Correct.  This is more clear on iOS, where we separated the states (there are about 9, IIRC) more than on Android (where there are about 5, IIRC).  The advantage is that the Android states have fun real-world names, whereas the iOS states have boring informative names (like TokenBeforeVerificationBoringState).
(In reply to Ryan Kelly [:rfkelly] from comment #36)
> If I understand your client states correctly, doing device-registration
> stuff from the `Engaged` state would let you also receive push notifications
> when the account and/or session gets verified.  Separate bug obviously, but
> worth keeping in mind.

Agreed that this would be great to have - I was just chatting with eoger about this today. I'm going to address this separately though, not as a side-effect of this change.
Flags: needinfo?(eoger)
See Also: → 1346061
Pushed by gkruglov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1b2d90a65aec
Re-subscribe for a push channel periodically r=eoger,nalexander
https://hg.mozilla.org/mozilla-central/rev/1b2d90a65aec
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1346362
Depends on: 1346390
Send Tab improvements and promotion is planned for 53, so let's uplift this as early in the cycle as possible.

[Feature/Bug causing the regression]: Bug 1295348, push-based send tabs

[User impact if declined]: Push support for Send Tabs might stop working after a prolonged period of device inactivity. Also, depending on account's age, they might not have a valid push registration at all, and this patch fixes that.

[Is this code covered by automated tests?]: Somewhat, but not this specific functionality.

[Has the fix been verified in Nightly?]: Verified that devices continue to receive push messages when tabs are sent after upgrading to nightly. On first run after upgrading to this patch, device will act as if it has a stale device registration (older than 21 days), and will re-subscribe. We'll need to wait 20 or so days from now to confirm that devices re-subscribe correctly at that point as well.
Also verified that this patch (nightly upgrade) unblocked an old install which was stuck without a valid registration and wouldn't receive push messages from FxA.

[Needs manual test from QE? If yes, steps to reproduce]: Not needed.

[List of other uplifts needed for the feature/fix]: Bug 1346390, a one-line [findbugs] pacifying patch.

[Is the change risky?]: Medium/low

[Why is the change risky/not risky?]: Patch itself is simple, but it does introduce a new flow which runs on startup and during sync and conditionally sends off two additional network requests.

[String changes made/needed]: n/a
Attachment #8846150 - Flags: approval-mozilla-aurora?
(patch needed a rebase for beta, but otherwise request is the same as for aurora uplift)

Send Tab improvements and promotion is planned for 53, so let's uplift this as early in the cycle as possible.

[Feature/Bug causing the regression]: Bug 1295348, push-based send tabs

[User impact if declined]: Push support for Send Tabs might stop working after a prolonged period of device inactivity. Also, depending on account's age, they might not have a valid push registration at all, and this patch fixes that.

[Is this code covered by automated tests?]: Somewhat, but not this specific functionality.

[Has the fix been verified in Nightly?]: Verified that devices continue to receive push messages when tabs are sent after upgrading to nightly. On first run after upgrading to this patch, device will act as if it has a stale device registration (older than 21 days), and will re-subscribe. We'll need to wait 20 or so days from now to confirm that devices re-subscribe correctly at that point as well.
Also verified that this patch (nightly upgrade) unblocked an old install which was stuck without a valid registration and wouldn't receive push messages from FxA.

[Needs manual test from QE? If yes, steps to reproduce]: Not needed.

[List of other uplifts needed for the feature/fix]: Bug 1346390, a one-line [findbugs] pacifying patch.

[Is the change risky?]: Medium/low

[Why is the change risky/not risky?]: Patch itself is simple, but it does introduce a new flow which runs on startup and during sync and conditionally sends off two additional network requests.

[String changes made/needed]: n/a
Attachment #8846152 - Flags: approval-mozilla-beta?
Since the patch is huge and the feature is visible to users, I would like to have QA's help on this before uplifting to 53/54.

Hi Brindusa, could you help to verify if the feature(send tabs to device) works well and the patch is fixed as expected?
Brindusa, see comment 44.
Flags: needinfo?(brindusa.tot)
Hey, As Brindusa is part of the Desktop team i will take this under Fennec QA side.
Flags: needinfo?(brindusa.tot) → needinfo?(ioana.chiorean)
(In reply to Ioana Chiorean from comment #46)
> Hey, As Brindusa is part of the Desktop team i will take this under Fennec
> QA side.

Let me know if you need any help testing this. Besides the usual push-based "send tabs" testing, there are a few general cases here that are particularly affected by this change. Some of them:
1) Updating existing logged in installation to this patch for the first time will cause device registration update. Consequently, pushing tabs should work.
2) Updating existing non-logged in installation to this patch shouldn't do anything.
3) Updating existing logged in but not verified installation to this patch should do anything.
4) Launching or syncing fennec 21 days after updating to this patch will also cause device registration update. Unfortunately you can't just adjust device's time to test this, as sync won't work once time is skewed too much. If you'd like, I can provide you a custom build which has this period adjusted to a few minutes instead of 21 days, with no other changes. This will force device registration update to happen every few minutes, during Fennec startup and/or during sync.
Ioana, do you have any updates as the the ETA for QA work?
Comment on attachment 8846150 [details] [diff] [review]
resub-push-sub-aurora.patch

At this point, let's uplift now to aurora and beta, and test on beta. If there are problems, we may need to back this out from beta and live with the problem for one more cycle.
Attachment #8846150 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8846152 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
hey Grisha, this caused https://treeherder.mozilla.org/logviewer.html#?job_id=85491326&repo=mozilla-beta on beta, could you take a look and maybe provide a follow up fix that we could uplift.

Thanks!
Flags: needinfo?(gkruglov)
My bad - it seems that I've never requested uplift for Bug 1346390. Done.
Flags: needinfo?(gkruglov)
Tested with Huawei MediaPad M2 (Android 5.1.1) and HTC Desire 820 (Android 6.0.1) on 53.0b5, also Nightly 55.0a1 on Win 10.

I received the tabs without have to sync again manually, on win 10 and on the android devices.
If I wasn't logged in I didn't see the option to send tabs.
On HTC I didn't receive any tabs from Huawei, but we will investigate more tomorrow on more devices.
Depends on: 1359279
Depends on: 1389175
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

Created:
Updated:
Size: