1400925 - Add a "reason" field to collection_changed messages

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P1)

Description

[Edouard Oger [:eoger]]

Every time a new device joins the Sync account, after the first upload of our own client record, we ask the FxA server to send a "collection_changed" push message. (bugs 1372655 1372657).
We do this to inform other clients that they should refresh their devices list immediately, so we can send tabs to that new device right away.

The problem is that we send the exact same "collection_changed" push message when we send a tab to another device.
iOS assumes that "collection_changed" == "tabs sent to me", which means in the first case it will show a blank notification, because there's no received tab to be found.

There's a few solutions to fix that problem:
- Alter the current "collection_changed" push message [0] to add a "reason" field ("sendtab", "newdevice"). On FxA, block the push messages sent to iOS that have the "newdevice" reason.
- On iOS, if a "collection_changed" message is received without a corresponding tab, show "A new device has connected". This is pretty bad: it can be very inaccurate and we already show a similar message when we receive device_connected.
- Send a completely different push message to send tabs to devices. This is the nuclear solution.

Any other ideas?

[0] https://github.com/mozilla/fxa-auth-server/blob/master/docs/pushpayloads.schema.json#L100-L137

Comment 1

[Ryan Kelly [:rfkelly]]

> - Send a completely different push message to send tabs to devices. This is the nuclear solution.

TBH I prefer this one as the longer-term path forward - add a specific "send tab" message type and stop doing it in the "clients" collection entirely.

But it may be hard to get there from here in the short term, so I'm open to an intermediate solution.

> Every time a new device joins the Sync account, after the first upload of our own client record,
> we ask the FxA server to send a "collection_changed" push message.

Given that FxA sends an "device connected" push message, is it necessary to send "collection_changed" as well in this case?  Or does it get sent at a slightly different point in the connection lifecycle that makes it work better?

Comment 2

[Edouard Oger [:eoger]]

> TBH I prefer this one as the longer-term path forward - add a specific "send tab" message type and stop doing it in the "clients" collection entirely.

I agree, but I'd like to find a solution in the meantime.

> Or does it get sent at a slightly different point in the connection lifecycle that makes it work better?

Yup, we re-sync the clients collection, and device_connected is sent at login so too early for our use-case.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(In reply to Ryan Kelly [:rfkelly] from comment #1)
> > - Send a completely different push message to send tabs to devices. This is the nuclear solution.
> 
> TBH I prefer this one as the longer-term path forward - add a specific "send
> tab" message type and stop doing it in the "clients" collection entirely.

We've chatted about this before, but my sticking point is the reliability of push. So when you say "specific ... message type", do you mean our current push system, or something else?

OTOH, I guess there's no specific reason to believe the clients collection is more reliable than push - I wonder if we should start to make some attempt at measuring this? I'm not sure what that would look like though...

Comment 4

[Ryan Kelly [:rfkelly]]

>  So when you say "specific ... message type", do you mean our current push system, or something else?

I mean our current system.  In this specific case, I feel like the reliability of push roughly matches the reliability expected of the product feature, giving best-effort delivery with some cap on lifetime of outstanding messages.

Or, might a user send a tab to a device that's currently offline, come back to that device several hours later after the push message has expired, and expect the tab to appear?

Comment 5

[James Hugman [:jhugman] [@jhugman]]

As is noted: the current set of messages and iOS' APNS are impedance mismatched: 

1. collection_changed => ['clients'] is overloaded
1b. there are redundant commands: e.g. collection_changed => ['clients'], with a WipeCommand
2. each message must display a message to the user – making (for example) collection_changed => ['history'] is essentially useless.
3. there's a limited amount of work you can do before throttling occurs
4. process separation between the message handling notification and the app makes accessing the profile database difficult. 

For send tab specifically, 
1. the displayURI commands being "collected from an inbox"
2. that sync needed to be spun up (multiple HTTP requests) just to get the URL to be displayed.

meant that the logic to receive and display sensible messages had to be much more complicated — and costly — than the we might want.

I think my personal preference/wishlist here would be to promote client commands to a new property (reason could be this property) with their own data object. For the sake of backwards compatibility, old clients would still run collection_changed=>['clients'], but newer clients would be able to more efficiently (and accurately) display the correct message.

> iOS assumes that "collection_changed" == "tabs sent to me", which means in the first case it will show a blank notification, because there's no received tab to be found.
This isn't quite true. A blank notification is a bug, likely a regression.

In the case where collection_changed => ['clients'] is sent, and no tab is found (either that tab has been consumed by another sync, or a command other than displayURI has been sent), then either a previous send tab notification is recycled or the "Tap to begin" notification is shown.

Comment 6

[Edouard Oger [:eoger]]

Are there any objections preventing us from moving on with the original plan? (add a |reason| field).

Comment 7

[Ryan Kelly [:rfkelly]]

No objections on my side

Comment 8

[Edouard Oger [:eoger]]

Server-side PR here: https://github.com/mozilla/fxa-auth-server/pull/2148

Comment 9

[Edouard Oger [:eoger]]

This has landed on the server and in the three client implementations, closing this.